fix(streaming): handle parameters.stream=true on model init#1926
fix(streaming): handle parameters.stream=true on model init#1926adityasingh2400 wants to merge 1 commit into
Conversation
Closes NVIDIA-NeMo#1325. When users set parameters.stream: true directly on a model in config.yml, providers like OpenAI forward the flag to the HTTP client, which returns an AsyncStream on every call. The non-streaming completion path then crashes with 'AsyncStream' object has no attribute 'model_dump'. The _prepare_model_kwargs helper now strips the stream flag at init time and logs a clear warning pointing users at the right opt-in (streaming flag on the rails config, or per-call via the API). stream_options and an explicit stream: false are left untouched. The kwargs dict is also defensively copied so the strip does not mutate the user's RailsConfig in place. Added regression tests covering the misconfiguration, the stream_options pass-through, and the silent drop of stream: false. Signed-off-by: Aditya Singh <adisin650@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR addresses a bug where users configuring ChangesStream Parameter Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a crash that occurred when users set
|
| Filename | Overview |
|---|---|
| nemoguardrails/rails/llm/llmrails.py | Adds stream key stripping in _prepare_model_kwargs with a warning for truthy values; also switches to a defensive shallow copy of model_config.parameters to prevent in-place mutation. Warning message hardcodes stream: true even when the actual value may differ, and falsy stream values are silently dropped with no log. |
| tests/test_llmrails.py | Adds three focused regression tests covering stream: true (warns + strips), stream_options (preserved), and stream: false (silently dropped). Tests verify no mutation of the original parameters dict and correct warning emission via caplog. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_prepare_model_kwargs(model_config)"] --> B["Shallow-copy model_config.parameters\ndict(...) if parameters else {}"]
B --> C{"'stream' key\npresent?"}
C -- No --> F
C -- Yes --> D{"value\ntruthy?"}
D -- "True / 1 / ..." --> E["pop 'stream'\nlog.warning()"]
D -- "False / 0 / None" --> G["pop 'stream'\n(silent, no warning)"]
E --> F{"api_key_env_var\nset?"}
G --> F
F -- Yes --> H["Resolve env var\ninject kwargs['api_key']"]
F -- No --> I["return kwargs"]
H --> I
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
nemoguardrails/rails/llm/llmrails.py:418-427
**Silent drop of `stream: false` may confuse users**
`kwargs.pop("stream", None)` always removes the `stream` key regardless of its value, but the warning branch fires only when the value is truthy. This means a user who explicitly writes `stream: false` in their config will see the key disappear from the effective kwargs with no log at any level. If they ever try to debug why the parameter is missing, there is nothing in the logs to explain it. A `logging.debug(...)` line for the falsy path would make the behaviour fully observable without adding noise to normal operation.
### Issue 2 of 2
nemoguardrails/rails/llm/llmrails.py:418-427
The warning message hardcodes the literal string `` `stream: true` `` but the condition fires for any truthy value (e.g. `1`, a non-empty string). Embedding the actual value makes the message more actionable when a user sets a non-boolean truthy value.
```suggestion
stream_val = kwargs.pop("stream", None)
if stream_val:
log.warning(
"Ignoring `stream: %r` set in `parameters` for model %r (engine %r). "
"Setting `stream` directly on a model causes the provider to return an "
"AsyncStream object on every call, which breaks the non-streaming path. "
"Request streaming via the API or set the `streaming` flag on the rails "
"config instead.",
stream_val,
getattr(model_config, "model", None),
getattr(model_config, "engine", None),
)
```
Reviews (1): Last reviewed commit: "fix(streaming): guard against parameters..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if kwargs.pop("stream", None): | ||
| log.warning( | ||
| "Ignoring `stream: true` set in `parameters` for model %r (engine %r). " | ||
| "Setting `stream` directly on a model causes the provider to return an " | ||
| "AsyncStream object on every call, which breaks the non-streaming path. " | ||
| "Request streaming via the API or set the `streaming` flag on the rails " | ||
| "config instead.", | ||
| getattr(model_config, "model", None), | ||
| getattr(model_config, "engine", None), | ||
| ) |
There was a problem hiding this comment.
stream: false may confuse users
kwargs.pop("stream", None) always removes the stream key regardless of its value, but the warning branch fires only when the value is truthy. This means a user who explicitly writes stream: false in their config will see the key disappear from the effective kwargs with no log at any level. If they ever try to debug why the parameter is missing, there is nothing in the logs to explain it. A logging.debug(...) line for the falsy path would make the behaviour fully observable without adding noise to normal operation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/rails/llm/llmrails.py
Line: 418-427
Comment:
**Silent drop of `stream: false` may confuse users**
`kwargs.pop("stream", None)` always removes the `stream` key regardless of its value, but the warning branch fires only when the value is truthy. This means a user who explicitly writes `stream: false` in their config will see the key disappear from the effective kwargs with no log at any level. If they ever try to debug why the parameter is missing, there is nothing in the logs to explain it. A `logging.debug(...)` line for the falsy path would make the behaviour fully observable without adding noise to normal operation.
How can I resolve this? If you propose a fix, please make it concise.| if kwargs.pop("stream", None): | ||
| log.warning( | ||
| "Ignoring `stream: true` set in `parameters` for model %r (engine %r). " | ||
| "Setting `stream` directly on a model causes the provider to return an " | ||
| "AsyncStream object on every call, which breaks the non-streaming path. " | ||
| "Request streaming via the API or set the `streaming` flag on the rails " | ||
| "config instead.", | ||
| getattr(model_config, "model", None), | ||
| getattr(model_config, "engine", None), | ||
| ) |
There was a problem hiding this comment.
`stream: true` but the condition fires for any truthy value (e.g. 1, a non-empty string). Embedding the actual value makes the message more actionable when a user sets a non-boolean truthy value.
| if kwargs.pop("stream", None): | |
| log.warning( | |
| "Ignoring `stream: true` set in `parameters` for model %r (engine %r). " | |
| "Setting `stream` directly on a model causes the provider to return an " | |
| "AsyncStream object on every call, which breaks the non-streaming path. " | |
| "Request streaming via the API or set the `streaming` flag on the rails " | |
| "config instead.", | |
| getattr(model_config, "model", None), | |
| getattr(model_config, "engine", None), | |
| ) | |
| stream_val = kwargs.pop("stream", None) | |
| if stream_val: | |
| log.warning( | |
| "Ignoring `stream: %r` set in `parameters` for model %r (engine %r). " | |
| "Setting `stream` directly on a model causes the provider to return an " | |
| "AsyncStream object on every call, which breaks the non-streaming path. " | |
| "Request streaming via the API or set the `streaming` flag on the rails " | |
| "config instead.", | |
| stream_val, | |
| getattr(model_config, "model", None), | |
| getattr(model_config, "engine", None), | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/rails/llm/llmrails.py
Line: 418-427
Comment:
The warning message hardcodes the literal string `` `stream: true` `` but the condition fires for any truthy value (e.g. `1`, a non-empty string). Embedding the actual value makes the message more actionable when a user sets a non-boolean truthy value.
```suggestion
stream_val = kwargs.pop("stream", None)
if stream_val:
log.warning(
"Ignoring `stream: %r` set in `parameters` for model %r (engine %r). "
"Setting `stream` directly on a model causes the provider to return an "
"AsyncStream object on every call, which breaks the non-streaming path. "
"Request streaming via the API or set the `streaming` flag on the rails "
"config instead.",
stream_val,
getattr(model_config, "model", None),
getattr(model_config, "engine", None),
)
```
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Closes #1325.
When users set
parameters.stream: truedirectly on a model in config.yml, providers like the OpenAI client forward the flag verbatim to the HTTP layer, which returns anAsyncStreamobject on every call. The non-streaming completion path then tries to call.model_dump()on that stream and the server crashes with'AsyncStream' object has no attribute 'model_dump'.The fix strips the
streamparameter insideLLMRails._prepare_model_kwargsbefore init and logs a warning that explains the problem and points the user at the correct opt-in (the rails-levelstreamingflag, or requesting streaming per-call via the API). The strip is narrow:stream_options, which is a legitimate provider knob used to opt into usage accounting on OpenAI streaming responses, is left alone, and an explicitstream: falseis silently dropped without a warning because it is already a no-op. The helper now also defensively copiesmodel_config.parametersso the strip cannot mutate the user'sRailsConfigin place, which closes a small preexisting hazard for theapi_keyinjection path.Added three regression tests in
tests/test_llmrails.py: the original misconfiguration from the issue is now handled cleanly with a warning,stream_optionsis preserved, andstream: falseis dropped silently. Fulltests/test_llmrails.py,tests/_compat, andtests/llmsuites continue to pass.Summary by CodeRabbit
streamdirectly within model parameters; streaming must be enabled via the rails streaming flag or API request instead. A warning will now appear if attempted.