-
Notifications
You must be signed in to change notification settings - Fork 140
feat(weave): Add integration for Google Genai SDK #2399
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
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=6800472bd896aa5680ab08c39c6459561f56b551 |
| acc = value | ||
| if not acc._done: | ||
| return value | ||
| if acc != value: |
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.
Is this if correct? What if the acc is done and it's the same as the value (e.g. you return just 1 token)
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.
If i don't add this if, the first token is being accumulated twice, both in the weave trace as well as in the output of the function. I agree that this is not the best way to tackle this issue and I'm open to suggestions to improve the logic here.
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.
Are you saying that adding the accumulator modifies the output of the function? It shouldn't do that -- we should never modify the user's output
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.
Yeah, I placed the check to prevent modification of output.
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.
sorry, I don't understand.
Here's what I think I'm reading:
- If the acc is fresh, use the first value of the iterator
- While the acc is not finished, use the next values
- If acc != value, then aggregate the candidate completions
So open questions are:
- How does the above modify the user's output? It shouldn't. We should only be "listening" to the stream
- Why do we need the acc != value check? I guess there's an interaction with
_done, but I'm not sure how
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.
Done.
| raise TypeError( | ||
| f"Cannot call next on an iterator of type {type(self._iterator_or_ctx_manager)}" | ||
| ) | ||
| self._iterator_or_ctx_manager = iter(self._iterator_or_ctx_manager) # type: ignore |
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.
Why are you putting a type ignore here?
Is this safe? I think it assumes that whatever is yielded by the contextmanager is iter-able
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.
If I don't put type ignore here, its failing the mypy checks.
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.
The point of mypy is to check if it's type-safe, and the ignore here suggests that it's not. Can you please remove the ignore and fix?
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.
We added this back then when the class was extended to support context manager for Anthropic, I pushed a hacky fix
Continuing from #2078
This PR adds the autopatch integration with Google GenerativeAI SDK.
Tasks:
Examples
Non async, non streaming
Non async, streaming
Async, non streaming
Async Streaming