-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed
@@ -457,7 +458,7 @@ def _log_langfuse_v2( # noqa: PLR0915 | |||
supports_costs = langfuse_version >= Version("2.7.3") | |||
supports_completion_start_time = langfuse_version >= Version("2.7.3") | |||
|
|||
tags = metadata.pop("tags", []) if supports_tags else [] | |||
tags = json.loads(metadata.pop("tags", "[]")) if supports_tags else [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make this a helper function - call it _get_langfuse_tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have split the method as per your suggestion. What do you think?
There was a problem hiding this 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", "[]")) | ||
|
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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
Title
Fix: Problem with langfuse_tags when using litellm proxy with langfuse integration
Relevant issues
Fixes #7801
Type
🐛 Bug Fix
Changes
[REQUIRED] Testing - Attach a screenshot of any new tests passing locally
If UI changes, send a screenshot/GIF of working UI fixes