Feature: GPT-5 Support #664
Conversation
Signed-off-by: Will Killian <[email protected]>
Signed-off-by: Will Killian <[email protected]>
willkill07
added
improvement
Signed-off-by: Will Killian <[email protected]>
Signed-off-by: Will Killian <[email protected]>
Signed-off-by: Will Killian <[email protected]>
willkill07
added
feature request
Signed-off-by: Will Killian <[email protected]>
willkill07
force-pushed
the
wkk_gpt5-support
branch
from
5bc70f0 to
f5d2b15
Compare
Signed-off-by: Will Killian <[email protected]>
willkill07
force-pushed
the
wkk_gpt5-support
branch
from
f5d2b15 to
9fbce97
Compare
There was a problem hiding this comment.
ReAct agents fail with GPT-5 models because llm.bind(stop=["Observation:"]) in https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/src/nat/agent/react_agent/agent.py#L100 adds runtime stop sequences that GPT-5’s API rejects. We need to detect GPT-5 models and gate this behavior accordingly.
Signed-off-by: Will Killian <[email protected]>
|
Warning Rate limit exceeded@willkill07 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughAdds model-gated configuration mixins (ModelGatedFieldMixin, TemperatureMixin, TopPMixin), integrates them into LLM provider configs, adjusts ReAct agent LLM binding to avoid stop sequences for GPT‑5‑like models, adds docs and tests for gating/mixins, and minor CI vocabulary regex tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Config
participant C as ProviderConfig (with Mixins)
participant M as ModelGatedFieldMixin
Note over C,M: Pydantic validation (after)
U->>C: Instantiate with model fields + temperature/top_p
C->>M: model_validate(after)
M->>M: Detect model via keys (model_name/model/azure_deployment or custom)
alt Field supported
alt Value provided
M-->>C: Accept provided value
else No value
M-->>C: Set default_if_supported (e.g., temp=0.0, top_p=1.0)
end
else Field unsupported
alt Value provided
M-->>C: Raise ValidationError
else No value
M-->>C: Keep None
end
end
C-->>U: Validated config
sequenceDiagram
autonumber
participant App as App
participant A as ReActAgentGraph
participant L as LLM
participant P as Prompt
App->>A: __init__(llm, prompt)
A->>A: _maybe_bind_llm_and_yield()
A->>L: inspect model/model_name
alt Not GPT‑5-like
A->>L: bind(stop=["Observation:"])
A->>A: agent = prompt | bound_llm
else GPT‑5-like
A->>A: agent = prompt | llm (no stop)
end
A-->>App: Initialized agent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment — Out-of-scope changes
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Will Killian <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/source/extend/adding-an-llm-provider.md (1)
33-36: Typo: “retreiver” → “retriever”Minor spelling error in a user-facing doc.
-In NeMo Agent toolkit, there are three provider types: `llm`, `embedder`, and `retreiver`. +In NeMo Agent toolkit, there are three provider types: `llm`, `embedder`, and `retriever`.src/nat/data_models/model_gated_field_mixin.py (1)
80-126: Critical: Refactor ModelGatedFieldMixin to avoid cross-mixin collisions and remove unnecessary getattr callsThe current implementation of
ModelGatedFieldMixininsrc/nat/data_models/model_gated_field_mixin.pystill:
- Uses
getattr(cls, "...", None)to read constant class attributes (unsupported_models,supported_models,model_keys,default_if_supported)—triggers Ruff B009 and obscures intent.- Defines
model_validateso that at runtimeself.__class__(“provider” class) is used to look upfield_nameand helpers, causing:
- In a provider composing TemperatureMixin + TopPMixin, only one mixin’s validator runs (the one earliest in the MRO), leaving the other field ungated.
- Mis-reported errors and wrong defaults when both mixins are present.
Mandatory fixes (in
src/nat/data_models/model_gated_field_mixin.py, lines 80–126):
- Replace all
getattr(cls, "...", None)with directcls.<attribute>.- In
detect_support, skipNoneor empty model identifiers before checking.- In the
@model_validatormethod, removeklass = self.__class__and use the mixin class captured in the__init_subclass__closure (cls) so each mixin’s validator always refers to its ownfield_name,model_keys, etc.Apply this diff:
@@ class ModelGatedFieldMixin.__init_subclass__: - @classmethod - def check_model(cls, model_name: str) -> bool: + @classmethod + def check_model(cls, model_name: str) -> bool: """ @@ - unsupported = getattr(cls, "unsupported_models", None) - supported = getattr(cls, "supported_models", None) + unsupported = cls.unsupported_models + supported = cls.supported_models if unsupported is not None: return not any(p.search(model_name) for p in unsupported) if supported is not None: return any(p.search(model_name) for p in supported) return False @@ class ModelGatedFieldMixin.__init_subclass__: - @classmethod - def detect_support(cls, instance: BaseModel) -> str | None: - for key in getattr(cls, "model_keys"): - if hasattr(instance, key): - model_name_value = getattr(instance, key) - is_supported = getattr(cls, "_model_gated_field_check_model")(str(model_name_value)) - return key if not is_supported else None - return None + @classmethod + def detect_support(cls, instance: BaseModel) -> str | None: + for key in cls.model_keys: + if not hasattr(instance, key): + continue + value = getattr(instance, key) + if value is None: + continue + s = str(value).strip() + if not s: + continue + if not cls._model_gated_field_check_model(s): + return key + return None @@ class ModelGatedFieldMixin.__init_subclass__: - @model_validator(mode="after") - def model_validate(self): - klass = self.__class__ - - field_name_local = getattr(klass, "field_name") - current_value = getattr(self, field_name_local, None) - - found_key = klass._model_gated_field_detect_support(self) - if found_key is not None: - if current_value is not None: - raise ValueError( - f"{field_name_local} is not supported for {found_key}: {getattr(self, found_key)}") - elif current_value is None: - setattr(self, field_name_local, getattr(klass, "default_if_supported", None)) - return self + @model_validator(mode="after") + def model_validate(self): + # Use the mixin class (captured as cls in this closure), not the provider, + # so each mixin’s own field_name, default_if_supported, etc., are used. + field_name_local = cls.field_name # type: ignore[attr-defined] + current_value = getattr(self, field_name_local, None) + + found_key = cls._model_gated_field_detect_support(self) + if found_key is not None: + if current_value is not None: + raise ValueError( + f"{field_name_local} is not supported for {found_key}: {getattr(self, found_key)}") + elif current_value is None: + setattr(self, field_name_local, cls.default_if_supported) + return self• Ensure AWSBedrockModelConfig (in
src/nat/llm/aws_bedrock_llm.py) includesTopPMixinif top-P gating is required.
• After applying, re-run the existing smoke tests and verify bothtemperatureandtop_pare correctly gated for multi-mixin providers.
🧹 Nitpick comments (13)
ci/vale/styles/config/vocabularies/nat/accept.txt (1)
47-47: Broaden “denylist” acceptance pattern to include hyphenated and plural forms
Vale currently only accepts[Dd]enylist, which won’t catch variants like “deny-list” or “denylists” (just as we already accept “API(s?)” and “Callable(s?)”). A quick repo scan showed hyphenated allow-list variants in comments (e.g.extension-pkg-allow-list), so extending the pattern prevents needless Vale warnings and keeps consistency.• File: ci/vale/styles/config/vocabularies/nat/accept.txt (around line 47)
Apply:-[Dd]enylist +[Dd]eny-?list(s?)src/nat/agent/react_agent/agent.py (1)
101-106: Correctly omits stop for GPT-5; add debug + make stop tokens slightly more robust.Logic is sound: bind
stoponly when the model supports it. Consider a tiny enhancement for observability and robustness:
- Log whether a stop sequence was bound (helps diagnose GPT-5 vs non–GPT-5 behavior in prod).
- Include both "Observation:" and "\nObservation:" as stop sequences to cover formatting variants.
Proposed inline tweak:
- if ReActAgentGraph._llm_needs_stop_sequence(llm): - bound_llm = llm.bind(stop=["Observation:"]) - self.agent = prompt | bound_llm - else: - self.agent = prompt | llm + needs_stop = ReActAgentGraph._llm_needs_stop_sequence(llm) + logger.debug("%s stop binding applied: %s", AGENT_LOG_PREFIX, needs_stop) + self.agent = prompt | (llm.bind(stop=["Observation:", "\nObservation:"]) if needs_stop else llm)docs/source/workflows/llms/index.md (1)
126-129: Azure note should also mention top_p; mirror AWS section with a temperature note
- If AzureOpenAIModelConfig includes both TemperatureMixin and TopPMixin (per PR summary), this admonition should include both fields to avoid confusion.
- Also consider adding a similar note under the AWS Bedrock section indicating that temperature is model-gated there (since AWS gets TemperatureMixin only).
Apply this diff to update the Azure note and add an AWS note:
@@ ### AWS Bedrock @@ * `max_retries` - The maximum number of retries for the request + +:::{note} +`temperature` is a model-gated field and may not be supported by all models. If unsupported and explicitly set, validation will fail. See [Model-Gated Fields](../../extend/model-gated-fields.md) for details. +::: @@ ### Azure OpenAI @@ -:::{note} -`temperature` is model-gated and may not be supported by all models. See [Model-Gated Fields](../../extend/model-gated-fields.md) for details. -::: +:::{note} +`temperature` and `top_p` are model-gated fields and may not be supported by all models. If unsupported and explicitly set, validation will fail. See [Model-Gated Fields](../../extend/model-gated-fields.md) for details. +:::Please confirm:
- AzureOpenAIModelConfig actually composes both TemperatureMixin and TopPMixin.
- AWSBedrockModelConfig composes TemperatureMixin only.
If not, we should align the docs with the actual compositions.docs/source/extend/adding-an-llm-provider.md (1)
83-93: Model-Gated Fields: tighten phrasing on rejectionsThe guidance is solid. Minor suggestion: explicitly state that allowlists (supported_models) treat unknown models as unsupported, while denylists (unsupported_models) treat unknown models as supported. This mirrors the actual behavior and avoids ambiguity.
- Some configuration parameters are only valid for certain models. The toolkit provides built-in mixins that automatically validate and default these parameters based on the selected model. + Some configuration parameters are only valid for certain models. The toolkit provides built-in mixins that automatically validate and default these parameters based on the selected model. Note: when using `supported_models` (allowlist), unknown models are treated as unsupported; when using `unsupported_models` (denylist), unknown models are treated as supported.docs/source/extend/model-gated-fields.md (3)
57-67: Regex example: prefer hyphenated GPT-5 and ignore-caseDeployment/model identifiers typically include the hyphen. Recommend anchoring the example to
^gpt-5$and retainingre.IGNORECASE(already present).- unsupported_models=(re.compile(r"^gpt5$", re.IGNORECASE),), + unsupported_models=(re.compile(r"^gpt-5$", re.IGNORECASE),),
71-80: Bullet formatting/wording nitMinor polish for consistency and readability.
-- {py:class}`~nat.data_models.temperature_mixin.TemperatureMixin` - - Field: `temperature` in [0, 1] - - Default when supported: `0.0` - - Not supported on GPT-5 models +- {py:class}`~nat.data_models.temperature_mixin.TemperatureMixin` + - Field: `temperature` in [0, 1] + - Default when supported: `0.0` + - Not supported on GPT-5 models. @@ -- {py:class}`~nat.data_models.top_p_mixin.TopPMixin` - - Field: `top_p` in [0, 1] - - Default when supported: `1.0` - - Not supported on GPT-5 models +- {py:class}`~nat.data_models.top_p_mixin.TopPMixin` + - Field: `top_p` in [0, 1] + - Default when supported: `1.0` + - Not supported on GPT-5 models.
29-34: Edge case note: empty/None model identifiersConsider adding a brief note that if a detection key exists but is empty/None, the mixin treats it as “no detectable model” and applies the default (assuming you adopt the code change below to skip empty values). This clarifies behavior for partially populated configs.
tests/nat/data_models/test_model_gated_field_mixin.py (2)
108-122: Edge case: treat None/empty model key as “not present” to avoid surprising gatingCurrent detect_support() (in ModelGatedFieldMixin) checks hasattr(instance, key) and then stringifies the value. If the attribute exists but is None or "", it will be treated as a present key with a value that almost certainly won’t match any pattern, which flips to “supported” in unsupported_models mode or “unsupported” in supported_models mode unexpectedly. This can lead to defaults being applied or withheld unintentionally when users explicitly pass model_name=None.
Recommend skipping keys whose value is None or "" so behavior matches the “no model key → default_if_supported” rule.
Potential fix in src/nat/data_models/model_gated_field_mixin.py (outside this file), within detect_support():
- def detect_support(cls, instance: BaseModel) -> str | None: - for key in getattr(cls, "model_keys"): - if hasattr(instance, key): - model_name_value = getattr(instance, key) - is_supported = getattr(cls, "_model_gated_field_check_model")(str(model_name_value)) - return key if not is_supported else None + def detect_support(cls, instance: BaseModel) -> str | None: + for key in getattr(cls, "model_keys"): + if hasattr(instance, key): + model_name_value = getattr(instance, key) + # Treat None/empty as absent to follow “no model key present” semantics + if model_name_value in (None, ""): + continue + is_supported = getattr(cls, "_model_gated_field_check_model")(str(model_name_value)) + return key if not is_supported else None return NoneIf you’d like, I can also add a unit test for the None/empty case once the behavior is agreed.
16-24: Add targeted tests for key precedence and empty model_keys validationTo harden the contract, consider:
- A test proving precedence when multiple keys are present (e.g., model_name vs model vs azure_deployment).
- A test that model_keys=() raises ValueError (there is a guard, but no test).
Apply this patch to extend coverage:
+def test_model_gated_field_key_precedence(): + + class BothKeys( + BaseModel, + ModelGatedFieldMixin[int], + field_name="dummy", + default_if_supported=11, + # Only 'beta' is supported + supported_models=(re.compile(r"^beta$"), ), + ): + dummy: int | None = Field(default=None) + # Both present; by default, model_name is checked before model + model_name: str = "alpha" + model: str = "beta" + + # Precedence should pick model_name first → unsupported → no default + m = BothKeys() + assert m.dummy is None + + +def test_model_gated_field_rejects_empty_model_keys_tuple(): + with pytest.raises(ValueError, match=r"model_keys must be provided and non-empty"): + class BadModelKeys( + BaseModel, + ModelGatedFieldMixin[int], + field_name="dummy", + default_if_supported=1, + supported_models=(re.compile(r".+"), ), + model_keys=(), + ): + dummy: int | None = Field(default=None)src/nat/llm/aws_bedrock_llm.py (1)
55-58: Param rename to_builderis a clean way to denote unused parameterNo functional impact; avoids linter noise. Also, minor copy edit suggestion in the description string.
Apply this tiny edit for grammar:
- yield LLMProviderInfo(config=llm_config, description="A AWS Bedrock model for use with an LLM client.") + yield LLMProviderInfo(config=llm_config, description="An AWS Bedrock model for use with an LLM client.")Confirm that AWS Bedrock intentionally omits TopPMixin. If bedrock models accept a top_p/topP parameter, consider adding TopPMixin here (and mapping to provider-specific naming if needed). I can prep a follow-up patch if desired.
src/nat/data_models/temperature_mixin.py (1)
26-36: Multiple BaseModel mixins: acceptable, but consider simplifying laterTemperatureMixin also subclasses BaseModel. Pydantic v2 tolerates multiple BaseModel bases, but a leaner approach is to have mixins avoid inheriting BaseModel and rely on the concrete config class to provide it. That can reduce MRO complexity and accidental model_config resolution surprises.
If you decide to refactor later, consider:
- Make TemperatureMixin a plain mixin (object), and keep the validator registration in ModelGatedFieldMixin working on BaseModel subclasses via typing Protocols or relaxed annotations.
- Centralize model patterns in a small constants module to avoid repetition across mixins.
src/nat/llm/openai_llm.py (1)
29-29: Config now inherits gating mixins: LGTM, but unify max_retries ownershipAdopting TemperatureMixin/TopPMixin here is correct. One consistency nit: this class still declares max_retries (Line 40) while Azure and NIM rely on RetryMixin. Keeping the field in only one place avoids divergence.
Proposed cleanup (remove the explicit field from this class and rely on RetryMixin’s default):
class OpenAIModelConfig(LLMBaseConfig, RetryMixin, TemperatureMixin, TopPMixin, name="openai"): @@ - max_retries: int = Field(default=10, description="The max number of retries for the request.")If RetryMixin doesn’t declare max_retries yet, consider moving the shared definition there for all providers.
src/nat/llm/azure_openai_llm.py (1)
29-49: Consider adding an Azure-specific test to exercise azure_deployment detectionUnit tests cover the mixins, but an end-to-end Azure model config case (e.g., azure_deployment="gpt-5o") would guard against regressions in alias handling.
I can add a small test under tests/nat/llm/ to instantiate AzureOpenAIModelConfig with azure_deployment="gpt-5o" and assert temperature/top_p remain None while validation passes. Want me to draft it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
ci/vale/styles/config/vocabularies/nat/accept.txt(2 hunks)docs/source/extend/adding-an-llm-provider.md(1 hunks)docs/source/extend/model-gated-fields.md(1 hunks)docs/source/index.md(1 hunks)docs/source/workflows/llms/index.md(3 hunks)src/nat/agent/react_agent/agent.py(2 hunks)src/nat/data_models/model_gated_field_mixin.py(1 hunks)src/nat/data_models/temperature_mixin.py(1 hunks)src/nat/data_models/top_p_mixin.py(1 hunks)src/nat/llm/aws_bedrock_llm.py(2 hunks)src/nat/llm/azure_openai_llm.py(1 hunks)src/nat/llm/nim_llm.py(2 hunks)src/nat/llm/openai_llm.py(2 hunks)tests/nat/data_models/test_model_gated_field_mixin.py(1 hunks)tests/nat/data_models/test_temperature_mixin.py(1 hunks)tests/nat/data_models/test_top_p_mixin.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
tests/nat/data_models/test_top_p_mixin.py (1)
src/nat/data_models/top_p_mixin.py (1)
TopPMixin(26-36)
tests/nat/data_models/test_model_gated_field_mixin.py (1)
src/nat/data_models/model_gated_field_mixin.py (1)
ModelGatedFieldMixin(27-125)
src/nat/llm/aws_bedrock_llm.py (1)
src/nat/data_models/temperature_mixin.py (1)
TemperatureMixin(26-36)
tests/nat/data_models/test_temperature_mixin.py (1)
src/nat/data_models/temperature_mixin.py (1)
TemperatureMixin(26-36)
src/nat/data_models/top_p_mixin.py (1)
src/nat/data_models/model_gated_field_mixin.py (1)
ModelGatedFieldMixin(27-125)
src/nat/llm/nim_llm.py (2)
src/nat/data_models/temperature_mixin.py (1)
TemperatureMixin(26-36)src/nat/data_models/top_p_mixin.py (1)
TopPMixin(26-36)
src/nat/llm/openai_llm.py (2)
src/nat/data_models/temperature_mixin.py (1)
TemperatureMixin(26-36)src/nat/data_models/top_p_mixin.py (1)
TopPMixin(26-36)
src/nat/data_models/temperature_mixin.py (1)
src/nat/data_models/model_gated_field_mixin.py (1)
ModelGatedFieldMixin(27-125)
src/nat/llm/azure_openai_llm.py (2)
src/nat/data_models/temperature_mixin.py (1)
TemperatureMixin(26-36)src/nat/data_models/top_p_mixin.py (1)
TopPMixin(26-36)
🪛 LanguageTool
docs/source/extend/adding-an-llm-provider.md
[grammar] ~87-~87: There might be a mistake here.
Context: ..., with a default of 0.0 when supported - TopPMixin: adds a top_p field in [0, 1], with a...
(QB_NEW_EN)
docs/source/extend/model-gated-fields.md
[grammar] ~71-~71: There might be a mistake here.
Context: ... ## Built-in Gated Mixins - {py:class}~nat.data_models.temperature_mixin.TemperatureMixin - Field: temperature in [0, 1] - Defau...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...ixin - Field:temperaturein [0, 1] - Default when supported:0.0` - Not su...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...in [0, 1] - Default when supported:0.0` - Not supported on GPT-5 models - {py:cla...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ... supported on GPT-5 models - {py:class}~nat.data_models.top_p_mixin.TopPMixin - Field: top_p in [0, 1] - Default whe...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ....TopPMixin - Field:top_pin [0, 1] - Default when supported:1.0` - Not su...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...in [0, 1] - Default when supported:1.0` - Not supported on GPT-5 models ### Examp...
(QB_NEW_EN)
🪛 Ruff (0.12.2)
src/nat/data_models/model_gated_field_mixin.py
100-100: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
103-103: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
113-113: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (39)
ci/vale/styles/config/vocabularies/nat/accept.txt (1)
81-81: LGTM: plural “Mixins” accepted.The updated pattern
[Mm]ixin(s?)aligns with existing pluralization patterns in this file and will quiet Vale across docs mentioning multiple Mixins.src/nat/agent/react_agent/agent.py (2)
19-19: Import is appropriate.The added
reimport is needed for the new model-detection helper.
109-116: Update GPT-5 regex to cover “gpt-5o” variantsThe proposed pattern
r"\bgpt-?5(?:\b|[-._])"still fails to detect variants likegpt-5o(e.g.gpt-5o-2025-06-18,azure/prod-gpt-5o), causingneeds_stop=Truewhen it should beFalse.• Location: src/nat/agent/react_agent/agent.py, lines 109–116
• Tests show:
openai_gpt5o→ needs_stop should be False but is Trueazure_prefixed→ needs_stop should be False but is TrueSuggested diff to catch any alphanumeric or separator suffix after “gpt-5”:
@staticmethod def _llm_needs_stop_sequence(llm: BaseChatModel) -> bool: - pattern = re.compile(r"\bgpt-?5(?:\b|[-._])", re.IGNORECASE) + # match any GPT-5 variant, including letter- or digit-suffixes (e.g. gpt-5o, gpt-5-32k) + pattern = re.compile(r"\bgpt-?5[\w\.-]*", re.IGNORECASE) model_id_attrs = ("model", "model_name", "azure_deployment") identifiers = (getattr(llm, attr, "") or "" for attr in model_id_attrs) haystack = " ".join(s for s in identifiers if isinstance(s, str)) return not bool(pattern.search(haystack))This revised regex (
\bgpt-?5[\w\.-]*) ensures all GPT-5 forks—includinggpt-5o,gpt5,gpt-5.1,prod-gpt-5-32k—are properly detected and won’t have a stop sequence bound.Likely an incorrect or invalid review comment.
docs/source/index.md (1)
116-116: Nav entry for Model-Gated Fields — looks goodThe new toctree item is placed logically alongside “Adding an LLM Provider” under Extend. No issues.
docs/source/workflows/llms/index.md (2)
69-72: Good callout: model-gated fields for NIMClear, concise note linking to the detailed docs. LGTM.
91-94: Good callout: model-gated fields for OpenAIConsistent wording with the NIM section; link path is correct.
docs/source/extend/adding-an-llm-provider.md (4)
59-66: Mixins section is a helpful additionClear explanation of why mixins exist and how they augment provider configs. Good separation of concerns.
67-81: RetryMixin example: consistent and minimalExample correctly demonstrates composition and extra="allow". No issues.
94-115: TemperatureMixin example: OKAccurately shows composition and field semantics. No action needed.
116-134: TopPMixin example: OKConsistent with TemperatureMixin example. No action needed.
docs/source/extend/model-gated-fields.md (1)
18-34: Great overview and behavior tableThe detection keys, selector modes, and behavior matrix are crisp and actionable. Nice work.
tests/nat/data_models/test_model_gated_field_mixin.py (7)
26-43: LGTM: selector mutual exclusivity is enforced at class definition timeBoth “both-provided” and “neither-provided” paths are correctly guarded with clear error messages. Good use of class-definition-time validation for fast feedback.
44-58: LGTM: selector presence validationThe error path for missing selectors is covered and messages are precise. This prevents silent misconfiguration of the mixin.
60-74: LGTM: default application when supported and value is NoneBehavior matches the documented contract: supported + omitted → default applied.
76-90: LGTM: validation error when unsupported and value is setThe raised ValidationError and message format match the mixin’s validator. Good.
92-106: LGTM: unsupported + omitted returns NoneThis correctly preserves “unsupported + omitted → leave None”.
124-141: LGTM: custom model_keys (unsupported path) behaves as intendedGating on a non-default key and leaving the field as None when banned is correct and readable.
142-158: LGTM: custom model_keys (supported path) applies defaultCompletes the supported-path coverage for custom keys.
tests/nat/data_models/test_top_p_mixin.py (5)
22-29: LGTM: default top_p when supportedDefaulting to 1.0 for supported models is asserted correctly.
31-38: LGTM: value retention for supported modelsThe provided value is preserved as expected.
49-56: LGTM: unsupported + omitted yields NoneThis aligns with gating semantics.
58-68: LGTM: range validationUpper and lower bounds are covered.
70-74: LGTM: default when no model keys are presentDirect instantiation confirms fallback to default_if_supported.
tests/nat/data_models/test_temperature_mixin.py (5)
22-29: LGTM: default temperature when supportedDefaulting to 0.0 for supported models is asserted correctly.
31-38: LGTM: value retention for supported modelsThe provided value is preserved as expected.
49-56: LGTM: unsupported + omitted yields NoneBehavior is correct and mirrors top_p.
58-68: LGTM: range validationBounds are exercised.
70-74: LGTM: default when no model keys presentDirect instantiation confirms fallback to default_if_supported.
src/nat/llm/aws_bedrock_llm.py (1)
25-29: LGTM: TemperatureMixin integrationAdopting TemperatureMixin for AWSBedrockModelConfig brings parity with other providers and centralizes validation/defaults.
src/nat/data_models/top_p_mixin.py (1)
26-36: LGTM: mixin definition and field constraintsThe field bounds and docstring are appropriate; integration with ModelGatedFieldMixin is clean.
src/nat/data_models/temperature_mixin.py (1)
1-15: License header: LGTMSPDX header and licensing block are correct and consistent with the repo.
src/nat/llm/openai_llm.py (3)
25-26: Importing TemperatureMixin and TopPMixin: LGTMGood modularization; keeps provider configs thin and model-aware.
44-44: Rename builder → _builder: LGTMEliminates the unused-arg warning without behavioral changes.
29-47: Verify gating in external LLM‐wrapper integrationI didn’t find any code in src/nat/llm/openai_llm.py (or its builder support) that actually assembles the HTTP payload—request construction appears to live in the LangChain (or other) wrapper you plug in at runtime. Because of that:
- There’s no direct
payload["temperature"] = …or dict‐comprehension filtering in this repo.- Neither OpenAIModelConfig nor AzureOpenAIModelConfig contains logic to omit nulls—they simply define the fields.
- The gating must therefore happen in your LLM‐framework integration (e.g. LangChain’s OpenAI/ChatOpenAI class).
Please manually verify in your wrapper code (the part that maps OpenAIModelConfig → LangChain → HTTP request) that
temperatureandtop_pare only added when not None—otherwise a JSON null may be sent downstream.src/nat/llm/nim_llm.py (3)
26-27: Importing TemperatureMixin and TopPMixin: LGTMConsistent with OpenAI/Azure providers.
30-30: Config now inherits gating mixins: LGTMDefaulting and gating will apply uniformly for NIM configs too.
44-44: Rename builder → _builder: LGTMNo functional change, resolves unused parameter.
src/nat/llm/azure_openai_llm.py (2)
25-26: Importing TemperatureMixin and TopPMixin: LGTMMatches the shared mixin strategy used across providers.
29-29: Config now inherits gating mixins: LGTMGating keys include azure_deployment by default, so detection works for Azure-specific model IDs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/nat/agent/react_agent/agent.py(2 hunks)
🔇 Additional comments (2)
src/nat/agent/react_agent/agent.py (2)
19-19: Imports look good and are necessary for the new typing and regex logic.
No issues spotted.Also applies to: 27-27, 34-34
103-103: Nice refactor: delegate LLM binding to a helper.
This keeps init clean and expresses intent clearly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/nat/agent/react_agent/agent.py (1)
116-122: Fix None-handling in GPT‑5 detection and includeazure_deployment(repeat).
- If
self.llm.modelorself.llm.model_nameexists but isNone,re.Pattern.search(None)raises a TypeError at runtime.- Azure deployments identify models via
azure_deployment; align with the PR’s model-key defaults to avoid missing GPT‑5 on Azure.- Keep using
search()(good) to match substrings likeopenai/gpt-5o.Apply this diff:
- # models that don't need (or don't support)a stop sequence - smart_models = re.compile(r"gpt-?5", re.IGNORECASE) - if any(smart_models.search(getattr(self.llm, model, "")) for model in ["model", "model_name"]): + # models that don't need (or don't support) a stop sequence + gpt5_re = re.compile(r"gpt-?5", re.IGNORECASE) + model_attrs = ("model", "model_name", "azure_deployment") + identifiers: list[str] = [] + for attr in model_attrs: + val = getattr(self.llm, attr, None) + if val: + identifiers.append(str(val)) + if any(gpt5_re.search(identifier) for identifier in identifiers): # no need to bind any additional parameters to the LLM return self.llmIf you adopt the module-level constant from my earlier comment, replace
gpt5_rewithGPT5_REGEXand drop the in-method compile.Run this to confirm attribute coverage across providers and catch other model-id properties you may want to include:
#!/bin/bash set -euo pipefail echo "Scanning provider LLMs for model identifier attributes…" rg -nP -C2 '\b(model_name|azure_deployment|model_id|model)\b' src/nat/llm 2>/dev/null || true echo echo "Searching for azure_deployment usage outside llm modules…" rg -nP -C2 '\bazure_deployment\b' src 2>/dev/null || true
🧹 Nitpick comments (6)
src/nat/agent/react_agent/agent.py (2)
19-19: Hoist GPT‑5 regex to a module-level constant for reuse.Compiling the same regex inside the method is unnecessary. Define it once at module scope and reuse to keep the hot path cheap and the intent clear.
Add near the imports (outside the changed hunk):
# Models that don't need (or don't support) a stop sequence (e.g., gpt-5, gpt5, gpt-5o, etc.) GPT5_REGEX = re.compile(r"gpt-?5", re.IGNORECASE)
107-115: Docstring nit: clarify phrasing.Minor wording/grammar nits. Consider: “If the LLM is a GPT‑5 variant, return it unmodified; otherwise bind a stop sequence.”
docs/source/extend/model-gated-fields.md (4)
29-34: Clarify precedence and fallback behavior when model keys are missing or conflictingTwo edge cases would benefit from explicit wording:
- If multiple detection keys are present (e.g., both model_name and azure_deployment), which one is used and in what order?
- “No detection keys present → applies default_if_supported” can surprise users on providers where a field is actually unsupported. Consider a note that consumers who can’t surface a model key should prefer supported_models (allowlist) to avoid false-positive defaults, or set model_keys=() to disable auto-defaulting.
Would you like me to open a small docs PR to add a short “Precedence and fallbacks” callout box?
69-80: Tighten wording and add explicit GPT‑5 regex to avoid ambiguity
- Minor grammar/punctuation: end each bullet with a period for consistency.
- Consider adding the actual GPT‑5 detection pattern used by the built-in mixins (e.g., r"gpt-?5") to set expectations about future variants (gpt5, gpt-5, gpt-5.1, etc.).
Suggested text tweak:
- {py:class}`~nat.data_models.temperature_mixin.TemperatureMixin` - - Field: `temperature` in [0, 1] - - Default when supported: `0.0` - - Not supported on GPT-5 models + - Field: `temperature` in [0, 1]. + - Default when supported: `0.0`. + - Not supported on GPT‑5 models (matched via regex like `r"gpt-?5"`). - {py:class}`~nat.data_models.top_p_mixin.TopPMixin` - - Field: `top_p` in [0, 1] - - Default when supported: `1.0` - - Not supported on GPT-5 models + - Field: `top_p` in [0, 1]. + - Default when supported: `1.0`. + - Not supported on GPT‑5 models (matched via regex like `r"gpt-?5"`).Can you confirm the exact regex used in the mixins so the doc reflects the implementation precisely?
81-91: Consider adding a short “provider-specific notes” calloutSince different providers wire in different subsets (e.g., AWSBedrockModelConfig includes TemperatureMixin only per the PR), a one-line note with links to each provider config would help users know what’s available where.
I can add a small table linking provider configs to the gated fields they expose (temperature, top_p), plus any provider-specific model_keys defaults. Want me to draft that?
95-97: Unify with earlier statement by making the “exactly one” constraint explicit here tooThis repeats guidance from above but without “exactly one” phrasing. Suggest aligning the wording and mentioning that the mixin enforces this at validation time with a clear error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
docs/source/extend/model-gated-fields.md(1 hunks)src/nat/agent/react_agent/agent.py(2 hunks)src/nat/data_models/top_p_mixin.py(1 hunks)tests/nat/data_models/test_temperature_mixin.py(1 hunks)tests/nat/data_models/test_top_p_mixin.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/nat/data_models/top_p_mixin.py
- tests/nat/data_models/test_top_p_mixin.py
- tests/nat/data_models/test_temperature_mixin.py
🧰 Additional context used
🪛 LanguageTool
docs/source/extend/model-gated-fields.md
[grammar] ~71-~71: There might be a mistake here.
Context: ... ## Built-in Gated Mixins - {py:class}~nat.data_models.temperature_mixin.TemperatureMixin - Field: temperature in [0, 1] - Defau...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...ixin - Field:temperaturein [0, 1] - Default when supported:0.0` - Not su...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...in [0, 1] - Default when supported:0.0` - Not supported on GPT-5 models - {py:cla...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ... supported on GPT-5 models - {py:class}~nat.data_models.top_p_mixin.TopPMixin - Field: top_p in [0, 1] - Default whe...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ....TopPMixin - Field:top_pin [0, 1] - Default when supported:1.0` - Not su...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...in [0, 1] - Default when supported:1.0` - Not supported on GPT-5 models ### Examp...
(QB_NEW_EN)
🔇 Additional comments (4)
src/nat/agent/react_agent/agent.py (2)
27-35: LGTM on new type imports.
LanguageModelInputandRunnableare correctly used for the method’s return type and composition; imports look appropriate for langchain-core.
103-103: Nice composition change.Delegating the bind decision to a helper keeps
__init__clean and preserves the prompt | llm pipeline ergonomics.docs/source/extend/model-gated-fields.md (2)
22-28: Concept and options are clear and consistent with PR goalsThe explanation of detection keys and the “exactly one of supported_models/unsupported_models” requirement is crisp and aligns with the migration behavior described in the PR summary.
20-21: Nice use of MyST roles for API cross-referencesThe {py:class} references look correct and should resolve in the built docs.
Signed-off-by: Will Killian <[email protected]>
willkill07
force-pushed
the
wkk_gpt5-support
branch
from
1f6f86a to
899d555
Compare
|
I wanted to confirm that this is working so I made the following change: diff --git a/examples/object_store/user_report/configs/config_s3.yml b/examples/object_store/user_report/configs/config_s3.yml
index 3040d4ca..d81a1e32 100644
--- a/examples/object_store/user_report/configs/config_s3.yml
+++ b/examples/object_store/user_report/configs/config_s3.yml
@@ -29,10 +29,9 @@ object_stores:
bucket_name: my-bucket
llms:
- nim_llm:
- _type: nim
- model_name: meta/llama-3.1-70b-instruct
- temperature: 0.0
+ llm:
+ _type: openai
+ model_name: gpt-5
functions:
get_user_report:
@@ -80,7 +79,7 @@ functions:
workflow:
_type: react_agent
tool_names: [get_user_report, put_user_report, update_user_report, delete_user_report]
- llm_name: nim_llm
+ llm_name: llm
verbose: true
handle_parsing_errors: true
max_retries: 2And ran this example: $ nat run --config_file examples/object_store/user_report/configs/config_s3.yml --input 'Get the latest report for user 6789'And got the following (mostly expected) output: Configuration Summary:
--------------------
Workflow Type: react_agent
Number of Functions: 4
Number of LLMs: 1
Number of Embedders: 0
Number of Memory: 0
Number of Object Stores: 1
Number of Retrievers: 0
Number of TTC Strategies: 0
Number of Authentication Providers: 0
2025-08-22 16:09:41,844 - nat.agent.react_agent.agent - INFO -
------------------------------
[AGENT]
Agent input: Get the latest report for user 6789
Agent's thoughts:
Question: Get the latest report for user 6789
Thought: I should fetch the latest report using get_user_report
Action: get_user_report
Action Input: {"user_id":"6789","date":null}
Observation: wait for the human to respond with the result from the tool, do not assume the response
------------------------------
2025-08-22 16:09:41,852 - nat_user_report.user_report_tools - INFO - Fetching report from reports/6789/latest.json
2025-08-22 16:09:41,865 - nat.builder.function - ERROR - Error with ainvoke in function with input: {'user_id': '6789', 'date': None}.
Traceback (most recent call last):
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/packages/nvidia_nat_s3/src/nat/plugins/s3/s3_object_store.py", line 144, in get_object
response = await self._client.get_object(Bucket=self.bucket_name, Key=key)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/.venv/lib/python3.12/site-packages/aiobotocore/context.py", line 36, in wrapper
return await func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/.venv/lib/python3.12/site-packages/aiobotocore/client.py", line 424, in _make_api_call
raise error_class(parsed_response, operation_name)
botocore.errorfactory.NoSuchKey: An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/src/nat/builder/function.py", line 149, in ainvoke
result = await self._ainvoke(converted_input)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/src/nat/builder/function.py", line 325, in _ainvoke
return await self._ainvoke_fn(value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/src/nat/builder/function_info.py", line 466, in _convert_input_pydantic
return await saved_final_single_fn(**value.model_dump())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/examples/object_store/user_report/src/nat_user_report/user_report_tools.py", line 42, in _inner
item = await object_store.get_object(key=key)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/packages/nvidia_nat_s3/src/nat/plugins/s3/s3_object_store.py", line 149, in get_object
raise NoSuchKeyError(key=key, additional_message=str(e))
nat.data_models.object_store.NoSuchKeyError: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
2025-08-22 16:09:41,873 - nat.agent.base - ERROR - [AGENT] Tool call failed after all retry attempts. Last error: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
2025-08-22 16:09:41,873 - nat.agent.base - INFO -
------------------------------
[AGENT]
Calling tools: get_user_report
Tool's input: {'user_id': '6789', 'date': None}
Tool's response:
Tool call failed after all retry attempts. Last error: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
------------------------------
2025-08-22 16:09:41,873 - nat.agent.base - INFO -
------------------------------
[AGENT]
Calling tools: get_user_report
Tool's input: {"user_id":"6789","date":null}
Tool's response:
Tool call failed after all retry attempts. Last error: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
------------------------------
2025-08-22 16:09:50,076 - nat.agent.react_agent.agent - INFO -
------------------------------
[AGENT]
Agent input: Get the latest report for user 6789
Agent's thoughts:
Thought: I now know the final answer
Final Answer: There is no “latest” report for user 6789. How would you like to proceed?
- I can fetch a report for a specific date (YYYY-MM-DD) if you provide one.
- I can create a new “latest” report—send me the report content and I’ll save it.
- Or I can create/update a report for a specific date—provide the date and content.
------------------------------
2025-08-22 16:09:50,081 - nat.front_ends.console.console_front_end_plugin - INFO -
--------------------------------------------------
Workflow Result:
['There is no “latest” report for user 6789. How would you like to proceed?\n- I can fetch a report for a specific date (YYYY-MM-DD) if you provide one.\n- I can create a new “latest” report—send me the report content and I’ll save it.\n- Or I can create/update a report for a specific date—provide the date and content.']
-------------------------------------------------- |
1 similar comment
|
I wanted to confirm that this is working so I made the following change: diff --git a/examples/object_store/user_report/configs/config_s3.yml b/examples/object_store/user_report/configs/config_s3.yml
index 3040d4ca..d81a1e32 100644
--- a/examples/object_store/user_report/configs/config_s3.yml
+++ b/examples/object_store/user_report/configs/config_s3.yml
@@ -29,10 +29,9 @@ object_stores:
bucket_name: my-bucket
llms:
- nim_llm:
- _type: nim
- model_name: meta/llama-3.1-70b-instruct
- temperature: 0.0
+ llm:
+ _type: openai
+ model_name: gpt-5
functions:
get_user_report:
@@ -80,7 +79,7 @@ functions:
workflow:
_type: react_agent
tool_names: [get_user_report, put_user_report, update_user_report, delete_user_report]
- llm_name: nim_llm
+ llm_name: llm
verbose: true
handle_parsing_errors: true
max_retries: 2And ran this example: $ nat run --config_file examples/object_store/user_report/configs/config_s3.yml --input 'Get the latest report for user 6789'And got the following (mostly expected) output: Configuration Summary:
--------------------
Workflow Type: react_agent
Number of Functions: 4
Number of LLMs: 1
Number of Embedders: 0
Number of Memory: 0
Number of Object Stores: 1
Number of Retrievers: 0
Number of TTC Strategies: 0
Number of Authentication Providers: 0
2025-08-22 16:09:41,844 - nat.agent.react_agent.agent - INFO -
------------------------------
[AGENT]
Agent input: Get the latest report for user 6789
Agent's thoughts:
Question: Get the latest report for user 6789
Thought: I should fetch the latest report using get_user_report
Action: get_user_report
Action Input: {"user_id":"6789","date":null}
Observation: wait for the human to respond with the result from the tool, do not assume the response
------------------------------
2025-08-22 16:09:41,852 - nat_user_report.user_report_tools - INFO - Fetching report from reports/6789/latest.json
2025-08-22 16:09:41,865 - nat.builder.function - ERROR - Error with ainvoke in function with input: {'user_id': '6789', 'date': None}.
Traceback (most recent call last):
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/packages/nvidia_nat_s3/src/nat/plugins/s3/s3_object_store.py", line 144, in get_object
response = await self._client.get_object(Bucket=self.bucket_name, Key=key)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/.venv/lib/python3.12/site-packages/aiobotocore/context.py", line 36, in wrapper
return await func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/.venv/lib/python3.12/site-packages/aiobotocore/client.py", line 424, in _make_api_call
raise error_class(parsed_response, operation_name)
botocore.errorfactory.NoSuchKey: An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/src/nat/builder/function.py", line 149, in ainvoke
result = await self._ainvoke(converted_input)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/src/nat/builder/function.py", line 325, in _ainvoke
return await self._ainvoke_fn(value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/src/nat/builder/function_info.py", line 466, in _convert_input_pydantic
return await saved_final_single_fn(**value.model_dump())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/examples/object_store/user_report/src/nat_user_report/user_report_tools.py", line 42, in _inner
item = await object_store.get_object(key=key)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/wkillian/Desktop/NeMo-Agent-Toolkit/packages/nvidia_nat_s3/src/nat/plugins/s3/s3_object_store.py", line 149, in get_object
raise NoSuchKeyError(key=key, additional_message=str(e))
nat.data_models.object_store.NoSuchKeyError: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
2025-08-22 16:09:41,873 - nat.agent.base - ERROR - [AGENT] Tool call failed after all retry attempts. Last error: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
2025-08-22 16:09:41,873 - nat.agent.base - INFO -
------------------------------
[AGENT]
Calling tools: get_user_report
Tool's input: {'user_id': '6789', 'date': None}
Tool's response:
Tool call failed after all retry attempts. Last error: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
------------------------------
2025-08-22 16:09:41,873 - nat.agent.base - INFO -
------------------------------
[AGENT]
Calling tools: get_user_report
Tool's input: {"user_id":"6789","date":null}
Tool's response:
Tool call failed after all retry attempts. Last error: No object found with key: reports/6789/latest.json. An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.
------------------------------
2025-08-22 16:09:50,076 - nat.agent.react_agent.agent - INFO -
------------------------------
[AGENT]
Agent input: Get the latest report for user 6789
Agent's thoughts:
Thought: I now know the final answer
Final Answer: There is no “latest” report for user 6789. How would you like to proceed?
- I can fetch a report for a specific date (YYYY-MM-DD) if you provide one.
- I can create a new “latest” report—send me the report content and I’ll save it.
- Or I can create/update a report for a specific date—provide the date and content.
------------------------------
2025-08-22 16:09:50,081 - nat.front_ends.console.console_front_end_plugin - INFO -
--------------------------------------------------
Workflow Result:
['There is no “latest” report for user 6789. How would you like to proceed?\n- I can fetch a report for a specific date (YYYY-MM-DD) if you provide one.\n- I can create a new “latest” report—send me the report content and I’ll save it.\n- Or I can create/update a report for a specific date—provide the date and content.']
-------------------------------------------------- |
|
/merge |
ModelGatedFieldMixin): support multiple and indirect inheritance; rename to GatedFieldMixin
#707
GPT-5 based models don't (currently) support `temperature` or `top_p`, which is problematic given the defaulted nature of them within the model. To this end, we should dynamically validate based on the model whether or not to include them in the configuration. This has been accomplished through a new base `Mixin` for configurations to inherit from.
> [!NOTE]
> We are not using the latest version of LangGraph/LangChain which potentially offers better parameter support for gpt-5 based models. Some prompts may need to be updated to better support GPT-5. This PR does make it so it will work and run without encountering any errors.
### Summary
- Introduces `ModelGatedFieldMixin` to gate config fields based on model support.
- Adds `TemperatureMixin` and `TopPMixin` to provider configs; rejects unsupported usage for GPT-5 models.
- Updates OpenAI, NIM, and AWS Bedrock configs to include these mixins and defaults.
### Why
- Centralize model-aware validation for config fields.
- Provide sensible defaults and stricter validation to prevent invalid parameters per model.
### Key Changes
- **New**
- `src/nat/data_models/model_gated_field_mixin.py`: Generic mixin to gate fields by model patterns; supports custom `model_keys`.
- `src/nat/data_models/temperature_mixin.py`: `temperature` in [0, 1], default 0.0 when supported; unsupported on models matching `gpt5`.
- `src/nat/data_models/top_p_mixin.py`: `top_p` in [0, 1], default 1.0 when supported; unsupported on models matching `gpt5`.
- Tests for all above mixins:
- `tests/nat/data_models/test_model_gated_field_mixin.py`
- `tests/nat/data_models/test_temperature_mixin.py`
- `tests/nat/data_models/test_top_p_mixin.py`
- **Modified**
- `src/nat/llm/openai_llm.py`: `OpenAIModelConfig` now includes `TemperatureMixin` and `TopPMixin`.
- `src/nat/llm/nim_llm.py`: `NIMModelConfig` now includes `TemperatureMixin` and `TopPMixin`.
- `src/nat/llm/aws_bedrock_llm.py`: `AWSBedrockModelConfig` now includes `TemperatureMixin`.
- `src/nat/llm/azure_openai_llm.py`: `AzureOpenAIModelConfig` now includes `TemperatureMixin` and `TopPMixin`.
### Behavior Details
- **Model-gated validation**
- Looks for model identifiers in `model_name`, `model`, or `azure_deployment` (overridable via `model_keys`).
- If a field is unsupported for the detected model and a value is provided, validation raises an error.
- If supported and value is omitted, applies `default_if_supported`.
- If no model key is present, defaults are applied.
- **Defaults and ranges**
- `temperature`: default 0.0 when supported; must be within [0, 1].
- `top_p`: default 1.0 when supported; must be within [0, 1].
### Breaking/Impact
- Potential validation failures if callers pass `temperature` or `top_p` when using GPT-5 models (match on `gpt5`, case-insensitive).
- Otherwise backward-compatible; new fields are optional and defaulted when supported.
### Migration Guide
- For GPT-5 models:
- Do not pass `temperature` or `top_p` in configs; remove them from workflows and environment config where present.
- For custom models/keys:
- If your config uses a different model identifier field, subclass with `model_keys=("<your_key>",)`.
### Tests
- Coverage for:
- Selector enforcement (`supported_models` vs `unsupported_models`).
- Default application, range validation, and unsupported-field rejections.
- Custom `model_keys` behavior.
- All new tests live under `tests/nat/data_models/`.
#
Authors:
- Will Killian (https://github.com/willkill07)
Approvers:
- David Gardner (https://github.com/dagardner-nv)
- Eric Evans II (https://github.com/ericevans-nv)
URL: NVIDIA#664
Signed-off-by: Sangharsh Aglave <[email protected]>
3 participants
Description
GPT-5 based models don't (currently) support
temperatureortop_p, which is problematic given the defaulted nature of them within the model. To this end, we should dynamically validate based on the model whether or not to include them in the configuration. This has been accomplished through a new baseMixinfor configurations to inherit from.Note
We are not using the latest version of LangGraph/LangChain which potentially offers better parameter support for gpt-5 based models. Some prompts may need to be updated to better support GPT-5. This PR does make it so it will work and run without encountering any errors.
Summary
ModelGatedFieldMixinto gate config fields based on model support.TemperatureMixinandTopPMixinto provider configs; rejects unsupported usage for GPT-5 models.Why
Key Changes
src/nat/data_models/model_gated_field_mixin.py: Generic mixin to gate fields by model patterns; supports custommodel_keys.src/nat/data_models/temperature_mixin.py:temperaturein [0, 1], default 0.0 when supported; unsupported on models matchinggpt5.src/nat/data_models/top_p_mixin.py:top_pin [0, 1], default 1.0 when supported; unsupported on models matchinggpt5.tests/nat/data_models/test_model_gated_field_mixin.pytests/nat/data_models/test_temperature_mixin.pytests/nat/data_models/test_top_p_mixin.pysrc/nat/llm/openai_llm.py:OpenAIModelConfignow includesTemperatureMixinandTopPMixin.src/nat/llm/nim_llm.py:NIMModelConfignow includesTemperatureMixinandTopPMixin.src/nat/llm/aws_bedrock_llm.py:AWSBedrockModelConfignow includesTemperatureMixin.src/nat/llm/azure_openai_llm.py:AzureOpenAIModelConfignow includesTemperatureMixinandTopPMixin.Behavior Details
model_name,model, orazure_deployment(overridable viamodel_keys).default_if_supported.temperature: default 0.0 when supported; must be within [0, 1].top_p: default 1.0 when supported; must be within [0, 1].Breaking/Impact
temperatureortop_pwhen using GPT-5 models (match ongpt5, case-insensitive).Migration Guide
temperatureortop_pin configs; remove them from workflows and environment config where present.model_keys=("<your_key>",).Tests
supported_modelsvsunsupported_models).model_keysbehavior.tests/nat/data_models/.Checklist
Documentation
What it is
How it works
model_name,model,azure_deployment(overridable viamodel_keys)unsupported_models: field is supported unless a regex matchessupported_models: field is supported only if a regex matchesdefault_if_supportedValidationErrorNonedefault_if_supportedDefining a gated field (example)
Overriding model selector keys
Using the built-in mixins
TemperatureMixintemperaturein [0, 1]0.0gpt5(case-insensitive)TopPMixintop_pin [0, 1]1.0gpt5(case-insensitive)Example provider config integration:
Best practices
supported_modelsfor allowlists andunsupported_modelsfor denylists; do not set both.model_keysaccordingly.Closes #610
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores