Skip to content

Conversation

Pouyanpi
Copy link
Collaborator

@Pouyanpi Pouyanpi commented Jul 2, 2025

Problem

The test test_sequential_streaming_output_rails_allowed was failing with words being concatenated without spaces:

AssertionError: assert 'compliant high quality' in 'This is a safeand complianthigh qualityjoke thatshould passall checks.'

Root Cause

The issue was in _run_output_rails_in_streaming method in llmrails.py. The method was calling chunk_str_rep.split() which removed all whitespace, then tried to reconstruct spacing with f" {word}", but this lost the original token format including trailing spaces.

This created a design contradiction: the buffer strategy creates multi-word chunks (e.g., "This is a funny ", "joke but ") for efficient output rails processing, but then the code immediately split these back into individual words, defeating the purpose.

Solution

The fix was to not split the chunks at all. The buffer strategy already creates appropriately sized chunks for output rails processing, so we simply yield user_output_chunks directly from the buffer strategy.

Core Bug Fix

  • Remove unnecessary split() operations that were breaking word spacing
  • Yield chunks directly from buffer strategy preserving original token format
  • Fix both stream_first and not stream_first code paths

Interface Improvements

  • Better API Design: Refactored BufferStrategy with process_stream() method and ChunkBatch named tuple
  • Clear Separation: Distinguished between processing_context (for output rails) and user_output_chunks (for streaming)
  • Named Tuple: Replaced opaque Tuple[List[str], List[str]] with self documenting ChunkBatch

Test Coverage Improvements

  • Enhanced tests/test_buffer_strategy.py from 2 basic tests to 11 comprehensive tests
  • Added edge case coverage (empty streams, boundary conditions, input validation)
  • Added realistic token data with proper spacing validation
  • Added interface testing (process_stream() vs __call__() parity)

- fix spacing loss in streaming output where words were concatenated without spaces
- refactor BufferStrategy interface with ChunkBatch named tuple for better API design
- improve variable names and add comprehensive Google-style documentation
- add robust exception handling for streaming generation tasks
- enhance test coverage
- remove unnecessary chunk splitting that defeated buffer strategy purpose

Fixes issue where `test_sequential_streaming_output_rails_allowed` failed due to
output like "safeand complianthigh quality" instead of "safe and compliant high quality".

The root cause was in `_run_output_rails_in_streaming` calling `split()` on chunks
that already had proper spacing, then failing to reconstruct the original format.
@Pouyanpi Pouyanpi self-assigned this Jul 2, 2025
@Pouyanpi Pouyanpi added enhancement New feature or request refactoring labels Jul 2, 2025
@Pouyanpi Pouyanpi added this to the v0.15.0 milestone Jul 2, 2025
@Pouyanpi Pouyanpi added the bug Something isn't working label Jul 2, 2025
@Pouyanpi
Copy link
Collaborator Author

Pouyanpi commented Jul 2, 2025

might resolve #1197. @andompesta if you are able to verify that the issue is resolved it'd be great. I've added a test based on #1197 in this commit.

@Pouyanpi Pouyanpi requested review from Copilot and tgasser-nv July 7, 2025 11:58
@Pouyanpi Pouyanpi marked this pull request as ready for review July 7, 2025 11:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes word concatenation issues in the streaming output rails by removing unnecessary split() calls and streaming raw chunks, refactors the buffer strategy API for clearer interfaces, and adds comprehensive tests for the new behavior.

  • Remove splitting of chunk strings in llmrails.py and yield raw chunks to preserve spacing
  • Introduce ChunkBatch, process_stream(), and format_chunks() in buffer.py
  • Expand test coverage for streaming and buffer strategy behavior

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/test_streaming.py Add sequential streaming output rails test to verify spacing
tests/test_buffer_strategy.py Update buffer strategy tests for new ChunkBatch interface
nemoguardrails/rails/llm/llmrails.py Swap out split() logic for direct chunk yielding
nemoguardrails/rails/llm/buffer.py Define ChunkBatch, refactor BufferStrategy and RollingBuffer

Copy link
Member

@trebedea trebedea 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, just some minor comments!
Great to have this fix. 👍

@Pouyanpi Pouyanpi force-pushed the fix/streaming-outputrails branch from 3412598 to 91159d5 Compare July 10, 2025 10:51
@Pouyanpi
Copy link
Collaborator Author

Thank you @trebedea for the review. I applied your suggestions, it made it more readable 👍🏻

@NVIDIA-NeMo NVIDIA-NeMo deleted a comment from codecov-commenter Jul 10, 2025
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 81.25000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 69.64%. Comparing base (9cdad05) to head (58edd2f).
Report is 7 commits behind head on develop.

Files with missing lines Patch % Lines
nemoguardrails/rails/llm/llmrails.py 71.87% 9 Missing ⚠️
nemoguardrails/rails/llm/buffer.py 90.62% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1259      +/-   ##
===========================================
+ Coverage    69.57%   69.64%   +0.06%     
===========================================
  Files          161      161              
  Lines        16023    16055      +32     
===========================================
+ Hits         11148    11181      +33     
+ Misses        4875     4874       -1     
Flag Coverage Δ
python 69.64% <81.25%> (+0.06%) ⬆️

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

Files with missing lines Coverage Δ
nemoguardrails/rails/llm/buffer.py 94.54% <90.62%> (+1.86%) ⬆️
nemoguardrails/rails/llm/llmrails.py 88.88% <71.87%> (+0.05%) ⬆️

... and 2 files with indirect coverage changes

🚀 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

Codecov Report

Attention: Patch coverage is 81.25000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 69.64%. Comparing base (9cdad05) to head (58edd2f).
Report is 7 commits behind head on develop.

Files with missing lines Patch % Lines
nemoguardrails/rails/llm/llmrails.py 71.87% 9 Missing ⚠️
nemoguardrails/rails/llm/buffer.py 90.62% 3 Missing ⚠️
Additional details and impacted files
🚀 New features to boost your workflow:

it is using outdated code for llmrails.py

it does not understand that our tests cover 4 missing lines from buffer.py

@Pouyanpi Pouyanpi merged commit 0d3ddfc into develop Jul 10, 2025
18 checks passed
@Pouyanpi Pouyanpi deleted the fix/streaming-outputrails branch July 10, 2025 11:17
Pouyanpi added a commit that referenced this pull request Oct 1, 2025
…1259)

* fix(streaming): resolve word concatenation in streaming output rails

- fix spacing loss in streaming output where words were concatenated without spaces
- refactor BufferStrategy interface with ChunkBatch named tuple for better API design
- improve variable names and add comprehensive Google-style documentation
- add robust exception handling for streaming generation tasks
- enhance test coverage
- remove unnecessary chunk splitting that defeated buffer strategy purpose

Fixes issue where `test_sequential_streaming_output_rails_allowed` failed due to
output like "safeand complianthigh quality" instead of "safe and compliant high quality".

The root cause was in `_run_output_rails_in_streaming` calling `split()` on chunks
that already had proper spacing, then failing to reconstruct the original format.
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 refactoring status: in review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants