-
Notifications
You must be signed in to change notification settings - Fork 8.4k
fix: improve condition check for streaming token callback in handle_on_chain_stream #11362
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
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA condition check in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (42.66%) is below the target coverage (55.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11362 +/- ##
==========================================
- Coverage 34.23% 31.64% -2.60%
==========================================
Files 1409 1414 +5
Lines 66923 67202 +279
Branches 9877 9910 +33
==========================================
- Hits 22914 21267 -1647
- Misses 42808 44719 +1911
- Partials 1201 1216 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Adam-Aghili
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.
Do we want to allow whitespace only stings, such as " "?
|
Yes, we want. That was the original issue; the chunks outputs were different from the text outputs. e.g. "PISA 2022" was becoming "PISA2022" when accumulating the chunks. |
| # Note: we should expect the callback, but we keep it optional for backwards compatibility | ||
| # as of v1.6.5 | ||
| if output_text and output_text.strip() and send_token_callback and message_id: | ||
| if output_text is not None and output_text != "" and send_token_callback and message_id: |
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 output_text is not None and output_text != "" and send_token_callback and message_id: | |
| if output_text is not None and output_text.lstrip() and output_text.rstrip() and output_text != "" and send_token_callback and message_id: |
Based on the tests failing I think it would be safe to use either lstrip or rstrip to preserve the previous intent of ignoring " " but allowing "PISA 2022".
We could use both but that seems unnecessary
This pull request makes a minor update to the logic for streaming token events in the
handle_on_chain_streamfunction. The change improves the condition that checks whether to send a token event, ensuring it is more explicit and reliable.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.