Skip to content

Conversation

@Pouyanpi
Copy link
Collaborator

@Pouyanpi Pouyanpi commented May 9, 2025

This PR resolves #1184 by introducing END_OF_STREAM sentinel and update streaming handling.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2025

Codecov Report

Attention: Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.62%. Comparing base (ca02cb6) to head (151f6b5).

Files with missing lines Patch % Lines
nemoguardrails/streaming.py 98.18% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1185      +/-   ##
===========================================
+ Coverage    68.57%   68.62%   +0.05%     
===========================================
  Files          161      161              
  Lines        15943    15966      +23     
===========================================
+ Hits         10933    10957      +24     
+ Misses        5010     5009       -1     
Flag Coverage Δ
python 68.62% <98.27%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
nemoguardrails/rails/llm/llmrails.py 87.12% <100.00%> (ø)
nemoguardrails/streaming.py 98.59% <98.18%> (+0.69%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Pouyanpi
Copy link
Collaborator Author

Pouyanpi commented May 9, 2025

@Pouyanpi Pouyanpi self-assigned this May 9, 2025
@Pouyanpi Pouyanpi added bug Something isn't working enhancement New feature or request labels May 9, 2025
@Pouyanpi Pouyanpi added this to the v0.14.0 milestone May 9, 2025
@Pouyanpi
Copy link
Collaborator Author

Pouyanpi commented May 9, 2025

Codecov Report

Attention: Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.44%. Comparing base (b65cf0e) to head (fb86618).

Files with missing lines Patch % Lines
nemoguardrails/streaming.py 98.18% 1 Missing ⚠️
Additional details and impacted files
🚀 New features to boost your workflow:

test_anext_with_other_runtime_error covers that, we should get 100% for once!

@Pouyanpi Pouyanpi requested review from cparisien and tgasser-nv May 9, 2025 10:21
@andompesta
Copy link
Contributor

Adding include_generation_metadata option is great, but it generates problems with output rails buffer:

  • the generate_chunk_str functions is not able to extract the text from the dictionary. Note that, without include_generation_metadata it works because chunks are just strings not dictionary
  • is it possible to not put in the buffer chunks containing END_OF_STREAM object and the "" tokens ? this is because output-rails will be triggered with 2 or 3 empty tokens added at the end of the bot message. This might cause the message to be blocked involuntary

Copy link
Collaborator

@tgasser-nv tgasser-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I like using an object to mark the end of stream rather than other values like None or "". Looks a little weird with the is comparisons in the logic rather than == but we need to check memory locations not values. Thanks!

@Pouyanpi
Copy link
Collaborator Author

Looks good! I like using an object to mark the end of stream rather than other values like None or "". Looks a little weird with the is comparisons in the logic rather than == but we need to check memory locations not values. Thanks!
I prefer is for ref: https://python-patterns.guide/python/sentinel-object/#sentinel-objects

Pouyanpi added 2 commits May 14, 2025 22:57
…ling

- Replaced inconsistent use of `None` and `""` for stream termination
  in `StreamingHandler` with a dedicated `END_OF_STREAM` sentinel object.
- Modified `push_chunk` to convert `None` to `END_OF_STREAM`.
- Updated `__anext__` to raise `StopAsyncIteration` only for `END_OF_STREAM`
  and to return empty strings or dicts with empty/None text as data.
- Adjusted `_process` to correctly handle `END_OF_STREAM` for buffering
  and queueing logic.
- Updated `on_llm_end` to use `END_OF_STREAM`.
- Revised tests in `test_streaming_handler.py` to reflect these changes,
  including how empty first tokens are handled and how `__anext__` behaves
  with various inputs.
@Pouyanpi Pouyanpi force-pushed the refactor/streaming-termination-sentinel branch from fb86618 to 151f6b5 Compare May 14, 2025 21:00
@Pouyanpi Pouyanpi merged commit 85400a5 into develop May 14, 2025
17 checks passed
@Pouyanpi Pouyanpi deleted the refactor/streaming-termination-sentinel branch May 14, 2025 21:05
Pouyanpi added a commit that referenced this pull request May 17, 2025
…ling (#1185)

* refactor(streaming): introduce END_OF_STREAM sentinel and update handling

- Replaced inconsistent use of `None` and `""` for stream termination
  in `StreamingHandler` with a dedicated `END_OF_STREAM` sentinel object.
- Modified `push_chunk` to convert `None` to `END_OF_STREAM`.
- Updated `__anext__` to raise `StopAsyncIteration` only for `END_OF_STREAM`
  and to return empty strings or dicts with empty/None text as data.
- Adjusted `_process` to correctly handle `END_OF_STREAM` for buffering
  and queueing logic.
- Updated `on_llm_end` to use `END_OF_STREAM`.
- Revised tests in `test_streaming_handler.py` to reflect these changes,
  including how empty first tokens are handled and how `__anext__` behaves
  with various inputs.

* coverage to the moon: fix missing generation_info and add more tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request status: in review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feature: Improve Robustness and Clarity of Stream Termination in StreamingHandler

6 participants