fix: Handle iterators not only generators#1455
Open
omerc7 wants to merge 1 commit intolangfuse:mainfrom
Open
Conversation
7ecce95 to
daa44e8
Compare
Author
|
Hey @hassiebp, did you get a chance to take a look at this PR? Happy to address any feedback, make changes, or provide additional context if needed. |
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.
Fixes langfuse/langfuse#10797
Important
Enhance Langfuse client to handle both iterators and generators, updating relevant functions and tests for comprehensive support.
inspect.isgeneratorandinspect.isasyncgenwithisinstancechecks forIteratorandAsyncIteratorinasync_wrapperandsync_wrapperfunctions inobserve.py._wrap_sync_generator_resultand_wrap_async_generator_resultto_wrap_sync_iterator_resultand_wrap_async_iterator_result._ContextPreservedSyncGeneratorWrapperto_ContextPreservedSyncIteratorWrapper._ContextPreservedAsyncGeneratorWrapperto_ContextPreservedAsyncIteratorWrapper.async_iterable_factoryfixture intest_decorators.pyto test both async generators and iterators.test_async_generator_context_preservationto useasync_iterable_factoryfor testing context preservation with both async generators and iterators.This description was created by
for 7ecce95. You can customize this summary. It will automatically update as commits are pushed.
Disclaimer: Experimental PR review
Greptile Overview
Greptile Summary
Changed iterator detection from
inspect.isgenerator()andinspect.isasyncgen()toisinstance()checks withIteratorandAsyncIteratorfromcollections.abc. This fixes a bug where custom iterator objects (not generators) were not being properly wrapped for context preservation.Key changes:
inspect.isgenerator(result)withisinstance(result, Iterator)inspect.isasyncgen(result)withisinstance(result, AsyncIterator)The bug occurred because
inspect.isgenerator()only returnsTruefor generator objects created by generator functions, but returnsFalsefor custom iterator classes that implement__iter__()and__next__(). Usingisinstance()with ABC types correctly identifies all iterators.Confidence Score: 5/5
inspect.isgenerator()(which only detects generator objects) toisinstance()with ABC Iterator types (which detects all iterators). The fix is well-tested with a parametrized test covering both generators and custom iterators. The refactoring from "generator" to "iterator" terminology improves code clarity.Important Files Changed
File Analysis
inspect.isgenerator()andinspect.isasyncgen()withisinstance()checks usingIteratorandAsyncIteratorfromcollections.abcto properly handle custom iterators, not just generators@observedecorator, ensuring context preservationSequence Diagram
sequenceDiagram participant User participant ObserveDecorator participant Function participant IteratorWrapper participant LangfuseSpan User->>ObserveDecorator: Call @observe decorated function ObserveDecorator->>LangfuseSpan: Create span/generation ObserveDecorator->>Function: Execute function Function-->>ObserveDecorator: Return Iterator/AsyncIterator alt isinstance(result, Iterator) or isinstance(result, AsyncIterator) ObserveDecorator->>ObserveDecorator: Detect iterator type (not just generators) ObserveDecorator->>IteratorWrapper: Wrap iterator with context preservation ObserveDecorator-->>User: Return wrapped iterator Note over LangfuseSpan: Span NOT ended yet User->>IteratorWrapper: Iterate over results loop Each iteration IteratorWrapper->>Function: Get next item (in preserved context) Function-->>IteratorWrapper: Yield item IteratorWrapper-->>User: Return item end IteratorWrapper->>LangfuseSpan: Update output and end span else Regular return value ObserveDecorator->>LangfuseSpan: Update output ObserveDecorator->>LangfuseSpan: End span ObserveDecorator-->>User: Return result end