Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Problem with langfuse_tags when using litellm proxy with langfus… #7825

Merged
merged 3 commits into from
Jan 18, 2025

Conversation

yuu341
Copy link
Contributor

@yuu341 yuu341 commented Jan 17, 2025

Title

Fix: Problem with langfuse_tags when using litellm proxy with langfuse integration

Relevant issues

Fixes #7801

Type

🐛 Bug Fix

Changes

  • Updated the tags processing logic to parse langfuse_tags using json.loads() to ensure it is properly handled as a list.

[REQUIRED] Testing - Attach a screenshot of any new tests passing locally

If UI changes, send a screenshot/GIF of working UI fixes

Copy link

vercel bot commented Jan 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 6:48pm

Copy link
Contributor

@ishaan-jaff ishaan-jaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed

litellm/integrations/langfuse/langfuse.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ishaan-jaff ishaan-jaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 more change needed

@staticmethod
def _get_langfuse_tags(metadata: dict) -> list[str]:
return json.loads(metadata.pop("tags", "[]"))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try / except this, (don't raise an exception if there is an error)

you can do `verbose_logger.exception("error getting langfuse tags %s", str(e))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concern here is json decode / encode errors

Copy link
Contributor Author

@yuu341 yuu341 Jan 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback.
If the tags cannot be converted to JSON successfully, should I convert an empty array? Or, because the severity is "error", should I avoid sending the data to Langfuse and return a non-200 status code to the client?
The previous behavior was to forcibly append to a string, so the request was treated as not being sent to Langfuse in the first place.


current code.

    @staticmethod
    def _get_langfuse_tags(metadata: dict) -> list[str]:
        try:
            return json.loads(metadata.pop("tags", "[]"))
        except Exception as e:
            verbose_logger.exception("error getting langfuse tags %s", str(e))
            return []

current logs

09:47:02 - LiteLLM:ERROR: langfuse.py:740 - error getting langfuse tags Expecting property name enclosed in double quotes: line 1 column 2 (char 1)
Traceback (most recent call last):
  File "/home/ubuntu/work/xxx/venv/lib/python3.12/site-packages/litellm/integrations/langfuse/langfuse.py", line 738, in _get_langfuse_tags
    return json.loads(metadata.pop("tags", "[]"))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/json/decoder.py", line 353, in raw_decode
    obj, end = self.scan_once(s, idx)
               ^^^^^^^^^^^^^^^^^^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning an array is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed the implementation above as is. Please review it.

Copy link
Contributor

@ishaan-jaff ishaan-jaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ishaan-jaff ishaan-jaff merged commit 7b3863b into BerriAI:main Jan 18, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Problem with langfuse_tags when using litellm proxy with langfuse integration
2 participants