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: serialize typing classes assigned to fields #963

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lokmeinmatz
Copy link

@lokmeinmatz lokmeinmatz commented Oct 12, 2024

We had a case where a function input used an lru_cache as attribute, not as a decorator, and lru_cache somewhere internally stores the return type of the function... and typing.Sequence for example has an attribute slots but that is None, so the serialization failed every time.

If someone has a better idea how to catch that or what types should be serialized to, I'd be glad to get input :)

Thanks!


Important

Fix serialization of typing classes with __slots__ in EventSerializer and add corresponding test.

  • Behavior:
    • In serializer.py, EventSerializer.default() now checks if obj is a type or a generic alias and serializes it as "<obj.__name__>".
    • Handles cases where __slots__ is None to prevent serialization failures.
  • Tests:
    • Adds test_slots_types() in test_serializer.py to verify serialization of typing classes assigned to fields.

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

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 79a79c3 in 17 seconds

More details
  • Looked at 52 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_FZDLNpJnW94ouowJ


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

langfuse/serializer.py Outdated Show resolved Hide resolved
Copy link

@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

This pull request addresses a serialization issue for typing classes assigned to fields, particularly when using lru_cache as an attribute.

  • Added check for types and generic aliases in langfuse/serializer.py to prevent serialization failures
  • Implemented serialization of typing classes to string representations (e.g., '')
  • Added test_slots_types in tests/test_serializer.py to verify correct handling of typing.Sequence[int]
  • Improved robustness of EventSerializer for edge cases involving lru_cache and typing classes
  • Consider exploring alternative approaches for more precise type representation in serialization

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

langfuse/serializer.py Outdated Show resolved Hide resolved
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.

4 participants