-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix error when Google returns only empty text parts #3388
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 error when Google returns only empty text parts #3388
Conversation
|
@naveen-corpusant Ay, thanks for the heads-up, I was already planning to do another release today so at least the latest version won't be broken for too long. I suppose the issue is that we raise |
|
nice yeah exactly, the else statement assert breaks things. totally my bad for not testing more extensively on this, I liked your suggestion but should have run it to catch this |
DouweM
left a comment
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.
@naveen-corpusant I agree a test would be good; since it's probably impossible to reproduce reliably, I suggest crafting a custom GenerateContentResponse and calling _process_response directly.
|
@DouweM I've addressed your comments, and added two unit tests (one for the stream iterator, and the other for processing messages). I've also verified that these tests fail on main, and pass on this PR: failing tests (for reference): uv run pytest -s tests/models/test_google.py::test_google_process_response_filters_empty_text_parts |
|
@DouweM addressed the comments! thanks for the pointer on snapshot(), that's neat :) |
In a previous commit (#3360), we ignored text parts. On further testing, this actually causes some issues because of the structure of the if/elif/else statements (sorry for not catching this earlier! I'd tested my original revisions, but your code suggestions looked solid and worked in the happy path, but did break when testing again on 1.13 now).
I'd like to add a snapshot test to this PR- the tricky part is that it's hard to get this type of error to repro (happens on one of our tests about 1 in 10 or 15 times). Let me know if you can think of a better way to test this, I'll continue down the line of snapshot testing but wanted to put something up to also notify you that there's an error in 1.13