Skip to content

Conversation

@Pouyanpi
Copy link
Collaborator

@Pouyanpi Pouyanpi commented Dec 11, 2025

Description

streaming is enalbed per request:
via stream_async method
via chat CLI (--streaming)
(we still support streaming when StremingHandler is used with generate_async and generate_events_async for backward compatibility)

the error message:

nemoguardrails.exceptions.StreamingNotSupportedError: Cannot use --streaming with config `./examples/bots/abc` because output rails are configured but streaming is not enabled for them.

To fix this, either:
  1. Enable streaming for output rails by adding to your config.yml:
     rails:
       output:
         streaming:
           enabled: True

  2. Or run without the --streaming flag:
     nemoguardrails chat ./examples/bots/abc

Documentation:

#1542

@Pouyanpi Pouyanpi force-pushed the feat/drop-streaming-field branch from 01e1fab to 912fcba Compare December 11, 2025 18:36
@codecov
Copy link

codecov bot commented Dec 11, 2025

Codecov Report

❌ Patch coverage is 57.69231% with 11 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nemoguardrails/cli/chat.py 43.75% 9 Missing ⚠️
nemoguardrails/rails/llm/llmrails.py 83.33% 1 Missing ⚠️
nemoguardrails/server/api.py 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Pouyanpi Pouyanpi marked this pull request as ready for review December 12, 2025 13:38
@Pouyanpi Pouyanpi marked this pull request as draft December 12, 2025 13:39
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 12, 2025

Greptile Overview

Greptile Summary

This PR refactors streaming functionality from a static configuration field to a per-request capability, removing the deprecated streaming field from RailsConfig.

Key Changes:

  • Removed RailsConfig.streaming field and streaming_supported property from nemoguardrails/rails/llm/config.py:1376
  • Removed main_llm_supports_streaming flag from LLMRails class
  • Streaming is now configured at call-site via stream_async() method, which calls _configure_main_llm_streaming() on-demand at nemoguardrails/rails/llm/llmrails.py:1226
  • Improved error handling in CLI to catch InvalidRailsConfigurationError and provide helpful setup instructions at nemoguardrails/cli/chat.py:95
  • Simplified server API streaming logic to directly use stream_async() instead of generate_async() with streaming handler at nemoguardrails/server/api.py:429
  • Changed stream_usage to always be enabled (was conditional on config.streaming) at nemoguardrails/rails/llm/llmrails.py:379
  • Updated exception types from ValueError to InvalidRailsConfigurationError for streaming validation errors
  • Comprehensive test updates removing all references to the deprecated streaming field

Impact:

  • Breaking change: Users with streaming: True in config.yml will need to remove this field
  • Streaming is now enabled by passing --streaming flag to CLI or stream: true in API requests
  • More flexible: Each request can independently choose streaming without config changes

Confidence Score: 4/5

  • This PR is safe to merge with careful attention to the breaking change and error message handling
  • The refactoring is well-implemented with comprehensive test coverage and improved error messages. The score is 4 (not 5) due to one minor concern: the error handling in nemoguardrails/cli/chat.py:95 catches exceptions generically and then re-checks the message string, which could be fragile if error messages change
  • Pay attention to nemoguardrails/cli/chat.py for the string-based error message matching logic

Important Files Changed

File Analysis

Filename Score Overview
nemoguardrails/cli/chat.py 4/5 Improved error handling for streaming with better user-facing error messages; removed early streaming validation check
nemoguardrails/rails/llm/config.py 5/5 Removed deprecated streaming field and streaming_supported property from RailsConfig
nemoguardrails/rails/llm/llmrails.py 4/5 Moved streaming configuration to stream_async() call site; removed main_llm_supports_streaming flag; stream_usage now always enabled
nemoguardrails/server/api.py 4/5 Simplified streaming API to use stream_async() directly instead of generate_async() with streaming_handler

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI/API
    participant LLMRails
    participant Config
    participant LLM

    Note over User,LLM: Before: Streaming configured in config.yml
    User->>CLI/API: Request with streaming flag
    CLI/API->>Config: Check streaming_supported
    Config-->>CLI/API: Return boolean
    CLI/API->>LLMRails: Check main_llm_supports_streaming
    alt streaming not supported
        CLI/API->>User: WARNING: fallback to normal mode
    else streaming supported
        CLI/API->>LLMRails: generate_async(streaming_handler)
        LLMRails->>LLM: Generate with streaming
        LLM-->>CLI/API: Stream chunks
    end

    Note over User,LLM: After: Streaming enabled per request
    User->>CLI/API: Request with streaming flag
    CLI/API->>LLMRails: stream_async(messages)
    LLMRails->>LLMRails: _validate_streaming_with_output_rails()
    alt output rails without streaming config
        LLMRails-->>CLI/API: Raise InvalidRailsConfigurationError
        CLI/API-->>User: Helpful error message with fix instructions
    else valid configuration
        LLMRails->>LLMRails: _configure_main_llm_streaming(llm)
        LLMRails->>LLM: Set streaming=True attribute
        LLMRails->>LLMRails: generate_async(streaming_handler)
        LLM-->>CLI/API: Stream chunks
        CLI/API-->>User: Display chunks
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (7)

  1. examples/configs/streaming/config.yml, line 17 (link)

    syntax: The streaming: True field has been removed from RailsConfig in this PR. This configuration file needs to be updated to remove this line.

  2. examples/configs/llm/hf_pipeline_dolly/config.yml, line 6 (link)

    syntax: The streaming: True field has been removed from RailsConfig. This configuration file needs to be updated.

  3. examples/configs/gs_content_safety/config/config.yml, line 22 (link)

    syntax: The streaming: True field has been removed from RailsConfig. This configuration file needs to be updated.

  4. examples/scripts/demo_streaming.py, line 38 (link)

    syntax: The streaming: True field has been removed from RailsConfig. This example code needs to be updated to remove this line from the YAML configuration.

  5. examples/scripts/demo_streaming.py, line 103 (link)

    syntax: The streaming: True field has been removed from RailsConfig. Remove this line from the YAML configuration.

  6. docs/configure-rails/yaml-schema/streaming/global-streaming.md, line 1-162 (link)

    syntax: This entire documentation page describes the streaming: True field which has been removed from RailsConfig. The page needs to be either removed or completely rewritten to reflect the new streaming behavior where streaming is enabled per-request via the stream_async() method rather than through global configuration.

  7. examples/notebooks/generate_events_and_streaming.ipynb, line 71 (link)

    syntax: The streaming: True field has been removed from RailsConfig. This notebook needs to be updated to remove this configuration line.

15 files reviewed, 7 comments

Edit Code Review Agent Settings | Greptile

# providers that don't support this parameter will simply ignore it
if self.config.streaming:
kwargs["stream_usage"] = True
kwargs["stream_usage"] = True
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Pouyanpi : we can move this later to llm_call when we drop the streaming callback handler feature.

@Pouyanpi Pouyanpi marked this pull request as ready for review December 12, 2025 14:08
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

15 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

- Remove `streaming` and `streaming_supported` properties from
RailsConfig
- Add `StreamingNotSupportedError` exception for clearer error handling
- Move streaming validation from config-time to runtime in LLMRails
- Remove `main_llm_supports_streaming` flag and related logic
- Update CLI to catch StreamingNotSupportedError instead of pre-checking
- Simplify server API streaming logic to use stream_async directly
- Configure LLM streaming only when stream_async is called
- Remove redundant streaming warnings and fallback logic

BREAKING CHANGE: `RailsConfig.streaming` and
`RailsConfig.streaming_supported` properties have been removed.
Streaming support is now validated at runtime when `stream_async()`
is called.
- Remove RailsConfig.streaming references from all tests
- Update streaming tests to use StreamingNotSupportedError
- Remove tests for deprecated streaming_supported property
- Remove tests for main_llm_supports_streaming flag
- Update CLI tests to verify StreamingNotSupportedError handling
- Simplify test fixtures to remove streaming config parameters
- Update token usage tests to reflect stream_usage always enabled
- Fix test mocks to align with new streaming validation approach
Replace InvalidRailsConfigurationError with a new, more specific
StreamingNotSupportedError for cases where streaming is requested but
not
supported by the configuration. Update all relevant imports, usages, and
tests to use the new exception. This improves error clarity and guidance
for users encountering streaming configuration issues.
@Pouyanpi Pouyanpi force-pushed the feat/drop-streaming-field branch from 6ab6f6e to 220cbd8 Compare December 12, 2025 15:32
Pouyanpi added a commit that referenced this pull request Dec 12, 2025
feature: #1538
Revise streaming docs to clarify usage of stream_async(), remove
outdated global streaming config, and add CLI usage instructions.
Explain output rails streaming requirements and deprecation of
StreamingHandler. Improve examples and guidance for token usage
tracking.
@Pouyanpi Pouyanpi self-assigned this Dec 12, 2025
@Pouyanpi Pouyanpi added refactoring enhancement New feature or request labels Dec 12, 2025
@Pouyanpi Pouyanpi added this to the 0.20.0 milestone Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants