Skip to content

Conversation

@hassiebp
Copy link
Contributor

@hassiebp hassiebp commented Dec 11, 2024

Important

Add utility to resolve media references by fetching media content and replacing references with base64 data URIs.

  • Behavior:
    • Adds resolve_media_references() to LangfuseMedia in media.py to replace media reference strings with base64 data URIs.
    • Adds fetch_media() to Langfuse in client.py to fetch media content by ID.
  • Tests:
    • Adds test_replace_media_reference_string_in_object() in test_media.py to verify media reference resolution.
    • Adds tests for LangfuseMedia initialization and reference string parsing in test_media.py.
  • Misc:
    • Updates ci.yml to include LANGFUSE_S3_EVENT_UPLOAD_ENDPOINT in environment variables.

This description was created by Ellipsis for 2222f02. It will automatically update as commits are pushed.

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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_references static method in langfuse/media.py to recursively traverse and replace media references with base64 content
  • Added fetch_media method in langfuse/client.py to retrieve media content by ID from the Langfuse API
  • Added comprehensive test in test_media.py covering 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:.+?@@@"
Copy link
Contributor

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

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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_ENDPOINT from http://localhost:9090 to http://host.docker.internal:9090 in .github/workflows/ci.yml to allow container access to host services

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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_ENDPOINT to http://localhost:9090 in .github/workflows/ci.yml for 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

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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

@hassiebp hassiebp merged commit 7e1ce87 into main Dec 11, 2024
8 of 9 checks passed
@hassiebp hassiebp deleted the feat-add-media-replace-utility branch December 11, 2024 18:37
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.

2 participants