Repository navigation
fix: do not forward the unknown-model sentinel as a per-request model override - #2141
Conversation
WalkthroughThe API server centralizes the unknown-model sentinel. The React agent excludes unspecified models from LangChain overrides. Regression tests cover unspecified and explicit model values. ChangesModel override handling
Estimated code review effort: 2 (Simple) | ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the sentinel comment and definition with the upstream contract.
ChatRequest.modeldefaults toNone; only the plain-string converter injects"unknown-model". This comment is misleading, and the sentinel literal is duplicated across packages. Prefer a shared constant or update the comment to document the converter behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py` around lines 46 - 48, The _UNKNOWN_MODEL_SENTINEL definition and comment should reflect the upstream contract: ChatRequest.model defaults to None, while only the plain-string converter supplies "unknown-model". Reuse the existing shared sentinel constant if available; otherwise update the comment to describe the converter behavior without implying ChatRequest.model uses it by default.packages/nvidia_nat_langchain/tests/agent/test_react.py (1)
1809-1811: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the unsupported-backend sentinel path.
Add an assertion using
supports_override=False; this ensures the sentinel is ignored before the backend capability check and can never raise an unsupported-override error.Proposed test addition
assert "configurable" not in _build_lc_config(5, sentinel_model) + assert "configurable" not in _build_lc_config(5, sentinel_model, supports_override=False) assert "configurable" not in _build_lc_config(5, None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_langchain/tests/agent/test_react.py` around lines 1809 - 1811, Extend the sentinel-model assertions in the test covering _build_lc_config to pass supports_override=False, verifying the sentinel is ignored before backend capability validation and does not raise an unsupported-override error. Keep the existing assertions for the default behavior and None model unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nvidia_nat_langchain/tests/agent/test_react.py`:
- Around line 1802-1803: Update the regression test docstring in the test
covering the `unknown-model` sentinel so its opening descriptive sentence is
concise, ends with a period on the same line, and uses backticks around code
entities such as `unknown-model` and `ChatRequest`.
---
Nitpick comments:
In
`@packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py`:
- Around line 46-48: The _UNKNOWN_MODEL_SENTINEL definition and comment should
reflect the upstream contract: ChatRequest.model defaults to None, while only
the plain-string converter supplies "unknown-model". Reuse the existing shared
sentinel constant if available; otherwise update the comment to describe the
converter behavior without implying ChatRequest.model uses it by default.
In `@packages/nvidia_nat_langchain/tests/agent/test_react.py`:
- Around line 1809-1811: Extend the sentinel-model assertions in the test
covering _build_lc_config to pass supports_override=False, verifying the
sentinel is ignored before backend capability validation and does not raise an
unsupported-override error. Keep the existing assertions for the default
behavior and None model unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 696a05f2-208f-4ea5-a669-d496745ff658
📒 Files selected for processing (2)
packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.pypackages/nvidia_nat_langchain/tests/agent/test_react.py
…request override Signed-off-by: knQzx <75641500+knQzx@users.noreply.github.com>
f74768e to
b23575c
Compare
|
I verified this independently against The chain, link by link. def _string_to_nat_chat_request(data: str) -> ChatRequest:
return ChatRequest.from_string(data, model="unknown-model") # api_server.py:954-955
So on Fixing inside The one thing I'd change: the sentinel now has two independent definitions. This repo already has the pattern for the fix: Minor, and I think intentional: with the sentinel short-circuiting the whole block, it no longer reaches the AI-assisted review. The code paths above were read on |
|
/ok to test 7f41f9c |
…se it the sentinel value lived as a bare literal in api_server and in both langchain agents, so a change in one place would silently stop matching in the others Signed-off-by: knQzx <75641500+knQzx@users.noreply.github.com>
|
/ok to test a4d96c7 |
|
/merge |
… override (NVIDIA#2141) Closes NVIDIA#2110 when a model can't be resolved, the ReAct agent builder receives the sentinel `model="unknown-model"` and forwards it straight through as a per-request `configurable.model_name` override, so every downstream LLM/NIM call is told to use a model literally named `unknown-model` and the provider returns 404. this is a regression from NVIDIA#2036, which introduced the sentinel but didn't guard against forwarding it the fix treats the `unknown-model` sentinel the same as an unset model - the per-request model override is only applied when a real model name is present the guard sits inside `_build_lc_config` rather than at its call sites, so it also covers `register_per_user_agent.py`, which imports the same helper and passes `message.model` the same way - `per_user_react_agent` has the identical defect on develop the sentinel value itself now lives in `nat/data_models/api_server.py` as `UNKNOWN_MODEL_SENTINEL` and every reference points at it (the five defaults/fallbacks there, the `_string_to_nat_chat_request` converter, this guard, and the literal in `tool_calling_agent/register.py`), so the value can't drift apart between packages one behaviour note: with the sentinel short-circuiting the block it no longer reaches the `supports_override` check, so a backend without per-request override support accepts plain string input instead of raising. that is intended - the user never asked for a model - and the test pins it reproduce: build a ReAct agent whose model resolves to the `unknown-model` sentinel (an unresolved model reference), then run it. on develop the request carries `configurable: {model_name: "unknown-model"}` and the call 404s; with this change no model override is attached and the configured default is used. the added `test_build_lc_config_ignores_unknown_model_sentinel` asserts the sentinel is not forwarded ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing/index.md) - We require that all contributors "sign-off" on their commits - this commit is signed off - New tests cover these changes - The documentation is up to date with these changes ## Summary by CodeRabbit - **Bug Fixes** - Prevented unspecified or unknown model values from being incorrectly applied as per-request model overrides. - Preserved validation and support checks when a valid model override is provided. - Standardized handling of unknown model values across chat responses and streaming responses. - **Tests** - Added regression coverage for missing, unknown, and valid model overrides. Authors: - Novikov Kirill (https://github.com/knQzx) - Will Killian (https://github.com/willkill07) Approvers: - Will Killian (https://github.com/willkill07) URL: NVIDIA#2141
Description
Closes #2110
when a model can't be resolved, the ReAct agent builder receives the sentinel
model="unknown-model"and forwards it straight through as a per-requestconfigurable.model_nameoverride, so every downstream LLM/NIM call is told to use a model literally namedunknown-modeland the provider returns 404. this is a regression from #2036, which introduced the sentinel but didn't guard against forwarding itthe fix treats the
unknown-modelsentinel the same as an unset model - the per-request model override is only applied when a real model name is presentthe guard sits inside
_build_lc_configrather than at its call sites, so it also coversregister_per_user_agent.py, which imports the same helper and passesmessage.modelthe same way -per_user_react_agenthas the identical defect on developthe sentinel value itself now lives in
nat/data_models/api_server.pyasUNKNOWN_MODEL_SENTINELand every reference points at it (the five defaults/fallbacks there, the_string_to_nat_chat_requestconverter, this guard, and the literal intool_calling_agent/register.py), so the value can't drift apart between packagesone behaviour note: with the sentinel short-circuiting the block it no longer reaches the
supports_overridecheck, so a backend without per-request override support accepts plain string input instead of raising. that is intended - the user never asked for a model - and the test pins itreproduce: build a ReAct agent whose model resolves to the
unknown-modelsentinel (an unresolved model reference), then run it. on develop the request carriesconfigurable: {model_name: "unknown-model"}and the call 404s; with this change no model override is attached and the configured default is used. the addedtest_build_lc_config_ignores_unknown_model_sentinelasserts the sentinel is not forwardedBy Submitting this PR I confirm:
Summary by CodeRabbit