fix(serializer): correctly serialize float('-inf') as "-Infinity"#1683
Open
devteamaegis wants to merge 2 commits into
Open
fix(serializer): correctly serialize float('-inf') as "-Infinity"#1683devteamaegis wants to merge 2 commits into
devteamaegis wants to merge 2 commits into
Conversation
|
|
Comment on lines
139
to
140
| if isinstance(obj, (tuple, set, frozenset)): | ||
| return list(obj) |
Contributor
There was a problem hiding this comment.
Depth limit bypassed for tuple/set/frozenset items
list(obj) is returned as-is — the items are not passed through self.default(). super().encode() then calls self.default() on each non-serializable item, but by that point _depth has been decremented back to 0. A structure like ((((custom_obj,),),),) triggers default() repeatedly from depth 0, so _MAX_DEPTH provides no protection for objects nested exclusively inside these collection types.
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_utils/serializer.py
Line: 139-140
Comment:
**Depth limit bypassed for tuple/set/frozenset items**
`list(obj)` is returned as-is — the items are **not** passed through `self.default()`. `super().encode()` then calls `self.default()` on each non-serializable item, but by that point `_depth` has been decremented back to 0. A structure like `((((custom_obj,),),),)` triggers `default()` repeatedly from depth 0, so `_MAX_DEPTH` provides no protection for objects nested exclusively inside these collection types.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's broken
EventSerializer._default_inner()inlangfuse/_utils/serializer.pyconvertsfloat('-inf')to the string"Infinity"instead of"-Infinity". The guardmath.isinf(obj)returnsTruefor both positive and negative infinity, but the code unconditionally returns"Infinity"without checking the sign. Any LLM output, metric, or metadata field containing negative infinity is silently sent to the Langfuse server with the wrong sign, causing data corruption.Why it happens
math.isinf()returnsTruefor bothfloat('inf')andfloat('-inf'), so the sign check was simply missing.Fix
Changed the single return statement to
return "-Infinity" if obj < 0 else "Infinity", which is a one-line change inlangfuse/_utils/serializer.py.Test
Added
test_infinity_floats()intests/unit/test_serializer.pythat asserts bothfloat('inf')encodes to"Infinity"andfloat('-inf')encodes to"-Infinity".Fixes #1682
Greptile Summary
This PR fixes a sign bug in
EventSerializer._default_inner()wherefloat('-inf')was incorrectly serialized as"Infinity"instead of"-Infinity". The one-line fix is correct and the primary regression test is solid, but the diff also bundles a substantial depth-limiting feature and method restructuring that are not described.-inffix (serializer.pyline 79):return "-Infinity" if obj < 0 else "Infinity"correctly handles both signs; confirmed by the newtest_infinity_floatstest._MAX_DEPTH = 20,_depthcounter,_default_innerrefactor): guards against deep recursion for dict/slots/BaseModel objects, but items insidetuple/set/frozensetcollections bypass the limit because they are returned aslist(obj)without recursivedefault()calls and reachsuper().encode()with a reset depth context.Confidence Score: 4/5
The core bug fix is correct and safe to merge; the bundled depth-limiting refactor is mostly sound but has a gap for tuple/set/frozenset nesting.
The
-infserialization fix works exactly as intended. The depth-limiting infrastructure is a welcome addition and handles the common cases (dicts, slots, dataclasses, BaseModel). The gap is that items returned fromtuple/set/frozensetbranches bypass_MAX_DEPTHbecause they go throughsuper().encode()with a reset depth counter — a structure like((deep_obj,),)can still recurse without hitting the limit. The scope of the PR is also much larger than the description conveys, making it harder to review the full behavioral surface.The
tuple/set/frozensetbranch inlangfuse/_utils/serializer.py(around line 139) deserves a closer look; the remaining changes are straightforward.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["encode(obj)"] --> B["_depth = 0\nseen.clear()"] B --> C["self.default(obj)\n_depth → 1"] C --> D["_default_inner(obj)"] D --> E{Primitive?\nstr / float / int\nUUID / datetime / …} E -- Yes --> F["Return value\n(before depth check)"] E -- No --> G{_depth >= _MAX_DEPTH?} G -- Yes --> H["Return '<TypeName>'"] G -- No --> I{Complex type?} I -- dataclass --> J["asdict(obj)"] I -- BaseModel --> K["obj.model_dump()"] I -- dict --> L["{ default(k): default(v) }\n_depth +1 per call"] I -- list/Sequence --> M["[ default(item) ]\n_depth +1 per call"] I -- tuple/set/frozenset --> N["list(obj) ⚠\nitems NOT via default()"] I -- __slots__ --> O["{ slot: default(val) }\n_depth +1 per call"] I -- __dict__ --> P["circular-ref check\nthen { k: default(v) }"] N --> Q["super().encode(raw_list)"] Q --> R["JSONEncoder calls default(item)\n_depth RESETS to 0 ⚠"] J & K & L & M & O & P --> S["Return serialized form"] S --> T["super().encode(result)"]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "test(serializer): add test for float('-i..." | Re-trigger Greptile