-
Notifications
You must be signed in to change notification settings - Fork 223
feat(media): utility to resolve media references with media content #1036
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
Conversation
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.
Disclaimer: Experimental PR review
PR Summary
Added a new media reference resolution utility to the LangfuseMedia class for converting media references back to base64 data URIs.
- Added
resolve_media_referencesstatic method inlangfuse/media.pyto recursively traverse and replace media references with base64 content - Added
fetch_mediamethod inlangfuse/client.pyto retrieve media content by ID from the Langfuse API - Added comprehensive test in
test_media.pycovering the full workflow from trace creation to media resolution - Implemented regex-based media reference detection with error handling for failed media fetches
- Added depth limiting and type-specific handling for strings, arrays, dictionaries and objects with dict attributes
3 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| # Handle string with potential media references | ||
| if isinstance(obj, str): | ||
| regex = r"@@@langfuseMedia:.+?@@@" |
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.
logic: Regex pattern .+? is greedy and could match more than intended. Use [^@]+ instead
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
No major changes found since last review. The only modification is adding the S3 event upload endpoint configuration to the CI workflow, which is a minor infrastructure change that doesn't affect the core functionality described in the previous review.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
Updated the CI workflow to use host.docker.internal instead of localhost for the S3 event upload endpoint, enabling proper Docker networking for media reference resolution functionality.
- Changed
LANGFUSE_S3_EVENT_UPLOAD_ENDPOINTfromhttp://localhost:9090tohttp://host.docker.internal:9090in.github/workflows/ci.ymlto allow container access to host services
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
Based on the latest changes, here's my summary:
Changed S3 event upload endpoint configuration in CI workflow from host.docker.internal back to localhost for improved container accessibility.
- Modified
LANGFUSE_S3_EVENT_UPLOAD_ENDPOINTtohttp://localhost:9090in.github/workflows/ci.ymlfor more reliable container networking
This is a minor infrastructure change that reverts a previous modification to ensure proper networking between containers during media reference resolution testing.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
Based on the latest changes and the provided files, here's my summary of the updates:
Added comprehensive test coverage for the LangfuseMedia class in test_media.py, focusing on media handling and reference string parsing.
- Added tests for LangfuseMedia initialization with different input types (base64, bytes, file) in
test_media.py - Added validation tests for content length and SHA256 hash calculation in
test_media.py - Added test cases for reference string parsing and validation in
test_media.py - Skipped media reference resolution test due to Docker networking issues (LFE-3159)
- Added file handling tests with proper error handling for non-existent files
The changes focus on ensuring robust testing of the media handling functionality while acknowledging current infrastructure limitations.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
Important
Add utility to resolve media references by fetching media content and replacing references with base64 data URIs.
resolve_media_references()toLangfuseMediainmedia.pyto replace media reference strings with base64 data URIs.fetch_media()toLangfuseinclient.pyto fetch media content by ID.test_replace_media_reference_string_in_object()intest_media.pyto verify media reference resolution.LangfuseMediainitialization and reference string parsing intest_media.py.ci.ymlto includeLANGFUSE_S3_EVENT_UPLOAD_ENDPOINTin environment variables.This description was created by
for 2222f02. It will automatically update as commits are pushed.