-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Langfuse integration support for parent_observation_id
parameter
#3559
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.
@hburrichter thanks for the PR - please attach a screenshot of it passing 'test_alangfuse.py'
import os |
cd litellm/litellm/tests
pytest test_alangfuse.py -x -vv
litellm/integrations/langfuse.py
Outdated
@@ -437,6 +437,7 @@ def _log_langfuse_v2( | |||
generation_params = { | |||
"name": generation_name, | |||
"id": clean_metadata.pop("generation_id", generation_id), | |||
"parent_observation_id": metadata.get("parent_observation_id"), |
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.
this would raise an error if the value is None i believe, as the dictionary would now contain a 'None' object
can you instead have this below in an if metadata.get('parent_observation_id') is not None statement
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 this PR branch in production in our application it does not raise an error.
But I will add it anyway to make sure, that future Langfuse updates do not potentially break this!
Bump on this, I'd like to see this in prod soon :D |
@hburrichter is this ready for review? |
If you can share a screenshot of it passing
|
bump on this? @hburrichter |
Sorry, guys, I have been extremely busy the last 10 days and did not get the tests up and running on the first try. I will try again this evening or tomorrow. |
Title
Langfuse integration support for
parent_observation_id
parameterRelevant issues
Fixes #3558
Related to #1832
Type
🆕 New Feature
📖 Documentation
Changes
LangFuseLogger
now receives theparent_observation_id
metadata value and passes it along to the Langfuse generation object.Testing
Notes
Pre-Submission Checklist (optional but appreciated):
OS Tests (optional but appreciated):