fix(ModelGatedFieldMixin): support multiple and indirect inheritance; rename to GatedFieldMixin#707
Conversation
…ritance Signed-off-by: Will Killian <wkillian@nvidia.com>
WalkthroughRemoves ModelGatedFieldMixin and adds a new GatedFieldMixin with keys-based gating (supported/unsupported regex). Updates TemperatureMixin and TopPMixin to use GatedFieldMixin, replaces tests with comprehensive gated-field tests, and renames/rewrites docs to "Gated Fields" with updated examples and links. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Sub as Subclass (BaseModel + Mixins)
participant Gated as GatedFieldMixin
participant Configs as Mixin Configs
participant Pyd as Pydantic Validation
Dev->>Sub: Define class using GatedFieldMixin(s) (field_name, keys, supported/unsupported)
Sub->>Gated: __init_subclass__ registers per-field configs & validators
Gated->>Configs: _collect_all_mixin_configs()
Gated->>Sub: _create_combined_validator(configs) attach combined validator
Dev->>Pyd: Instantiate Sub(..., model_name=..., model=..., azure_deployment=...)
Pyd->>Sub: run model validators -> combined gated validator
alt Field supported and value missing
Sub-->>Pyd: set default_if_supported for that field
else Field unsupported and value present
Sub-->>Pyd: raise ValidationError with blocking key/value
else Field supported and value present
Sub-->>Pyd: accept value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nat/data_models/model_gated_field_mixin.py (1)
236-253: Instance methods bound at class level could cause issuesThe helper methods
_check_field_support_instanceand_find_blocking_key_instanceare defined as instance methods but then assigned to the class. This unusual pattern could lead to confusion and potential issues with method resolution order.Consider making these proper instance methods or storing them differently:
@classmethod def _create_combined_validator(cls, all_mixins: list[ModelGatedFieldMixinConfig]) -> None: """Create a combined validator that handles all fields.""" @model_validator(mode="after") def combined_model_validate(self): """Validate all model-gated fields.""" for mixin_config in all_mixins: field_name_local = mixin_config.field_name current_value = getattr(self, field_name_local, None) - if not self._check_field_support_instance(mixin_config): + if not cls._check_field_support(self, + mixin_config.unsupported_models, + mixin_config.supported_models, + mixin_config.model_keys): if current_value is not None: - blocking_key = self._find_blocking_key_instance(mixin_config) + blocking_key = cls._find_blocking_key(self, + mixin_config.unsupported_models, + mixin_config.supported_models, + mixin_config.model_keys) model_value = getattr(self, blocking_key, "<unknown>") raise ValueError(f"{field_name_local} is not supported for {blocking_key}: {model_value}") elif current_value is None: setattr(self, field_name_local, mixin_config.default_if_supported) return self cls._combined_model_gated_field_validator = combined_model_validate - - # Add helper methods - def _check_field_support_instance(self, mixin_config: ModelGatedFieldMixinConfig) -> bool: - """Check if a specific field is supported based on its configuration.""" - return cls._check_field_support(self, - mixin_config.unsupported_models, - mixin_config.supported_models, - mixin_config.model_keys) - - def _find_blocking_key_instance(self, mixin_config: ModelGatedFieldMixinConfig) -> str: - """Find which model key is blocking the field.""" - return cls._find_blocking_key(self, - mixin_config.unsupported_models, - mixin_config.supported_models, - mixin_config.model_keys) - - cls._check_field_support_instance = _check_field_support_instance - cls._find_blocking_key_instance = _find_blocking_key_instance
🧹 Nitpick comments (3)
src/nat/data_models/model_gated_field_mixin.py (1)
133-134: Consider improving the validation conditionThe condition
if model_keys is not None and len(model_keys) == 0:is redundant sincemodel_keysalways has a value (it has a default value in the method signature). The check should just beif len(model_keys) == 0:.- if model_keys is not None and len(model_keys) == 0: + if len(model_keys) == 0: raise ValueError("model_keys must be provided and non-empty when subclassing ModelGatedFieldMixin")tests/nat/data_models/test_model_gated_field_mixin.py (2)
26-50: Consider making fixtures return pattern lists for consistencyThe fixtures are well-organized, but since the mixin expects sequences of patterns, consider having fixtures return tuples/lists for direct use in tests.
@pytest.fixture def gpt_pattern(): - return re.compile(r"gpt") + return (re.compile(r"gpt"),) @pytest.fixture def claude_pattern(): - return re.compile(r"claude") + return (re.compile(r"claude"),)This would simplify test usage from
(gpt_pattern,)to justgpt_pattern.
176-192: Add explicit tests for multi-key priority and fallback behaviorThe existing
test_first_key_supportedcorrectly verifies that when the first model key matches the supported pattern, the gated field uses thedefault_if_supportedvalue. To fully exercise the priority logic in_check_field_support, please add:
- A test where the first key does not match but a later key does, confirming that the field is supported in that case (fallback behavior).
- A test where both the first and a subsequent key match the pattern, ensuring that the mixin returns support based solely on the first key.
- (Optional) A test for the unsupported_models case mirroring the above scenarios to verify blocking logic.
This will ensure each branch of the key-priority loop is covered and that fallback and blocking behaviors work as intended.
📜 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 (2)
src/nat/data_models/model_gated_field_mixin.py(3 hunks)tests/nat/data_models/test_model_gated_field_mixin.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments, use abbreviations: NAT (informal/comments), nat (API namespace/CLI), nvidia-nat (package). Do not use these abbreviations in documentation.
Run isort first with line_length=120, multi_line_output=3, include_trailing_comma=true, force_single_line=true.
Run yapf second with PEP8 base and column_limit=120.
Respect Python naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants.
Fix flake8 (pflake8) warnings unless explicitly ignored in pyproject.toml.
All public Python APIs require type hints (Python 3.11+) on parameters and return values.
Prefer typing/collections.abc abstractions (e.g., Sequence over list).
Use typing.Annotated to express units or extra metadata when useful.
Treat pyright warnings as errors during development (configured via pyproject.toml).
Provide Google‑style docstrings for every public module, class, function, and CLI command.
Docstring first line must be a concise description ending with a period.
Do not hard‑code version numbers in Python code; versions are derived by setuptools‑scm.
Prefer httpx with SSL verification enabled by default for HTTP calls; follow OWASP Top‑10.
Use async/await for I/O‑bound work (HTTP, DB, file reads).
Cache expensive computations with functools.lru_cache or an external cache when appropriate.
Leverage NumPy vectorized operations when beneficial and feasible.
Files:
tests/nat/data_models/test_model_gated_field_mixin.pysrc/nat/data_models/model_gated_field_mixin.py
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use pytest, with pytest‑asyncio for asynchronous tests.
Files:
tests/nat/data_models/test_model_gated_field_mixin.py
**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*: Every file must start with the standard SPDX Apache‑2.0 header.
All source files must include the SPDX Apache‑2.0 header template; CI verifies via ci/scripts/github/checks.sh.
Files:
tests/nat/data_models/test_model_gated_field_mixin.pysrc/nat/data_models/model_gated_field_mixin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ or packages//src/.
Files:
src/nat/data_models/model_gated_field_mixin.py
🧬 Code graph analysis (1)
tests/nat/data_models/test_model_gated_field_mixin.py (1)
src/nat/data_models/model_gated_field_mixin.py (1)
ModelGatedFieldMixin(40-252)
🔇 Additional comments (7)
src/nat/data_models/model_gated_field_mixin.py (5)
26-35: Good use of dataclass for configuration managementThe
ModelGatedFieldMixinConfigdataclass effectively encapsulates the mixin configuration. This improves code organization and makes it easier to pass configuration between methods.
63-70: Return type annotation added correctlyGood addition of the
-> Nonereturn type annotation to__init_subclass__, following Python typing best practices.
74-86: Robust inheritance detection logicThe logic correctly handles both direct inheritance from
ModelGatedFieldMixinand deep inheritance chains. The approach of checkingcls.__bases__for direct inheritance and then always attempting to collect mixins from all base classes ensures proper support for complex inheritance hierarchies.
87-120: Well-structured direct mixin setupThe
_setup_direct_mixinmethod effectively:
- Validates parameters before processing
- Creates and stores the validator with a unique name
- Initializes the
_model_gated_mixinslist if needed- Stores the configuration for later use in multiple inheritance scenarios
This separation of concerns improves maintainability.
172-182: No inconsistency in early returns—logic is symmetricThe
_check_field_supportloop always exits on the first matching key, regardless of whether you’re usingsupported_modelsorunsupported_models. Theelifis only reached whensupported_modelsisNone, so in both cases you return immediately on the first key that exists on the instance. There is no branch that “continues checking all keys” in theunsupported_modelscase.No changes needed.
Likely an incorrect or invalid review comment.
tests/nat/data_models/test_model_gated_field_mixin.py (2)
52-301: Comprehensive test coverage for the mixin functionalityThe test suite is well-structured and covers:
- Parameter validation edge cases
- Default value application
- Multiple model keys
- Numeric model values
- Deep inheritance chains
- Combined validator scenarios
- Error cases with appropriate validation messages
The parameterized tests and clear test names make the test suite maintainable and easy to understand.
256-283: Excellent test for deep inheritance chainsThis test effectively validates that the mixin works correctly through intermediate classes that don't directly inherit from
ModelGatedFieldMixin. It properly tests:
- Default value application when supported
- Validation error when unsupported with a value
- Preserving explicitly set values when supported
This addresses one of the key fixes mentioned in the PR objectives.
willkill07
changed the title
ModelGatedFieldMixin supports multiple and indirect inheritanceModelGatedFieldMixin to support multiple and indirect inheritance
willkill07
changed the title
ModelGatedFieldMixin to support multiple and indirect inheritanceModelGatedFieldMixin to support multiple and indirect inheritance
Signed-off-by: Will Killian <wkillian@nvidia.com>
willkill07
changed the title
ModelGatedFieldMixin to support multiple and indirect inheritanceModelGatedFieldMixin): support multiple and indirect inheritance; rename to GatedFieldMixin
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
docs/source/extend/gated-fields.md (2)
24-34: Clarify default detection keys to match the API.The docs imply “keys” may be omitted, but they don’t state the default explicitly. Recommend clarifying that keys default to ("model_name", "model", "azure_deployment") so readers don’t assume they must always pass them.
Apply this edit to make the default explicit:
- - **Detection keys**: The mixin scans `keys` specified on the instance to identify values used to determine if the field is supported. + - **Detection keys**: The mixin scans `keys` specified on the instance to identify values used to determine if the field is supported. Defaults to `("model_name", "model", "azure_deployment")`.
70-79: Tighten wording for model family examples.“Not supported on GPT-5 models” is fine for illustrative purposes, but consider aligning the phrasing with the regex used throughout (
gpt-?5) to avoid confusion for variants (e.g., “gpt5”, “gpt-5”). A short parenthetical would help.Suggested tweak:
- - Not supported on GPT-5 models + - Not supported on GPT‑5 models (matched via `gpt-?5`)src/nat/data_models/top_p_mixin.py (2)
21-31: Remove stray whitespace in tuple literal (yapf/isort consistency).There’s an extra space before the closing parenthesis in the singleton tuple for
unsupported. This can trip formatting checks.- unsupported=(re.compile(r"gpt-?5", re.IGNORECASE), ), + unsupported=(re.compile(r"gpt-?5", re.IGNORECASE),),
32-36: Enrich docstring to Google style (public API).Per guidelines, public classes should have Google‑style docstrings. Consider briefly documenting the field, default, and gating condition.
Example:
class TopPMixin(...): """ Mixin class for top-p configuration. Attributes: top_p: Top‑p for distribution sampling in [0.0, 1.0]. Defaults to 1.0 when supported. """src/nat/data_models/temperature_mixin.py (2)
21-31: Remove stray whitespace in tuple literal (yapf/isort consistency).Same formatting nit as in TopPMixin.
- unsupported=(re.compile(r"gpt-?5", re.IGNORECASE), ), + unsupported=(re.compile(r"gpt-?5", re.IGNORECASE),),
32-36: Enrich docstring to Google style (public API).Add a short Google‑style docstring describing the attribute and defaulting behavior.
Example:
class TemperatureMixin(...): """ Mixin class for temperature configuration. Attributes: temperature: Sampling temperature in [0.0, 1.0]. Defaults to 0.0 when supported. """tests/nat/data_models/test_gated_field_mixin.py (2)
223-236: Redundant fallback tests — consider consolidating.
test_no_model_key_fallbackandtest_no_model_keys_fallbackvalidate essentially the same behavior (defaulting when no keys are present/usable). You can consolidate to a single parametrized test to keep the suite lean.
296-298: Add a test for default keys (if keys become optional).If we accept default keys at the mixin level (see suggested change in GatedFieldMixin), add a test to capture that behavior.
Proposed addition:
+ def test_default_keys_are_used_when_omitted(self, gpt_pattern): + class DefaultKeysModel(BaseModel, + GatedFieldMixin[int], + field_name="feat", + default_if_supported=7, + supported=(gpt_pattern,)): + feat: int | None = Field(default=None) + model_name: str + + m = DefaultKeysModel(model_name="gpt-4") + assert m.feat == 7src/nat/data_models/gated_field_mixin.py (2)
205-211: Collect mixin configs across the full MRO to support deep/diamond inheritance.
_collect_all_mixin_configsonly inspects direct bases. In deep or diamond hierarchies, this can miss mixins defined on ancestors beyond one level. Walking the MRO is safer.- def _collect_all_mixin_configs(cls) -> list[GatedFieldMixinConfig]: - """Collect all mixin configurations from base classes.""" - all_mixins = [] - for base in cls.__bases__: - if hasattr(base, "_gated_field_mixins"): - all_mixins.extend(base._gated_field_mixins) - return all_mixins + def _collect_all_mixin_configs(cls) -> list[GatedFieldMixinConfig]: + """Collect all mixin configurations from all ancestor classes in MRO (excluding cls).""" + seen: set[tuple[str, tuple[str, ...]]] = set() + collected: list[GatedFieldMixinConfig] = [] + for base in cls.__mro__[1:]: + mixins = getattr(base, "_gated_field_mixins", None) + if not mixins: + continue + for cfg in mixins: + key = (cfg.field_name, tuple(cfg.keys)) + if key in seen: + continue + seen.add(key) + collected.append(cfg) + return collectedYou can verify coverage by composing a model that inherits from two intermediate classes, each inheriting from different direct mixins, and asserting both fields are handled by the combined validator.
213-245: Potential double-validation when combining mixins.When a final model aggregates multiple mixins, base classes still carry their per‑field validators, and the combined validator runs again on the same fields. Functionally OK, but it can duplicate work and duplicate errors. Optional: add a guard (e.g., a private flag) to disable per‑field validators when a combined validator is installed.
One lightweight approach is to set a class flag in
_create_combined_validatorand early‑return in per‑field validators if the flag is present. If you want, I can draft a small refactor that keeps behavior identical while avoiding duplicate runs.
📜 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 (9)
docs/source/extend/adding-an-llm-provider.md(1 hunks)docs/source/extend/gated-fields.md(4 hunks)docs/source/index.md(1 hunks)docs/source/workflows/llms/index.md(3 hunks)src/nat/data_models/gated_field_mixin.py(1 hunks)src/nat/data_models/model_gated_field_mixin.py(0 hunks)src/nat/data_models/temperature_mixin.py(1 hunks)src/nat/data_models/top_p_mixin.py(1 hunks)tests/nat/data_models/test_gated_field_mixin.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/nat/data_models/model_gated_field_mixin.py
✅ Files skipped from review due to trivial changes (3)
- docs/source/index.md
- docs/source/extend/adding-an-llm-provider.md
- docs/source/workflows/llms/index.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments, use abbreviations: NAT (informal/comments), nat (API namespace/CLI), nvidia-nat (package). Do not use these abbreviations in documentation.
Run isort first with line_length=120, multi_line_output=3, include_trailing_comma=true, force_single_line=true.
Run yapf second with PEP8 base and column_limit=120.
Respect Python naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants.
Fix flake8 (pflake8) warnings unless explicitly ignored in pyproject.toml.
All public Python APIs require type hints (Python 3.11+) on parameters and return values.
Prefer typing/collections.abc abstractions (e.g., Sequence over list).
Use typing.Annotated to express units or extra metadata when useful.
Treat pyright warnings as errors during development (configured via pyproject.toml).
Provide Google‑style docstrings for every public module, class, function, and CLI command.
Docstring first line must be a concise description ending with a period.
Do not hard‑code version numbers in Python code; versions are derived by setuptools‑scm.
Prefer httpx with SSL verification enabled by default for HTTP calls; follow OWASP Top‑10.
Use async/await for I/O‑bound work (HTTP, DB, file reads).
Cache expensive computations with functools.lru_cache or an external cache when appropriate.
Leverage NumPy vectorized operations when beneficial and feasible.
Files:
tests/nat/data_models/test_gated_field_mixin.pysrc/nat/data_models/gated_field_mixin.pysrc/nat/data_models/temperature_mixin.pysrc/nat/data_models/top_p_mixin.py
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use pytest, with pytest‑asyncio for asynchronous tests.
Files:
tests/nat/data_models/test_gated_field_mixin.py
**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*: Every file must start with the standard SPDX Apache‑2.0 header.
All source files must include the SPDX Apache‑2.0 header template; CI verifies via ci/scripts/github/checks.sh.
Files:
tests/nat/data_models/test_gated_field_mixin.pysrc/nat/data_models/gated_field_mixin.pysrc/nat/data_models/temperature_mixin.pydocs/source/extend/gated-fields.mdsrc/nat/data_models/top_p_mixin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ or packages//src/.
Files:
src/nat/data_models/gated_field_mixin.pysrc/nat/data_models/temperature_mixin.pysrc/nat/data_models/top_p_mixin.py
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Use the exact product naming in documentation: first use “NVIDIA NeMo Agent toolkit”; subsequent references “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T).
Never use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq) in documentation unless explicitly referring to deprecated names or compatibility layers.
Use the intended name “AIQ Blueprint” and do not change it in docs.
Documentation sources must be Markdown files under docs/source.
Surround code entities with backticks in documentation to avoid Vale false positives.
Keep documentation in sync with code; Sphinx build must be error‑free with no broken links.
Do not hard‑code project version numbers in documentation; versions are derived by setuptools‑scm.
Files:
docs/source/extend/gated-fields.md
🧬 Code graph analysis (3)
tests/nat/data_models/test_gated_field_mixin.py (1)
src/nat/data_models/gated_field_mixin.py (1)
GatedFieldMixin(39-245)
src/nat/data_models/temperature_mixin.py (1)
src/nat/data_models/gated_field_mixin.py (1)
GatedFieldMixin(39-245)
src/nat/data_models/top_p_mixin.py (1)
src/nat/data_models/gated_field_mixin.py (1)
GatedFieldMixin(39-245)
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/nat/data_models/temperature_mixin.py (5)
16-16: Add a module-level docstring right after the license header.Per the coding guidelines, public modules must include a concise Google-style module docstring. Please add one describing the purpose of this mixin and its gating behavior.
@@ import re +"""Temperature mixin gated by model identity. + +This module provides TemperatureMixin, which exposes a temperature field gated by model keys +via GatedFieldMixin. When a recognized model key indicates support and the field is unset, +the mixin defaults temperature to 0.0. +""" + from pydantic import BaseModel from pydantic import Field
30-31: Regex coverage for future GPT-5 variants — confirm scope.The pattern
gpt-?5will match gpt-5, gpt5, and also strings like gpt-5o/gpt5o (because of substring match), which is likely desirable. If you intend to exclude only the base “gpt-5” but allow “gpt-5o” or other suffixed variants, this pattern is too broad; conversely, if you intend to exclude the entire 5.x family, you may want to future-proof with something likegpt-?5(\D|$).
33-37: Clarify docstring with key precedence and fallback behavior.Given the early-return evaluation across keys and the defaulting behavior when no key is present (treated as supported), documenting this explicitly will prevent surprises for consumers.
""" - Mixin class for temperature configuration. Unsupported on models like gpt-5. + Mixin class for temperature configuration. + + Unsupported on models like the GPT‑5 family (see `unsupported` patterns). Key precedence for gating is + evaluated in the order specified by `keys` (first present key wins). If no listed key is present on a + model instance, the field is treated as supported and will default as described below. Attributes: - temperature: Sampling temperature in [0, 1]. Defaults to 0.0 when supported on the model. + temperature: Sampling temperature in [0, 1]. When supported and unset, defaults to 0.0. If unsupported + for the detected model key, setting a non-None value raises a validation error. """
38-43: Field definition LGTM; optional: add examples to aid schema/docs.The constraints and description are correct. Optionally add
examplesto improve generated docs.temperature: float | None = Field( default=None, ge=0.0, le=1.0, - description="Sampling temperature in [0, 1]. Defaults to 0.0 when supported on the model.", + description="Sampling temperature in [0, 1]. Defaults to 0.0 when supported on the model.", + json_schema_extra={"examples": [0.0, 0.2, 0.7]}, )
24-31: Prioritize Azure deployment in temperature gating logic
TheGatedFieldMixinstops at the first matching key when determining support, so for Azure hosts (which define bothmodel_nameandazure_deployment), you must checkazure_deploymentfirst. Otherwise genericmodel_namewill always win, ignoring the intended deployment override.Files to update:
- src/nat/data_models/temperature_mixin.py (keys tuple)
- src/nat/data_models/top_p_mixin.py (keys tuple; keep consistency)
Suggested changes:
--- a/src/nat/data_models/temperature_mixin.py @@ class TemperatureMixin( - keys=("model_name", "model", "azure_deployment"), + # Precedence when multiple model identifiers are present: + # 1. azure_deployment 2. model 3. model_name + keys=("azure_deployment", "model", "model_name"),--- a/src/nat/data_models/top_p_mixin.py @@ class TopPMixin( - keys=("model_name", "model", "azure_deployment"), + # Precedence when multiple model identifiers are present: + # 1. azure_deployment 2. model 3. model_name + keys=("azure_deployment", "model", "model_name"),Additionally, update each mixin’s docstring (just above the class declaration) to note that key precedence is azure_deployment > model > model_name, so that downstream users understand which identifier governs support.
📜 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 (3)
src/nat/data_models/temperature_mixin.py(1 hunks)src/nat/data_models/top_p_mixin.py(1 hunks)tests/nat/data_models/test_gated_field_mixin.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/nat/data_models/test_gated_field_mixin.py
- src/nat/data_models/top_p_mixin.py
🧰 Additional context used
📓 Path-based instructions (7)
{src/**/*.py,packages/*/src/**/*.py,examples/*/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' only for env var prefixes and informal code comments (never in documentation)
Files:
src/nat/data_models/temperature_mixin.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ so namespace packages resolve correctly
Files:
src/nat/data_models/temperature_mixin.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial
Files:
src/nat/data_models/temperature_mixin.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
src/nat/data_models/temperature_mixin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All public APIs require Python 3.11+ type hints on parameters and return values
Provide Google-style docstrings for every public module, class, function and CLI command
Docstring first line must be a concise description ending with a period
Files:
src/nat/data_models/temperature_mixin.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
src/nat/data_models/temperature_mixin.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
src/nat/data_models/temperature_mixin.py
🧬 Code graph analysis (1)
src/nat/data_models/temperature_mixin.py (1)
src/nat/data_models/gated_field_mixin.py (1)
GatedFieldMixin(39-244)
🔇 Additional comments (1)
src/nat/data_models/temperature_mixin.py (1)
21-21: Import switch to GatedFieldMixin is correct.Matches the PR’s rename and refactor objectives; path and symbol look good.
|
/merge |
2 participants
Summary
This PR addresses a critical limitation in the
ModelGatedFieldMixinclass that prevented it from working correctly with multiple inheritance patterns and deep inheritance chains. The fix enables the mixin to properly handle complex class hierarchies while maintaining backward compatibility.It also generalizes the naming by removing
Model--GatedFieldMixinas it is capable of gating on any configuration field.Problem
The original implementation had several limitations:
ModelGatedFieldMixinSolution
Refactored the mixin architecture to support:
Key Changes
Core Architecture
ModelGatedFieldMixinConfigdataclass to store mixin configuration_collect_all_mixin_configs()to gather configurations from base classes_create_combined_validator()for handling multiple fieldsMethod Refactoring
_setup_direct_mixin()for direct inheritance scenarios_check_field_support()and_find_blocking_key()as reusable utilitiesInheritance Handling
__init_subclass__to detect both direct and indirect inheritanceTesting
Impact
This fix enables more flexible and robust usage of the
ModelGatedFieldMixinin complex NeMo Agent Toolkit workflows while maintaining full backward compatibility.By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests