feat!: Implement Function Groups #684
Conversation
willkill07
added
feature request
willkill07
force-pushed
the
wkk_function-groups
branch
11 times, most recently
from
401b77f to
195624c
Compare
WalkthroughAdds first-class Function Groups: data models/configs, builder/runtime APIs, CLI/type-registry registration, workflow integration, agent configs accepting group refs, tooling for group-based tool resolution, docs/examples updated, GitHub tools consolidated, tests and mocks extended to exercise groups. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant CLI as register_function_group
participant TR as TypeRegistry
participant WB as WorkflowBuilder
participant RG as RegisteredBuildFn
participant FG as FunctionGroup
Dev->>CLI: @register_function_group(config_type=...)
CLI->>TR: register_function_group(RegisteredFunctionGroupInfo)
TR-->>CLI: OK
Dev->>WB: add_function_group("user_report", config)
WB->>RG: invoke build_fn(config, builder)
RG-->>WB: yields FunctionGroup
WB->>WB: register exposed functions (instance.function_name)
WB-->>Dev: FunctionGroup recorded in workflow
sequenceDiagram
autonumber
participant CFG as AgentConfig
participant AG as Agent
participant WB as WorkflowBuilder
participant FG as FunctionGroup
CFG-->>AG: tool_names = [FunctionGroupRef("user_report"), FunctionRef("tool_x")]
AG->>WB: get_tools(tool_names, wrapper_type)
WB->>FG: get_accessible_functions("user_report")
FG-->>WB: {"user_report.get","user_report.put",...}
WB->>WB: wrap group functions + individual tools
WB-->>AG: list of wrapped tools
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
willkill07
force-pushed
the
wkk_function-groups
branch
from
195624c to
e7b4a0e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
willkill07
force-pushed
the
wkk_function-groups
branch
from
e7b4a0e to
be40b6c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
willkill07
force-pushed
the
wkk_function-groups
branch
2 times, most recently
from
7f46af3 to
a44e4e9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
willkill07
force-pushed
the
wkk_function-groups
branch
from
e049ca4 to
164e4ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/object_store/user_report/configs/config_redis.yml (1)
60-66: Outdated function name in docs; use namespaced group function.Docs reference delete_user_report, which no longer exists with function groups. Use user_report.delete (or the exact namespaced address your runtime resolves).
- delete_description: > - Deletes user diagnostic report from the bucket given a user ID and date. If the report does not exist, it will fail. - Do not run `delete_user_report` without the explicit intention to delete an existing report, even in the case of an attempt - to insert a new report which already exists. + delete_description: | + Deletes a user diagnostic report from the bucket given a user ID and date. If the report does not exist, it will fail. + Do not call `user_report.delete` unless deletion is explicitly intended; never use it to resolve put/update conflicts. Args: user_id: str: The user ID to delete the report for. date: str | null: The date to delete the report for. Format: YYYY-MM-DD. If not provided, the report will be named "latest".tests/nat/builder/test_builder.py (1)
891-934: Fix shadowed variable; assertion is tautological.workflow_config is redefined to workflow.config, making
workflow_config.workflow == workflow_config.workflowmeaningless. Compare built config to the original workflow config instead.@@ - function_config = FunctionReturningFunctionConfig() - workflow_config = FunctionReturningFunctionConfig() + function_config = FunctionReturningFunctionConfig() + workflow_cfg = FunctionReturningFunctionConfig() @@ - await builder.set_workflow(workflow_config) + await builder.set_workflow(workflow_cfg) @@ - workflow_config = workflow.config + built_config = workflow.config @@ - assert workflow_config.general == general_config - assert workflow_config.functions == {"function1": function_config} - assert workflow_config.workflow == workflow_config.workflow - assert workflow_config.llms == {"llm1": llm_config} - assert workflow_config.embedders == {"embedder1": embedder_config} - assert workflow_config.memory == {"memory1": memory_config} - assert workflow_config.retrievers == {"retriever1": retriever_config} - assert workflow_config.object_stores == {"object_store1": object_store_config} - assert workflow_config.ttc_strategies == {"ttc_strategy": ttc_config} + assert built_config.general == general_config + assert built_config.functions == {"function1": function_config} + assert built_config.workflow == workflow_cfg + assert built_config.llms == {"llm1": llm_config} + assert built_config.embedders == {"embedder1": embedder_config} + assert built_config.memory == {"memory1": memory_config} + assert built_config.retrievers == {"retriever1": retriever_config} + assert built_config.object_stores == {"object_store1": object_store_config} + assert built_config.ttc_strategies == {"ttc_strategy": ttc_config}
♻️ Duplicate comments (7)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (4)
147-150: Signature narrower than abstract; widen to accept FunctionGroupRef.
Prevents type-checking drift and override mismatch.- async def add_function_group(self, name: str, config: FunctionGroupBaseConfig) -> FunctionGroup: + async def add_function_group(self, name: str | FunctionGroupRef, config: FunctionGroupBaseConfig) -> FunctionGroup: """Mock implementation - not used in tool testing.""" raise NotImplementedError("Mock implementation does not support add_function_group")
20-20: Import framework and ref types to match Builder API.
Prepares for widened method signatures below.from collections.abc import Sequence +from nat.builder.framework_enum import LLMFrameworkEnum +from nat.data_models.component_ref import FunctionRef, FunctionGroupRef
159-162: Widen get_function_group_config signature.
Matches abstract definition.- def get_function_group_config(self, name: str) -> FunctionGroupBaseConfig: + def get_function_group_config(self, name: str | FunctionGroupRef) -> FunctionGroupBaseConfig: """Mock implementation.""" return FunctionGroupBaseConfig()
179-182: Match Builder.get_tools signature and avoid ARG002.
Accept refs; annotate return; mark args as intentionally unused.- def get_tools(self, tool_names: Sequence[str], wrapper_type): - """Mock implementation.""" - return [] + def get_tools( + self, + tool_names: Sequence[str | FunctionRef | FunctionGroupRef], + wrapper_type: LLMFrameworkEnum | str, + ) -> list[typing.Any]: + """Mock implementation.""" + _ = (tool_names, wrapper_type) + return []src/nat/data_models/config.py (2)
359-361: WorkflowAnnotation should accept FunctionGroup configs as well.
Else workflows defined as a function group will fail validation.- WorkflowAnnotation = typing.Annotated[(type_registry.compute_annotation(FunctionBaseConfig)), - Discriminator(TypedBaseModel.discriminator)] + WorkflowAnnotation = typing.Annotated[ + ( + type_registry.compute_annotation(FunctionBaseConfig) + | type_registry.compute_annotation(FunctionGroupBaseConfig) + ), + Discriminator(TypedBaseModel.discriminator), + ]
63-66: Workflow suggestions should include both functions and function groups.
Improves diagnostics for mixed workflows.- if (info.field_name in ('workflow', 'functions')): - registered_keys = GlobalTypeRegistry.get().get_registered_functions() + if info.field_name == 'workflow': + registered_keys = ( + list(GlobalTypeRegistry.get().get_registered_functions()) + + list(GlobalTypeRegistry.get().get_registered_function_groups()) + ) + elif info.field_name == 'functions': + registered_keys = GlobalTypeRegistry.get().get_registered_functions() elif (info.field_name == "function_groups"): registered_keys = GlobalTypeRegistry.get().get_registered_function_groups()src/nat/builder/workflow_builder.py (1)
1157-1167: ChildBuilder.get_tools(): normalize keys before membership checks.
FunctionGroupRef instances won’t match without normalization.- tools = self._workflow_builder.get_tools(tool_names, wrapper_type) - for tool_name in tool_names: - if tool_name in self._workflow_builder._function_groups: - self._dependencies.add_function_group(tool_name) - else: - self._dependencies.add_function(tool_name) + tools = self._workflow_builder.get_tools(tool_names, wrapper_type) + for tool_name in tool_names: + key = str(tool_name) + if key in self._workflow_builder._function_groups: + self._dependencies.add_function_group(key) + else: + self._dependencies.add_function(key) return tools
🧹 Nitpick comments (12)
examples/object_store/user_report/configs/config_redis.yml (3)
41-46: Preserve formatting in multi-line descriptions.Use literal block scalars (|) to retain line breaks for Args lists; folded scalars (>) collapse newlines.
Apply:
- get_description: > + get_description: | Fetches user diagnostic report from the bucket given a user ID and date. Args: user_id: str: The user ID to fetch the report for. date: str | null: The date to fetch the report for. Format: YYYY-MM-DD. If not provided, the latest report will be fetched.
46-53: Clarify conflict behavior for put to avoid unintended overwrites.Be explicit that put must not overwrite existing content and should return a conflict; tighten wording.
- put_description: > - Inserts a new user diagnostic report into the bucket given a user ID and date. If the report already exists, it will fail. - If it does fail, never delete the existing report to replace it. Just let the user know that the report already exists. + put_description: | + Inserts a new user diagnostic report into the bucket given a user ID and date. + If a report already exists for the (user_id, date) key, do not overwrite it; return a conflict error. + Never delete or replace an existing report as part of this operation. Args: report: str: The report to put into the bucket. user_id: str: The user ID to put the report for. date: str | null: The date to put the report for. Format: YYYY-MM-DD. If not provided, the report will be named "latest".
53-60: State “upsert” semantics for update; ensure idempotency.Spell out that update should create when missing and be idempotent.
- update_description: > - Updates a user diagnostic report in the bucket given a user ID and date. If the report does not exist, it will behave as - if the report was created. Do not delete the existing report to replace it. + update_description: | + Updates a user diagnostic report for the given user_id and date. + If the report does not exist, create it (upsert semantics). Do not delete/recreate existing reports. + The operation should be idempotent for the same (user_id, date, report) inputs. Args: report: str: The report to update in the bucket. user_id: str: The user ID to update the report for. date: str | null: The date to update the report for. Format: YYYY-MM-DD. If not provided, the report will be named "latest".src/nat/data_models/function.py (1)
51-56: Add explicit return type on model validator (nit).
Helps pyright/ruff consistency.Apply:
- @model_validator(mode="after") - def validate_include_exclude(self): + @model_validator(mode="after") + def validate_include_exclude(self) -> "FunctionGroupBaseConfig": if self.include and self.exclude: raise ValueError("include and exclude cannot be used together") return selftests/nat/agent/test_reasoning_agent.py (3)
141-146: Annotate mutable class attributes as ClassVar to silence RUF012.
These are class-level sets; mark as ClassVar.Apply:
+from typing import ClassVar @@ - class FakeDeps: - functions = {"SomeTool"} - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = {"SomeTool"} + function_groups: ClassVar[set[str]] = set()
305-310: Same ClassVar fix here.
Replicate the annotation to avoid linter noise.- class FakeDeps: - functions = {"ToolA", "ToolB"} - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = {"ToolA", "ToolB"} + function_groups: ClassVar[set[str]] = set()
380-385: Same ClassVar fix for the empty-tools case.
Prevents repeated RUF012.- class FakeDeps: - functions = set() - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = set() + function_groups: ClassVar[set[str]] = set()src/nat/builder/function.py (1)
401-406: Nit: compile the regex once.
Micro-optimization; avoids recompiling per call.+NAME_RE = re.compile(r"^[a-zA-Z0-9_-]+$") @@ - if not re.match(r"^[a-zA-Z0-9_-]+$", name): + if NAME_RE.fullmatch(name) is None: raise ValueError(f"Function name can only contain letters, numbers, underscores, and hyphens: {name}")(Add NAME_RE near the imports.)
src/nat/builder/workflow_builder.py (2)
396-404: Seed group dependencies under the instance key to avoid stray entries.
Currently initializes under config.type then stores under name.- self.current_function_group_building = config.type - # Empty set of dependencies for the current function group - self.function_group_dependencies[config.type] = FunctionDependencies() + self.current_function_group_building = config.type + # Seed dependencies under the instance key + self.function_group_dependencies[name] = FunctionDependencies() @@ - self.function_group_dependencies[name] = inner_builder.dependencies + self.function_group_dependencies[name] = inner_builder.dependencies
561-577: Add return type for get_tool (nit) and preserve traceback.
Return annotation improves parity with abstract API.- def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str): + def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any: ... - except Exception as e: - logger.error("Error fetching tool `%s`: %s", fn_name, e) - raise + except Exception as e: + logger.error("Error fetching tool `%s`: %s", fn_name, e) + raisetests/nat/builder/test_builder.py (2)
286-419: Fix Ruff TRY003/ARG001 in function-group registrations.
- TRY003: these raises are intentional for tests; silence with inline noqa.
- ARG001: rename unused parameter in register_failing_function_group.
Apply:
@@ - if config.raise_error: - raise ValueError("Function group initialization failed") + if config.raise_error: + raise ValueError("Function group initialization failed") # noqa: TRY003 @@ - if config.raise_error: - raise ValueError("Function group initialization failed") + if config.raise_error: + raise ValueError("Function group initialization failed") # noqa: TRY003 @@ - if config.raise_error: - raise ValueError("Function group initialization failed") + if config.raise_error: + raise ValueError("Function group initialization failed") # noqa: TRY003 @@ - if config.raise_error: - raise ValueError("Function group initialization failed") + if config.raise_error: + raise ValueError("Function group initialization failed") # noqa: TRY003 @@ - async def register_failing_function_group(config: FailingFunctionGroupConfig, _builder: Builder): + async def register_failing_function_group(_config: FailingFunctionGroupConfig, _builder: Builder): @@ - raise ValueError("Function group initialization failed") + raise ValueError("Function group initialization failed") # noqa: TRY003
1054-1065: Prefix check should use config type, not instance name.Function keys are namespaced by config.type (default_function_group), not the builder-assigned name (empty_group). While it still passes (zero functions), it’s clearer to assert on the actual prefix used by FunctionGroup.
- included_functions = [k for k in builder._functions.keys() if k.startswith("empty_group.")] + included_functions = [k for k in builder._functions.keys() if k.startswith("default_function_group.")]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/source/extend/function-groups.md(1 hunks)docs/source/workflows/function-groups.md(1 hunks)examples/object_store/user_report/README.md(8 hunks)examples/object_store/user_report/configs/config_redis.yml(2 hunks)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py(5 hunks)src/nat/agent/reasoning_agent/reasoning_agent.py(1 hunks)src/nat/builder/function.py(3 hunks)src/nat/builder/workflow_builder.py(17 hunks)src/nat/cli/register_workflow.py(4 hunks)src/nat/data_models/config.py(10 hunks)src/nat/data_models/function.py(2 hunks)src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py(1 hunks)tests/nat/agent/test_reasoning_agent.py(3 hunks)tests/nat/builder/test_builder.py(16 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/extend/function-groups.md
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/object_store/user_report/README.md
- src/nat/cli/register_workflow.py
- src/nat/agent/reasoning_agent/reasoning_agent.py
- src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py
- docs/source/workflows/function-groups.md
🧰 Additional context used
📓 Path-based instructions (12)
**/configs/**/*.y?(a)ml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must live in a neighbouring configs/ directory
Files:
examples/object_store/user_report/configs/config_redis.yml
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
examples/object_store/user_report/configs/config_redis.ymlsrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/function.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
examples/object_store/user_report/configs/config_redis.ymlsrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/function.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/object_store/user_report/configs/config_redis.yml
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Core functionality under src/nat should prioritize backward compatibility when changed
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/function.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/function.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
🧠 Learnings (8)
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are sibling classes that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other and the order doesn't matter for classification purposes.
Applied to files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are separate class hierarchies that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other.
Applied to files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:30:12.178Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/data_models/function.py:39-45
Timestamp: 2025-08-28T14:30:12.178Z
Learning: In the NAT codebase, stricter validation of function names (like checking for empty/whitespace-only strings) in FunctionGroupBaseConfig.validate_expose is unnecessary because workflow parsing and validation already handle these validation concerns at a different layer in the system.
Applied to files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.
Applied to files:
src/nat/builder/workflow_builder.pytests/nat/builder/test_builder.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:29:34.897Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/workflow_builder.py:373-410
Timestamp: 2025-08-28T14:29:34.897Z
Learning: ComponentRef objects (like FunctionGroupRef) automatically convert to strings when used as dictionary keys in Python, so explicit str() conversions are not required when using them as dict keys.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T23:22:41.742Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-28T23:22:41.742Z
Learning: Applies to **/*.py : When re-raising exceptions, use bare `raise` and log with logger.error(), not logger.exception()
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.
Applied to files:
tests/nat/builder/test_builder.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
tests/nat/builder/test_builder.pysrc/nat/builder/function.py
🧬 Code graph analysis (7)
src/nat/data_models/config.py (2)
src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)src/nat/cli/type_registry.py (3)
get(1049-1050)get_registered_function_groups(535-541)compute_annotation(997-1041)
src/nat/builder/workflow_builder.py (8)
src/nat/builder/function.py (5)
FunctionGroup(350-526)add_function(371-411)Function(45-300)get_included_functions(488-512)get_accessible_functions(441-463)src/nat/data_models/component_ref.py (12)
FunctionGroupRef(105-113)FunctionRef(94-102)component_group(69-76)component_group(90-91)component_group(101-102)component_group(112-113)component_group(123-124)component_group(134-135)component_group(145-146)component_group(156-157)component_group(167-168)component_group(178-179)src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)src/nat/data_models/function_dependencies.py (5)
FunctionDependencies(21-80)add_function(61-62)add_function_group(64-65)add_object_store(76-77)add_retriever(79-80)src/nat/data_models/config.py (1)
Config(238-423)src/nat/builder/workflow.py (2)
Workflow(41-160)from_entry_fn(128-160)src/nat/builder/builder.py (11)
get_function_group(79-80)add_function(67-68)add_function_group(71-72)get_function(75-76)get_function_config(90-91)get_function_group_config(94-95)get_function_group_dependencies(280-281)get_tools(110-113)get_tool(116-117)add_object_store(157-158)add_retriever(217-218)src/nat/profiler/utils.py (1)
detect_llm_frameworks_in_build_fn(36-144)
tests/nat/builder/test_builder.py (5)
src/nat/builder/function.py (8)
Function(45-300)FunctionGroup(350-526)add_function(371-411)get_accessible_functions(441-463)get_all_functions(514-526)get_included_functions(488-512)get_excluded_functions(465-486)get_config(413-422)src/nat/cli/register_workflow.py (2)
register_function_group(180-211)register_function(148-177)src/nat/cli/type_registry.py (4)
register_function_group(499-515)register_function(477-485)get_function_group(517-533)get_function(487-493)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (11)
add_function(412-423)add_function(1111-1112)add_function_group(426-448)add_function_group(1115-1116)get_function_group(460-466)get_function_group(1128-1134)get_function_group_config(478-484)get_function_group_config(1141-1142)get_function(451-457)get_function(1119-1125)dependencies(1107-1108)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (5)
src/nat/builder/builder.py (6)
Builder(64-281)add_function_group(71-72)get_function_group(79-80)get_function_group_config(94-95)get_tools(110-113)get_function_group_dependencies(280-281)src/nat/builder/function.py (2)
Function(45-300)FunctionGroup(350-526)src/nat/data_models/function.py (2)
FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (10)
add_function_group(426-448)add_function_group(1115-1116)get_function_group(460-466)get_function_group(1128-1134)get_function_group_config(478-484)get_function_group_config(1141-1142)get_tools(526-559)get_tools(1157-1166)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)src/nat/data_models/function_dependencies.py (2)
add_function_group(64-65)FunctionDependencies(21-80)
src/nat/builder/function.py (3)
src/nat/data_models/function.py (3)
EmptyFunctionConfig(58-59)FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/context.py (1)
Context(93-277)src/nat/builder/workflow_builder.py (2)
add_function(412-423)add_function(1111-1112)
src/nat/data_models/function.py (1)
src/nat/data_models/common.py (2)
TypedBaseModel(93-168)BaseModelRegistryTag(88-90)
tests/nat/agent/test_reasoning_agent.py (2)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (2)
get_function_dependencies(284-286)get_function_group_dependencies(288-290)src/nat/builder/workflow_builder.py (4)
get_function_dependencies(514-517)get_function_dependencies(1296-1297)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)
🪛 Ruff (0.12.2)
src/nat/builder/workflow_builder.py
406-407: Prefer TypeError exception for invalid type
(TRY004)
406-407: Avoid specifying long messages outside the exception class
(TRY003)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
431-431: Avoid specifying long messages outside the exception class
(TRY003)
442-442: Avoid specifying long messages outside the exception class
(TRY003)
455-455: Avoid specifying long messages outside the exception class
(TRY003)
464-464: Avoid specifying long messages outside the exception class
(TRY003)
473-473: Avoid specifying long messages outside the exception class
(TRY003)
482-482: Avoid specifying long messages outside the exception class
(TRY003)
536-536: Avoid specifying long messages outside the exception class
(TRY003)
541-541: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/builder/test_builder.py
292-292: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
413-413: Unused function argument: config
(ARG001)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
157-157: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Unused method argument: name
(ARG002)
179-179: Unused method argument: tool_names
(ARG002)
179-179: Unused method argument: wrapper_type
(ARG002)
288-288: Unused method argument: fn_name
(ARG002)
src/nat/builder/function.py
402-402: Avoid specifying long messages outside the exception class
(TRY003)
404-404: Avoid specifying long messages outside the exception class
(TRY003)
406-406: Avoid specifying long messages outside the exception class
(TRY003)
432-433: Avoid specifying long messages outside the exception class
(TRY003)
481-482: Avoid specifying long messages outside the exception class
(TRY003)
506-508: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/data_models/function.py
48-48: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/agent/test_reasoning_agent.py
141-141: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
142-142: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
305-305: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
380-380: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
381-381: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (31)
examples/object_store/user_report/configs/config_redis.yml (2)
70-70: Verify support for function groups intool_names
I didn’t find any code or docs referencingtool_namesin group resolution—please manually confirm that WorkflowBuilder and tool resolution correctly handle group names (or add explicit include/exclude settings if you only need a subset).
37-40: No changes needed – include/exclude fields are already provided for scoping, and the config type “user_report” with its object_store field is correctly defined.src/nat/data_models/function.py (5)
18-21: Pydantic v2 imports look correct.
Using Field/field_validator/model_validator is appropriate here.
30-34: Clear public docstring for the new base config.
Good, concise description.
35-42: Good defaults for include/exclude lists.
default_factory=list avoids shared state; field descriptions are clear.
44-50: Uniqueness validator LGTM.
Covers both fields via multi-field validator.
64-65: TypeVar export looks good.
Useful for downstream typing.packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (2)
63-66: Mock registration for function groups: LGTM.
Simple and consistent with other mocks.
288-291: Stub for group dependencies is fine.
Return empty FunctionDependencies.src/nat/data_models/config.py (4)
141-143: Good: dict fields use default_factory.
Prevents shared mutable defaults; compliant with ruff.
246-265: Function and Function Group fields added with safe defaults.
Looks correct and backward compatible.
287-288: Nice: summary includes Function Groups.
Small UX improvement.
339-342: FunctionGroupsAnnotation build is correct.
Keeps annotations in sync with registry.src/nat/builder/function.py (6)
17-17: Import of re is appropriate for name validation.
349-413: FunctionGroup core structure LGTM.
Clear API, typed, and documented; instance-name namespacing is good.
427-439: Exclude handling preserves order and qualifies names.
Good validation and mapping.
480-486: Excluded functions API LGTM.
Validates unknown names; returns namespaced keys.
505-513: Included functions API LGTM.
Deterministic order from include list; good error text.
523-526: get_all_functions returns fully-qualified keys.
Consistent addressing.src/nat/builder/workflow_builder.py (7)
93-97: ConfiguredFunctionGroup dataclass: LGTM.
Keeps config/instance paired like other components.
158-160: Tracking _function_groups in builder is correct.
Symmetric with _functions.
176-179: Dependency tracking fields added: LGTM.
Enables per-group dependency surfacing.
284-288: Mirror fix for instances set (relies on exposed_functions).
After the above change, this block is correct.Ensure no duplication of group-exposed functions in function_instances.
526-559: get_tools(): normalization and de-dup logic LGTM.
Handles FunctionRef/FunctionGroupRef correctly before lookup.
1054-1056: populate_builder: function group addition wiring LGTM.
Matches new ComponentGroup.FUNCTION_GROUPS.
243-249: Replace non-existentconfig.expose
FunctionGroupBaseConfigonly definesinclude/exclude, sov.config.exposewill raise an AttributeError. Compute exposed functions via each group’sget_included_functions()instead:- # Get all functions which are exposed by function groups - exposed_functions = {f"{k}.{n}" for k, v in self._function_groups.items() for n in v.config.expose} + # Get all functions exposed (included) by function groups + exposed_functions: set[str] = set() + for group in self._function_groups.values(): + exposed_functions.update(group.instance.get_included_functions().keys())Ensure
get_included_functions()exists on the function-group instance and correctly appliesinclude/exclude.⛔ Skipped due to learnings
Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.tests/nat/builder/test_builder.py (5)
19-23: Correct Pydantic imports added.Good fix replacing/adding BaseModel and Field from pydantic; aligns with prior issues.
50-51: FunctionGroupBaseConfig import is appropriate.Brings tests in sync with the new Function Group surface.
114-148: Test configs use Field(default_factory=...) correctly.Nice job avoiding mutable defaults and removing unsupported frozen args.
1329-1344: Custom instance name behavior validated well.Good coverage for namespacing override via instance_name.
783-839: Acknowledge async signature aligns with implementation
Tests correctly awaitget_retriever_config, which is declaredasync, whileget_object_store_configandget_llm_configremain synchronous by design—no test changes required.
willkill07
force-pushed
the
wkk_function-groups
branch
from
164e4ec to
06fa121
Compare
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 (2)
src/nat/builder/workflow_builder.py (2)
561-577: Add return type; include exc_info to aid debugging while preserving traceback.Public API must have return annotations per repo guidelines.
- def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str): + def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any: @@ - except Exception as e: - logger.error("Error fetching tool `%s`: %s", fn_name, e) - raise + except Exception as e: + logger.error("Error fetching tool `%s`: %s", fn_name, e, exc_info=True) + raise
1168-1175: Annotate return type and normalize dependency key.Ensures API typing compliance and consistent dependency tracking.
- def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str): + def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any: # If a function tries to get another function as a tool, we assume it uses it fn = self._workflow_builder.get_tool(fn_name, wrapper_type) - self._dependencies.add_function(fn_name) + self._dependencies.add_function(str(fn_name)) return fn
♻️ Duplicate comments (9)
examples/object_store/user_report/configs/config_mem.yml (1)
70-70: Add test to verify tool_names group expansion and ordering.Please ensure a test asserts that tool_names: [user_report] expands to the expected ordered, namespaced tools.
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (5)
159-162: Match config getter signature and silence ARG002.Accept FunctionGroupRef and underscore the unused param.
- def get_function_group_config(self, name: str) -> FunctionGroupBaseConfig: + def get_function_group_config(self, _name: str | FunctionGroupRef) -> FunctionGroupBaseConfig: """Mock implementation.""" return FunctionGroupBaseConfig()
20-20: Import missing types to align with Builder API.Bring in the type aliases used by abstract signatures to avoid pyright/ruff drift.
-from collections.abc import Sequence +from collections.abc import Sequence +from nat.builder.framework_enum import LLMFrameworkEnum +from nat.data_models.component_ref import FunctionRef, FunctionGroupRef
147-150: Align add_function_group signature with Builder.Accept FunctionGroupRef in addition to str.
- async def add_function_group(self, name: str, config: FunctionGroupBaseConfig) -> FunctionGroup: + async def add_function_group(self, name: str | FunctionGroupRef, config: FunctionGroupBaseConfig) -> FunctionGroup:
151-158: Widen get_function_group signature and return a spec’ed mock.Prevents interface drift; safer attribute access.
- def get_function_group(self, name: str) -> FunctionGroup: + def get_function_group(self, name: str | FunctionGroupRef) -> FunctionGroup: """Return a mock function group if one is configured.""" - if name in self._mocks: - mock_fn_group = MagicMock(spec=FunctionGroup) - mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[name]) + key = str(name) + if key in self._mocks: + mock_fn_group = MagicMock(spec=FunctionGroup) + mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[key]) return mock_fn_group raise ValueError(f"Function group '{name}' not mocked. Use mock_function_group() to add it.")
179-182: Widen get_tools signature; type wrapper; silence unused arg warnings.Matches Builder and fixes ARG002.
- def get_tools(self, tool_names: Sequence[str], wrapper_type): + def get_tools( + self, + tool_names: Sequence[str | FunctionRef | FunctionGroupRef], + wrapper_type: LLMFrameworkEnum | str, + ) -> list[typing.Any]: """Mock implementation.""" - return [] + _ = (tool_names, wrapper_type) + return []examples/object_store/user_report/configs/config_mysql.yml (1)
74-74: Test group expansion and order.Add/ensure a test validates expansion of [user_report] to the expected ordered tool list.
examples/object_store/user_report/configs/config_redis.yml (1)
73-73: Test group expansion and order.Same as other envs—please cover [user_report] expansion in tests.
src/nat/builder/workflow_builder.py (1)
1157-1167: Normalize keys when recording dependencies forget_toolsin ChildBuilder.
tool_namemay be aFunctionRef/FunctionGroupRef; membership and recording should use normalized keys. (Reiterating prior feedback.)- tools = self._workflow_builder.get_tools(tool_names, wrapper_type) - for tool_name in tool_names: - if tool_name in self._workflow_builder._function_groups: - self._dependencies.add_function_group(tool_name) - else: - self._dependencies.add_function(tool_name) + tools = self._workflow_builder.get_tools(tool_names, wrapper_type) + for tool_name in tool_names: + key = str(tool_name) + if key in self._workflow_builder._function_groups: + self._dependencies.add_function_group(key) + else: + self._dependencies.add_function(key)
🧹 Nitpick comments (13)
src/nat/data_models/function.py (1)
44-55: Minor: tighten lint noise (TRY003) message wording.Ruff flags long exception messages; your messages are already short, but to silence TRY003 consistently, prefer concise phrasing.
Apply:
- raise ValueError("Function names must be unique") + raise ValueError("Function names must be unique.")- raise ValueError("include and exclude cannot be used together") + raise ValueError("`include` and `exclude` are mutually exclusive.")tests/nat/agent/test_reasoning_agent.py (3)
141-143: Annotate mutable class attributes as ClassVar to satisfy Ruff (RUF012).Declare these sets as ClassVar or move them to init.
Apply:
- class FakeDeps: - functions = {"SomeTool"} - function_groups = set() + class FakeDeps: + functions: typing.ClassVar[set[str]] = {"SomeTool"} + function_groups: typing.ClassVar[set[str]] = set()Also add at file top:
from typing import ClassVar # or use typing.ClassVar in annotations
305-307: Repeat: mark sets as ClassVar.Same fix as above for this FakeDeps definition.
- class FakeDeps: - functions = {"ToolA", "ToolB"} - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = {"ToolA", "ToolB"} + function_groups: ClassVar[set[str]] = set()
380-385: Repeat: mark sets as ClassVar.Same change here to avoid RUF012.
- class FakeDeps: - functions = set() - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = set() + function_groups: ClassVar[set[str]] = set()Outside this hunk, add:
from typing import ClassVarpackages/nvidia_nat_test/src/nat/test/tool_test_runner.py (1)
288-291: Silence ARG002 for unused parameter.Prefix with underscore in this mock.
- def get_function_group_dependencies(self, fn_name: str) -> FunctionDependencies: + def get_function_group_dependencies(self, _fn_name: str) -> FunctionDependencies: """Mock implementation.""" return FunctionDependencies()src/nat/builder/function.py (3)
431-439: Shorten ValueError to satisfy Ruff TRY003.Current message spans two sentences; condense to list only the unknown names.
- if set(self._config.exclude) - set(self._functions.keys()): - raise ValueError(f"Function group {self._instance_name} excludes functions that are not found in the group." - f" Available functions: {list(self._functions.keys())}") + missing = set(self._config.exclude) - set(self._functions.keys()) + if missing: + raise ValueError(f"Unknown excludes: {sorted(missing)}")
480-486: Same: condense error text for excludes.Mirror the shorter style here.
- if set(self._config.exclude) - set(self._functions.keys()): - raise ValueError(f"Function group {self._instance_name} excludes functions that are not found in the group." - f" Available functions: {list(self._functions.keys())}") + missing = set(self._config.exclude) - set(self._functions.keys()) + if missing: + raise ValueError(f"Unknown excludes: {sorted(missing)}")
505-509: Same: condense error text for includes.Short message reduces TRY003 noise.
- if set(self._config.include) - set(self._functions.keys()): - raise ValueError( - f"Function group {self._instance_name} includes functions that are not found in the group. " - f"Available functions: {list(self._functions.keys())}") + missing = set(self._config.include) - set(self._functions.keys()) + if missing: + raise ValueError(f"Unknown includes: {sorted(missing)}")src/nat/builder/workflow_builder.py (5)
373-410: Seed group dependencies under the instance key to avoid stray entries.You initialize at
config.typethen store atname, leaving an extra key.- self.current_function_group_building = config.type - # Empty set of dependencies for the current function group - self.function_group_dependencies[config.type] = FunctionDependencies() + self.current_function_group_building = config.type + # Empty set of dependencies for this function group (by instance key) + self.function_group_dependencies[name] = FunctionDependencies() @@ - self.function_group_dependencies[name] = inner_builder.dependencies + self.function_group_dependencies[name] = inner_builder.dependencies
405-407: Exception type: prefer TypeError for wrong return type.Matches Ruff TRY004 and intent.
- raise ValueError("Expected a FunctionGroup object to be returned from the function group builder. " + raise TypeError("Expected a FunctionGroup object to be returned from the function group builder. " f"Got {type(build_result)}")
438-446: Avoid double calls toget_included_functions()and guard once.Saves a dict build and keeps logic in one place.
- for k in build_result.instance.get_included_functions(): + included = build_result.instance.get_included_functions() + for k in included: if k in self._functions: raise ValueError(f"Exposed function `{k}` from group `{name}` conflicts with an existing function") - self._functions.update({ - k: ConfiguredFunction(config=v.config, instance=v) - for k, v in build_result.instance.get_included_functions().items() - }) + self._functions.update({k: ConfiguredFunction(config=v.config, instance=v) for k, v in included.items()})
525-559: Deduplicate emitted tools when a group and its member(s) are both requested.Currently the same fn can be returned twice (once via group, once via direct name).
- tools = [] + tools = [] seen = set() + emitted: set[str] = set() @@ - if n not in self._function_groups: + if n not in self._function_groups: # the passed tool name is probably a function if is_function_group_ref: raise ValueError(f"Function group `{n}` not found in the list of function groups") - tools.append(self.get_tool(n, wrapper_type)) + if n not in emitted: + tools.append(self.get_tool(n, wrapper_type)) + emitted.add(n) continue @@ - for fn_name, fn_instance in current_function_group.instance.get_accessible_functions().items(): + for fn_name, fn_instance in current_function_group.instance.get_accessible_functions().items(): try: # Wrap in the correct wrapper and add to tools list - tools.append(tool_wrapper_reg.build_fn(fn_name, fn_instance, self)) + if fn_name in emitted: + continue + tools.append(tool_wrapper_reg.build_fn(fn_name, fn_instance, self)) + emitted.add(fn_name) except Exception: logger.error("Error fetching tool `%s`", fn_name, exc_info=True) raise
1048-1048: Grammar nit: “an object store.”Tiny readability fix.
- # Instantiate a object store client + # Instantiate an object store client
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/source/extend/function-groups.md(1 hunks)docs/source/workflows/function-groups.md(1 hunks)examples/object_store/user_report/README.md(8 hunks)examples/object_store/user_report/configs/config_mem.yml(1 hunks)examples/object_store/user_report/configs/config_mysql.yml(1 hunks)examples/object_store/user_report/configs/config_redis.yml(1 hunks)examples/object_store/user_report/configs/config_s3.yml(1 hunks)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py(5 hunks)src/nat/agent/reasoning_agent/reasoning_agent.py(1 hunks)src/nat/builder/function.py(3 hunks)src/nat/builder/workflow_builder.py(17 hunks)src/nat/cli/register_workflow.py(4 hunks)src/nat/data_models/config.py(10 hunks)src/nat/data_models/function.py(2 hunks)src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py(1 hunks)tests/nat/agent/test_reasoning_agent.py(3 hunks)tests/nat/builder/test_builder.py(16 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/workflows/function-groups.md
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/object_store/user_report/configs/config_s3.yml
- src/nat/agent/reasoning_agent/reasoning_agent.py
- src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py
- src/nat/cli/register_workflow.py
- examples/object_store/user_report/README.md
- docs/source/extend/function-groups.md
🧰 Additional context used
📓 Path-based instructions (12)
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks
Files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Core functionality under src/nat should prioritize backward compatibility when changed
Files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
Files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pysrc/nat/builder/function.pytests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pyexamples/object_store/user_report/configs/config_mem.ymlsrc/nat/builder/function.pyexamples/object_store/user_report/configs/config_redis.ymlexamples/object_store/user_report/configs/config_mysql.ymltests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pysrc/nat/builder/function.pytests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pyexamples/object_store/user_report/configs/config_mem.ymlsrc/nat/builder/function.pyexamples/object_store/user_report/configs/config_redis.ymlexamples/object_store/user_report/configs/config_mysql.ymltests/nat/builder/test_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/configs/**/*.y?(a)ml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must live in a neighbouring configs/ directory
Files:
examples/object_store/user_report/configs/config_mem.ymlexamples/object_store/user_report/configs/config_redis.ymlexamples/object_store/user_report/configs/config_mysql.yml
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/object_store/user_report/configs/config_mem.ymlexamples/object_store/user_report/configs/config_redis.ymlexamples/object_store/user_report/configs/config_mysql.yml
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
🧠 Learnings (8)
📚 Learning: 2025-08-28T14:30:12.178Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/data_models/function.py:39-45
Timestamp: 2025-08-28T14:30:12.178Z
Learning: In the NAT codebase, stricter validation of function names (like checking for empty/whitespace-only strings) in FunctionGroupBaseConfig.validate_expose is unnecessary because workflow parsing and validation already handle these validation concerns at a different layer in the system.
Applied to files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pysrc/nat/builder/function.pyexamples/object_store/user_report/configs/config_mysql.ymltests/nat/builder/test_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.
Applied to files:
src/nat/builder/workflow_builder.pysrc/nat/builder/function.pyexamples/object_store/user_report/configs/config_mysql.ymltests/nat/builder/test_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:29:34.897Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/workflow_builder.py:373-410
Timestamp: 2025-08-28T14:29:34.897Z
Learning: ComponentRef objects (like FunctionGroupRef) automatically convert to strings when used as dictionary keys in Python, so explicit str() conversions are not required when using them as dict keys.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T23:22:41.742Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-28T23:22:41.742Z
Learning: Applies to **/*.py : When re-raising exceptions, use bare `raise` and log with logger.error(), not logger.exception()
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are sibling classes that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other and the order doesn't matter for classification purposes.
Applied to files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pytests/nat/builder/test_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are separate class hierarchies that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other.
Applied to files:
src/nat/builder/workflow_builder.pysrc/nat/data_models/config.pytests/nat/builder/test_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
src/nat/builder/function.pytests/nat/builder/test_builder.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.
Applied to files:
src/nat/builder/function.pyexamples/object_store/user_report/configs/config_mysql.ymltests/nat/builder/test_builder.pysrc/nat/data_models/function.py
🧬 Code graph analysis (7)
src/nat/builder/workflow_builder.py (8)
src/nat/builder/function.py (5)
FunctionGroup(350-526)add_function(371-411)Function(45-300)get_included_functions(488-512)get_accessible_functions(441-463)src/nat/data_models/component_ref.py (12)
FunctionGroupRef(105-113)FunctionRef(94-102)component_group(69-76)component_group(90-91)component_group(101-102)component_group(112-113)component_group(123-124)component_group(134-135)component_group(145-146)component_group(156-157)component_group(167-168)component_group(178-179)src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)src/nat/data_models/function_dependencies.py (5)
FunctionDependencies(21-80)add_function(61-62)add_function_group(64-65)add_object_store(76-77)add_retriever(79-80)src/nat/data_models/config.py (1)
Config(238-423)src/nat/builder/workflow.py (2)
Workflow(41-160)from_entry_fn(128-160)src/nat/builder/builder.py (11)
get_function_group(79-80)add_function(67-68)add_function_group(71-72)get_function(75-76)get_function_config(90-91)get_function_group_config(94-95)get_function_group_dependencies(280-281)get_tools(110-113)get_tool(116-117)add_object_store(157-158)add_retriever(217-218)src/nat/profiler/utils.py (1)
detect_llm_frameworks_in_build_fn(36-144)
src/nat/data_models/config.py (2)
src/nat/data_models/function.py (3)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)EmptyFunctionConfig(58-59)src/nat/cli/type_registry.py (4)
GlobalTypeRegistry(1044-1065)get(1049-1050)get_registered_function_groups(535-541)compute_annotation(997-1041)
src/nat/builder/function.py (2)
src/nat/data_models/function.py (3)
EmptyFunctionConfig(58-59)FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/context.py (1)
Context(93-277)
tests/nat/builder/test_builder.py (5)
src/nat/builder/function.py (8)
Function(45-300)FunctionGroup(350-526)add_function(371-411)get_accessible_functions(441-463)get_all_functions(514-526)get_included_functions(488-512)get_excluded_functions(465-486)get_config(413-422)src/nat/cli/register_workflow.py (2)
register_function_group(180-211)register_function(148-177)src/nat/cli/type_registry.py (4)
register_function_group(499-515)register_function(477-485)get_function_group(517-533)get_function(487-493)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (11)
add_function(412-423)add_function(1111-1112)add_function_group(426-448)add_function_group(1115-1116)get_function_group(460-466)get_function_group(1128-1134)get_function_group_config(478-484)get_function_group_config(1141-1142)get_function(451-457)get_function(1119-1125)dependencies(1107-1108)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (6)
src/nat/builder/builder.py (6)
Builder(64-281)add_function_group(71-72)get_function_group(79-80)get_function_group_config(94-95)get_tools(110-113)get_function_group_dependencies(280-281)src/nat/builder/function.py (2)
Function(45-300)FunctionGroup(350-526)src/nat/cli/type_registry.py (2)
GlobalTypeRegistry(1044-1065)get_function_group(517-533)src/nat/data_models/function.py (2)
FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (10)
add_function_group(426-448)add_function_group(1115-1116)get_function_group(460-466)get_function_group(1128-1134)get_function_group_config(478-484)get_function_group_config(1141-1142)get_tools(526-559)get_tools(1157-1166)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)src/nat/data_models/function_dependencies.py (2)
add_function_group(64-65)FunctionDependencies(21-80)
src/nat/data_models/function.py (1)
src/nat/data_models/common.py (2)
TypedBaseModel(93-168)BaseModelRegistryTag(88-90)
tests/nat/agent/test_reasoning_agent.py (2)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (2)
get_function_dependencies(284-286)get_function_group_dependencies(288-290)src/nat/builder/workflow_builder.py (4)
get_function_dependencies(514-517)get_function_dependencies(1296-1297)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)
🪛 Ruff (0.12.2)
src/nat/builder/workflow_builder.py
406-407: Prefer TypeError exception for invalid type
(TRY004)
406-407: Avoid specifying long messages outside the exception class
(TRY003)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
431-431: Avoid specifying long messages outside the exception class
(TRY003)
442-442: Avoid specifying long messages outside the exception class
(TRY003)
455-455: Avoid specifying long messages outside the exception class
(TRY003)
464-464: Avoid specifying long messages outside the exception class
(TRY003)
473-473: Avoid specifying long messages outside the exception class
(TRY003)
482-482: Avoid specifying long messages outside the exception class
(TRY003)
536-536: Avoid specifying long messages outside the exception class
(TRY003)
541-541: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/builder/function.py
402-402: Avoid specifying long messages outside the exception class
(TRY003)
404-404: Avoid specifying long messages outside the exception class
(TRY003)
406-406: Avoid specifying long messages outside the exception class
(TRY003)
432-433: Avoid specifying long messages outside the exception class
(TRY003)
481-482: Avoid specifying long messages outside the exception class
(TRY003)
506-508: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/builder/test_builder.py
292-292: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
413-413: Unused function argument: config
(ARG001)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
157-157: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Unused method argument: name
(ARG002)
179-179: Unused method argument: tool_names
(ARG002)
179-179: Unused method argument: wrapper_type
(ARG002)
288-288: Unused method argument: fn_name
(ARG002)
src/nat/data_models/function.py
48-48: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/agent/test_reasoning_agent.py
141-141: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
142-142: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
305-305: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
380-380: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
381-381: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (22)
src/nat/data_models/function.py (1)
30-42: Solid base config and typing.Includes/excludes with validators and Google-style docstring look good. Default factories avoid shared state.
examples/object_store/user_report/configs/config_mem.yml (1)
34-47: Config consolidation to a Function Group looks good.Group wiring and workflow reference are consistent with the new model.
Also applies to: 51-63, 68-75
examples/object_store/user_report/configs/config_mysql.yml (1)
38-71: MySQL config matches the Function Group pattern.Looks consistent with mem/redis variants and workflow wiring.
Also applies to: 72-79
examples/object_store/user_report/configs/config_redis.yml (1)
37-70: Redis config consolidation LGTM.Group definitions and workflow reference are consistent.
Also applies to: 71-78
src/nat/data_models/config.py (5)
23-24: Good: using Field(default_factory=...).Avoids mutable defaults across dict-like sections.
62-66: Helpful error routing for function_groups.Adding the function_groups branch in _process_validation_error improves messages.
339-342: Annotation rebuilding covers function_groups.FunctionGroupsAnnotation and field hook look correct.
Also applies to: 384-388
359-361: Confirm workflow cannot be a FunctionGroup; update suggestions if needed.If workflow is ever allowed to be a FunctionGroup, WorkflowAnnotation should union-in FunctionGroupBaseConfig, and suggestions for 'workflow' should include both functions and groups. If not, current typing is fine.
286-288: Nice touch: summary includes Function Groups.Printout addition is useful.
src/nat/builder/function.py (3)
350-370: Well-scoped FunctionGroup with namespacing.Constructor and internal mapping are clear; namespacing via instance_name is consistent.
371-412: Name validation and duplicate protection: good baseline.Rejecting blanks, dots, and duplicates is appropriate; wrapping via LambdaFunction is clean.
514-526: Deterministic ordering preserved.Iterating the ordered mapping to build fully-qualified keys is correct.
tests/nat/builder/test_builder.py (7)
20-22: LGTM!The import updates correctly add the necessary dependencies for function group testing - BaseModel, Field, FunctionGroup, register_function_group, and FunctionGroupBaseConfig.
Also applies to: 27-27, 36-36, 50-50
114-148: LGTM!The function group test configurations are well-designed and comprehensive:
- Uses Field(default_factory=...) to avoid mutable default issues
- Covers different scenarios: includes, excludes, empty, all functions
- Includes a failing configuration for error testing
- Good docstrings explaining each config's purpose
175-175: LGTM!The parameter names have been correctly updated to
_builderto follow the established naming convention in the codebase and avoid shadowing concerns.Also applies to: 234-234, 242-242, 250-250, 263-263, 275-275, 278-278
286-418: LGTM!The function group registration implementations are excellent:
- Comprehensive coverage of different config types (includes/excludes/all/failing)
- Proper error handling with configurable failure modes
- Consistent function implementations across registrations
- Good use of descriptive function documentation
- Proper yield pattern for async generators
The test functions (add, multiply, subtract) provide good coverage for testing inclusion/exclusion scenarios.
443-443: LGTM!The
type: ignoreannotations are appropriate for handling decorator type checking issues that don't affect runtime behavior.Also applies to: 497-497
783-783: LGTM!The function name updates correctly reflect the new async nature of retriever methods and the use of default GeneralConfig.
Also applies to: 818-818, 828-828, 836-836, 839-839, 893-893
936-1344: LGTM!The function group test suite is comprehensive and well-structured:
Excellent test coverage:
- Basic CRUD operations (add, get, get_config)
- Inclusion/exclusion behavior validation
- Edge cases (empty groups, all functions)
- Error scenarios (name conflicts, invalid configs)
- Integration with workflow builder
- Function execution testing
- Custom instance names
Good test practices:
- Clear, descriptive test names and docstrings
- Proper error assertion with specific error messages
- Verification of both positive and negative cases
- Integration testing that validates end-to-end functionality
Thorough validation:
- Tests verify that included functions are accessible globally
- Tests verify that excluded functions are not accessible globally but accessible through the group
- Tests validate dependency tracking
- Tests check configuration validation and error handling
The tests provide excellent coverage of the function group functionality and will catch regressions effectively.
src/nat/builder/workflow_builder.py (3)
93-97: New ConfiguredFunctionGroup dataclass looks correct.Accurate types and placement; aligns with the FunctionGroup design.
158-160: Registries for function groups and dependencies initialized properly.State separation between functions and function_groups is clear.
Also applies to: 176-179
243-249: Ignore removedconfig.exposecomment:exposeremains a valid field on FunctionGroupBaseConfig; no build breakLikely an incorrect or invalid review comment.
willkill07
force-pushed
the
wkk_function-groups
branch
from
06fa121 to
aea7e1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nat/builder/workflow_builder.py (1)
1169-1175: ChildBuilder.get_tool(): record dependency with normalized name.Ensures consistent str keys in FunctionDependencies.
- fn = self._workflow_builder.get_tool(fn_name, wrapper_type) - - self._dependencies.add_function(fn_name) + key = str(fn_name) + fn = self._workflow_builder.get_tool(key, wrapper_type) + self._dependencies.add_function(key)
♻️ Duplicate comments (13)
src/nat/builder/component_utils.py (1)
261-263: Guardtotal_node_countfor backward compatibility.Older configs may lack
function_groups; direct attribute access breaks the builder. Usegetattr(..., {})for all component dicts.Apply:
- total_node_count = (len(config.embedders) + len(config.functions) + len(config.function_groups) + len(config.llms) + - len(config.memory) + len(config.object_stores) + len(config.retrievers) + - len(config.ttc_strategies) + len(config.authentication) + 1) # +1 for the workflow + total_node_count = ( + len(getattr(config, "embedders", {})) + + len(getattr(config, "functions", {})) + + len(getattr(config, "function_groups", {})) + + len(getattr(config, "llms", {})) + + len(getattr(config, "memory", {})) + + len(getattr(config, "object_stores", {})) + + len(getattr(config, "retrievers", {})) + + len(getattr(config, "ttc_strategies", {})) + + len(getattr(config, "authentication", {})) + + 1 # +1 for the workflow + )Optional hardening (outside this hunk): in
config_to_dependency_objects, prefer
component_dict = getattr(config, group.value, {})for consistency.examples/object_store/user_report/configs/config_s3.yml (1)
16-22: Remove duplicate object_store under front_end.Group-level object_store is authoritative; keeping both risks schema validation failure.
general: front_end: _type: fastapi - object_store: report_object_store cors: allow_origins: ['*']packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (5)
20-24: Import missing types to match Builder API.Required for widened signatures below.
from collections.abc import Sequence +from nat.builder.framework_enum import LLMFrameworkEnum +from nat.data_models.component_ref import FunctionRef, FunctionGroupRefAlso applies to: 28-35
147-149: Align add_function_group signature with abstract base.Accept FunctionGroupRef.
- async def add_function_group(self, name: str, config: FunctionGroupBaseConfig) -> FunctionGroup: + async def add_function_group(self, name: str | FunctionGroupRef, config: FunctionGroupBaseConfig) -> FunctionGroup: """Mock implementation - not used in tool testing.""" raise NotImplementedError("Mock implementation does not support add_function_group")
151-158: Align get_function_group signature and use spec’ed mock.Prevents interface drift; safer mocking.
- def get_function_group(self, name: str) -> FunctionGroup: + def get_function_group(self, name: str | FunctionGroupRef) -> FunctionGroup: """Return a mock function group if one is configured.""" - if name in self._mocks: - mock_fn_group = MagicMock(spec=FunctionGroup) - mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[name]) + key = str(name) + if key in self._mocks: + mock_fn_group = MagicMock(spec=FunctionGroup) + mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[key]) return mock_fn_group raise ValueError(f"Function group '{name}' not mocked. Use mock_function_group() to add it.")
159-162: Align get_function_group_config signature with base.- def get_function_group_config(self, name: str) -> FunctionGroupBaseConfig: + def get_function_group_config(self, name: str | FunctionGroupRef) -> FunctionGroupBaseConfig: """Mock implementation.""" return FunctionGroupBaseConfig()
179-182: Widen get_tools signature and type wrapper.Matches Builder.get_tools.
- def get_tools(self, tool_names: Sequence[str], wrapper_type): + def get_tools( + self, + tool_names: Sequence[str | FunctionRef | FunctionGroupRef], + wrapper_type: LLMFrameworkEnum | str, + ) -> list[typing.Any]: """Mock implementation.""" return []examples/object_store/user_report/configs/config_mysql.yml (1)
16-22: Remove duplicate object_store under front_end.Keep only group-level object_store.
general: front_end: _type: fastapi - object_store: report_object_store cors: allow_origins: ['*']examples/object_store/user_report/configs/config_redis.yml (1)
16-22: Remove duplicate object_store under front_end.Same rationale as other configs.
general: front_end: _type: fastapi - object_store: report_object_store cors: allow_origins: ['*']src/nat/data_models/config.py (1)
62-66: ‘workflow’ suggestions should include both functions and function groups.Currently only functions are suggested.
- if (info.field_name in ('workflow', 'functions')): - registered_keys = GlobalTypeRegistry.get().get_registered_functions() + if info.field_name == 'workflow': + registered_keys = ( + list(GlobalTypeRegistry.get().get_registered_functions()) + + list(GlobalTypeRegistry.get().get_registered_function_groups()) + ) + elif info.field_name == 'functions': + registered_keys = GlobalTypeRegistry.get().get_registered_functions() - elif (info.field_name == "function_groups"): + elif info.field_name == "function_groups": registered_keys = GlobalTypeRegistry.get().get_registered_function_groups()src/nat/tool/github_tools.py (2)
195-203: Encode labels as comma-separated string for GitHub issues filter.GitHub expects labels as a CSV string, not a list.
for issue in issues_list.filter_parameters: - params = issue.model_dump(exclude_unset=True, exclude_none=True) + params = issue.model_dump(exclude_unset=True, exclude_none=True) + if isinstance(params.get("labels"), list): + params["labels"] = ",".join(params["labels"]) response = await client.get(url, params=params)
424-429: Fix raw.githubusercontent.com URL (remove refs/heads).Current URL is invalid; include branch in file_path already.
- # The following URL is the raw URL of the file. refs/heads/ always points to the top commit of the branch - raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/refs/heads/{file_metadata.file_path}" + # Build correct raw GitHub URL: owner/repo/{branch}/{path} + raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/{file_metadata.file_path}"src/nat/builder/workflow_builder.py (1)
1157-1166: ChildBuilder.get_tools(): normalize keys before dependency tracking.Without normalization, FunctionGroupRef won’t match builder keys and dependencies get misclassified.
- for tool_name in tool_names: - if tool_name in self._workflow_builder._function_groups: - self._dependencies.add_function_group(tool_name) - else: - self._dependencies.add_function(tool_name) + for tool_name in tool_names: + key = str(tool_name) + if key in self._workflow_builder._function_groups: + self._dependencies.add_function_group(key) + else: + self._dependencies.add_function(key)
🧹 Nitpick comments (11)
src/nat/builder/function.py (1)
17-17: Precompile the name-validation regex once.Avoid recompiling the pattern per call; define it at module scope and reuse.
Add near the imports (outside the changed hunk):
NAME_RE = re.compile(r"^[A-Za-z0-9_-]+\Z")src/nat/data_models/function.py (1)
51-55: Add return type to model-level validator.Minor typing polish per repo rules.
- @model_validator(mode="after") - def validate_include_exclude(self): + @model_validator(mode="after") + def validate_include_exclude(self) -> "FunctionGroupBaseConfig": if self.include and self.exclude: raise ValueError("include and exclude cannot be used together") return selfpackages/nvidia_nat_test/src/nat/test/tool_test_runner.py (1)
135-142: Return a spec’ed Function mock for consistency.Current AsyncMock lacks spec; prefer spec to catch misuse.
- if name in self._mocks: - mock_fn = AsyncMock() - mock_fn.ainvoke = AsyncMock(return_value=self._mocks[name]) - return mock_fn + if name in self._mocks: + mock_fn = MagicMock(spec=Function) + mock_fn.ainvoke = AsyncMock(return_value=self._mocks[name]) + return mock_fnsrc/nat/builder/builder.py (1)
279-282: Consider widening dependency lookup to accept FunctionGroupRef.Improves parity with other APIs and WorkflowBuilder.
- def get_function_group_dependencies(self, fn_name: str) -> FunctionDependencies: + def get_function_group_dependencies(self, fn_name: str | FunctionGroupRef) -> FunctionDependencies: passsrc/nat/tool/github_tools.py (3)
437-447: Validate start/end ordering for line ranges.Handle start > end explicitly.
- selected_lines = lines[file_metadata.start_line - 1:file_metadata.end_line] + if file_metadata.start_line > file_metadata.end_line: + return (f"Error: Start line {file_metadata.start_line} is greater than " + f"end line {file_metadata.end_line} for file {file_metadata.file_path}") + selected_lines = lines[file_metadata.start_line - 1:file_metadata.end_line]
166-181: Add GitHub API version header.Improves compatibility across org policies.
headers = { "Authorization": f"Bearer {token}", "Accept": "application/vnd.github+json", - "User-Agent": "NeMo-Agent-Toolkit", + "User-Agent": "NeMo-Agent-Toolkit", + "X-GitHub-Api-Version": "2022-11-28", }
151-160: Annotate async generators with return types.Conform to repo typing rules.
-from typing import Literal +from typing import Literal, AsyncIterator @@ -@register_function_group(config_type=GithubGroupConfig) -async def github_tool(config: GithubGroupConfig, _builder: Builder): +@register_function_group(config_type=GithubGroupConfig) +async def github_tool(config: GithubGroupConfig, _builder: Builder) -> AsyncIterator[FunctionGroup]: @@ -@register_function(config_type=GithubFilesGroupConfig) -async def github_files_tool(config: GithubFilesGroupConfig, _builder: Builder): +@register_function(config_type=GithubFilesGroupConfig) +async def github_files_tool(config: GithubFilesGroupConfig, _builder: Builder) -> AsyncIterator[FunctionInfo]:Also applies to: 377-381
src/nat/builder/workflow_builder.py (2)
373-410: Seed dependencies under instance key, not config.type.Storing at
config.typethen again atnameleaves a stray entry and complicates lookups. Recommend seeding undernameonly.- self.current_function_group_building = config.type - # Empty set of dependencies for the current function group - self.function_group_dependencies[config.type] = FunctionDependencies() + self.current_function_group_building = config.type + # Empty set of dependencies for the current function group (track by instance key) + self.function_group_dependencies[name] = FunctionDependencies() @@ - self.function_group_dependencies[name] = inner_builder.dependencies + self.function_group_dependencies[name] = inner_builder.dependencies
438-447: Avoid double lookup; cache included functions.Micro-optimization and readability.
- # If the function group exposes functions, record and add them to the registry - for k in build_result.instance.get_included_functions(): + # If the function group exposes functions, record and add them to the registry + included = build_result.instance.get_included_functions() + for k in included: if k in self._functions: raise ValueError(f"Exposed function `{k}` from group `{name}` conflicts with an existing function") - self._functions.update({ - k: ConfiguredFunction(config=v.config, instance=v) - for k, v in build_result.instance.get_included_functions().items() - }) + self._functions.update({k: ConfiguredFunction(config=v.config, instance=v) for k, v in included.items()})tests/nat/builder/test_builder.py (2)
139-143: Docstring mismatch (“includes” vs “excludes”).AllExcludesFunctionGroupConfig says “includes all functions.” Should say “excludes all functions.”
-class AllExcludesFunctionGroupConfig(FunctionGroupBaseConfig, name="all_excludes_function_group"): - """Test configuration that includes all functions.""" +class AllExcludesFunctionGroupConfig(FunctionGroupBaseConfig, name="all_excludes_function_group"): + """Test configuration that excludes all functions."""
1138-1165: Add a build()-path test to catch expose/include regression.Currently no test calls build() with groups; the builder bug around exposed functions wouldn’t be caught. Add a test that adds a group with includes, builds, and asserts the included functions are not duplicated in config.functions.
I can draft a targeted test if you want.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
ci/scripts/path_checks.py(1 hunks)docs/source/extend/function-groups.md(1 hunks)docs/source/index.md(2 hunks)docs/source/workflows/function-groups.md(1 hunks)examples/object_store/user_report/README.md(8 hunks)examples/object_store/user_report/configs/config_mem.yml(1 hunks)examples/object_store/user_report/configs/config_mysql.yml(1 hunks)examples/object_store/user_report/configs/config_redis.yml(1 hunks)examples/object_store/user_report/configs/config_s3.yml(1 hunks)examples/object_store/user_report/src/nat_user_report/user_report_tools.py(3 hunks)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py(5 hunks)src/nat/agent/react_agent/register.py(2 hunks)src/nat/agent/reasoning_agent/reasoning_agent.py(1 hunks)src/nat/agent/rewoo_agent/register.py(2 hunks)src/nat/agent/tool_calling_agent/register.py(2 hunks)src/nat/builder/builder.py(5 hunks)src/nat/builder/component_utils.py(4 hunks)src/nat/builder/function.py(3 hunks)src/nat/builder/workflow.py(5 hunks)src/nat/builder/workflow_builder.py(17 hunks)src/nat/cli/register_workflow.py(4 hunks)src/nat/cli/type_registry.py(10 hunks)src/nat/data_models/component.py(2 hunks)src/nat/data_models/component_ref.py(1 hunks)src/nat/data_models/config.py(10 hunks)src/nat/data_models/function.py(2 hunks)src/nat/data_models/function_dependencies.py(3 hunks)src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py(1 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py(1 hunks)src/nat/profiler/utils.py(2 hunks)src/nat/tool/github_tools.py(1 hunks)src/nat/tool/github_tools/create_github_commit.py(0 hunks)src/nat/tool/github_tools/create_github_issue.py(0 hunks)src/nat/tool/github_tools/create_github_pr.py(0 hunks)src/nat/tool/github_tools/get_github_file.py(0 hunks)src/nat/tool/github_tools/get_github_issue.py(0 hunks)src/nat/tool/github_tools/get_github_pr.py(0 hunks)src/nat/tool/github_tools/update_github_issue.py(0 hunks)src/nat/tool/register.py(1 hunks)tests/nat/agent/test_reasoning_agent.py(3 hunks)tests/nat/builder/test_builder.py(16 hunks)tests/nat/front_ends/mcp/test_mcp_custom_routes.py(2 hunks)
💤 Files with no reviewable changes (7)
- src/nat/tool/github_tools/create_github_issue.py
- src/nat/tool/github_tools/create_github_commit.py
- src/nat/tool/github_tools/create_github_pr.py
- src/nat/tool/github_tools/update_github_issue.py
- src/nat/tool/github_tools/get_github_issue.py
- src/nat/tool/github_tools/get_github_pr.py
- src/nat/tool/github_tools/get_github_file.py
✅ Files skipped from review due to trivial changes (1)
- docs/source/workflows/function-groups.md
🚧 Files skipped from review as they are similar to previous changes (17)
- src/nat/data_models/function_dependencies.py
- src/nat/data_models/component.py
- src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
- src/nat/agent/react_agent/register.py
- src/nat/agent/tool_calling_agent/register.py
- docs/source/index.md
- docs/source/extend/function-groups.md
- src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py
- examples/object_store/user_report/configs/config_mem.yml
- src/nat/data_models/component_ref.py
- examples/object_store/user_report/README.md
- src/nat/agent/rewoo_agent/register.py
- src/nat/profiler/utils.py
- src/nat/cli/register_workflow.py
- tests/nat/front_ends/mcp/test_mcp_custom_routes.py
- examples/object_store/user_report/src/nat_user_report/user_report_tools.py
- src/nat/builder/workflow.py
🧰 Additional context used
📓 Path-based instructions (14)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
Files:
ci/scripts/path_checks.pysrc/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pysrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pytests/nat/builder/test_builder.pysrc/nat/data_models/config.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
ci/scripts/path_checks.pysrc/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pyexamples/object_store/user_report/configs/config_s3.ymlsrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pytests/nat/builder/test_builder.pyexamples/object_store/user_report/configs/config_mysql.ymlsrc/nat/data_models/config.pyexamples/object_store/user_report/configs/config_redis.ymlpackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
ci/scripts/path_checks.pysrc/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pysrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pytests/nat/builder/test_builder.pysrc/nat/data_models/config.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
ci/scripts/path_checks.pysrc/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pyexamples/object_store/user_report/configs/config_s3.ymlsrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pytests/nat/builder/test_builder.pyexamples/object_store/user_report/configs/config_mysql.ymlsrc/nat/data_models/config.pyexamples/object_store/user_report/configs/config_redis.ymlpackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
ci/scripts/path_checks.pysrc/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pyexamples/object_store/user_report/configs/config_s3.ymlsrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pytests/nat/builder/test_builder.pyexamples/object_store/user_report/configs/config_mysql.ymlsrc/nat/data_models/config.pyexamples/object_store/user_report/configs/config_redis.ymlpackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pytests/nat/agent/test_reasoning_agent.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks
Files:
src/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pysrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Core functionality under src/nat should prioritize backward compatibility when changed
Files:
src/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pysrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/tool/register.pysrc/nat/tool/github_tools.pysrc/nat/agent/reasoning_agent/reasoning_agent.pysrc/nat/builder/component_utils.pysrc/nat/builder/function.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
**/configs/**/*.y?(a)ml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must live in a neighbouring configs/ directory
Files:
examples/object_store/user_report/configs/config_s3.ymlexamples/object_store/user_report/configs/config_mysql.ymlexamples/object_store/user_report/configs/config_redis.yml
**/*.{yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
In workflow/config YAML, set llms.._type: nat_test_llm to stub responses.
Files:
examples/object_store/user_report/configs/config_s3.ymlexamples/object_store/user_report/configs/config_mysql.ymlexamples/object_store/user_report/configs/config_redis.yml
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/object_store/user_report/configs/config_s3.ymlexamples/object_store/user_report/configs/config_mysql.ymlexamples/object_store/user_report/configs/config_redis.yml
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
src/nat/builder/component_utils.pysrc/nat/builder/function.pytests/nat/builder/test_builder.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are separate class hierarchies that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other.
Applied to files:
src/nat/builder/component_utils.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pytests/nat/builder/test_builder.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are sibling classes that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other and the order doesn't matter for classification purposes.
Applied to files:
src/nat/builder/component_utils.pysrc/nat/cli/type_registry.pysrc/nat/builder/builder.pytests/nat/builder/test_builder.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:30:12.178Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/data_models/function.py:39-45
Timestamp: 2025-08-28T14:30:12.178Z
Learning: In the NAT codebase, stricter validation of function names (like checking for empty/whitespace-only strings) in FunctionGroupBaseConfig.validate_expose is unnecessary because workflow parsing and validation already handle these validation concerns at a different layer in the system.
Applied to files:
src/nat/builder/function.pytests/nat/builder/test_builder.pyexamples/object_store/user_report/configs/config_mysql.ymlsrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.
Applied to files:
src/nat/builder/function.pytests/nat/builder/test_builder.pyexamples/object_store/user_report/configs/config_mysql.ymlsrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.
Applied to files:
src/nat/builder/function.pytests/nat/builder/test_builder.pyexamples/object_store/user_report/configs/config_mysql.ymlsrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:29:34.897Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/workflow_builder.py:373-410
Timestamp: 2025-08-28T14:29:34.897Z
Learning: ComponentRef objects (like FunctionGroupRef) automatically convert to strings when used as dictionary keys in Python, so explicit str() conversions are not required when using them as dict keys.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T23:22:41.742Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-28T23:22:41.742Z
Learning: Applies to **/*.py : When re-raising exceptions, use bare `raise` and log with logger.error(), not logger.exception()
Applied to files:
src/nat/builder/workflow_builder.py
🧬 Code graph analysis (13)
src/nat/tool/register.py (1)
src/nat/tool/github_tools.py (2)
github_tool(152-370)github_files_tool(378-450)
src/nat/tool/github_tools.py (4)
src/nat/builder/builder.py (1)
Builder(64-281)src/nat/builder/function.py (2)
FunctionGroup(350-526)add_function(371-411)src/nat/data_models/function.py (2)
FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (2)
add_function(412-423)add_function(1111-1112)
src/nat/agent/reasoning_agent/reasoning_agent.py (3)
examples/object_store/user_report/tests/test_user_report_tool.py (1)
builder(33-49)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (2)
get_function_dependencies(284-286)get_function_group_dependencies(288-290)src/nat/builder/workflow_builder.py (4)
get_function_dependencies(514-517)get_function_dependencies(1296-1297)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)
src/nat/builder/component_utils.py (2)
src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/data_models/component.py (1)
ComponentGroup(46-56)
src/nat/builder/function.py (2)
src/nat/data_models/function.py (3)
EmptyFunctionConfig(58-59)FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/context.py (1)
Context(93-277)
src/nat/cli/type_registry.py (8)
src/nat/builder/function.py (1)
FunctionGroup(350-526)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/builder.py (2)
Builder(64-281)get_function_group(79-80)src/nat/cli/register_workflow.py (1)
register_function_group(180-211)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (1)
get_function_group(151-157)src/nat/builder/workflow_builder.py (2)
get_function_group(460-466)get_function_group(1128-1134)src/nat/data_models/component.py (1)
ComponentEnum(22-43)src/nat/data_models/common.py (1)
static_type(154-155)
src/nat/builder/builder.py (6)
src/nat/builder/function.py (2)
FunctionGroup(350-526)Function(45-300)src/nat/data_models/component_ref.py (2)
FunctionGroupRef(105-113)FunctionRef(94-102)src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (7)
add_function_group(147-149)get_function(135-141)get_function_group(151-157)get_function_config(143-145)get_function_group_config(159-161)get_tools(179-181)get_function_group_dependencies(288-290)src/nat/builder/workflow_builder.py (14)
add_function_group(426-448)add_function_group(1115-1116)get_function(451-457)get_function(1119-1125)get_function_group(460-466)get_function_group(1128-1134)get_function_config(469-475)get_function_config(1137-1138)get_function_group_config(478-484)get_function_group_config(1141-1142)get_tools(526-559)get_tools(1157-1166)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)src/nat/data_models/function_dependencies.py (2)
add_function_group(64-65)FunctionDependencies(21-80)
tests/nat/builder/test_builder.py (6)
src/nat/builder/builder.py (6)
Builder(64-281)add_function(67-68)add_function_group(71-72)get_function_group(79-80)get_function_group_config(94-95)get_function(75-76)src/nat/builder/function.py (8)
Function(45-300)FunctionGroup(350-526)add_function(371-411)get_accessible_functions(441-463)get_all_functions(514-526)get_included_functions(488-512)get_excluded_functions(465-486)get_config(413-422)src/nat/cli/register_workflow.py (2)
register_function_group(180-211)register_function(148-177)src/nat/cli/type_registry.py (4)
register_function_group(499-515)register_function(477-485)get_function_group(517-533)get_function(487-493)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (12)
add_function(412-423)add_function(1111-1112)WorkflowBuilder(141-1095)add_function_group(426-448)add_function_group(1115-1116)get_function_group(460-466)get_function_group(1128-1134)get_function_group_config(478-484)get_function_group_config(1141-1142)get_function(451-457)get_function(1119-1125)dependencies(1107-1108)
src/nat/data_models/config.py (3)
src/nat/data_models/function.py (3)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)EmptyFunctionConfig(58-59)src/nat/cli/type_registry.py (3)
get(1049-1050)get_registered_function_groups(535-541)compute_annotation(997-1041)src/nat/data_models/common.py (2)
TypedBaseModel(93-168)discriminator(162-168)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (5)
src/nat/builder/builder.py (6)
Builder(64-281)add_function_group(71-72)get_function_group(79-80)get_function_group_config(94-95)get_tools(110-113)get_function_group_dependencies(280-281)src/nat/builder/function.py (2)
Function(45-300)FunctionGroup(350-526)src/nat/data_models/function.py (2)
FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (10)
add_function_group(426-448)add_function_group(1115-1116)get_function_group(460-466)get_function_group(1128-1134)get_function_group_config(478-484)get_function_group_config(1141-1142)get_tools(526-559)get_tools(1157-1166)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)src/nat/data_models/function_dependencies.py (2)
add_function_group(64-65)FunctionDependencies(21-80)
src/nat/builder/workflow_builder.py (9)
src/nat/builder/function.py (5)
FunctionGroup(350-526)add_function(371-411)Function(45-300)get_included_functions(488-512)get_accessible_functions(441-463)src/nat/data_models/component_ref.py (12)
FunctionGroupRef(105-113)FunctionRef(94-102)component_group(69-76)component_group(90-91)component_group(101-102)component_group(112-113)component_group(123-124)component_group(134-135)component_group(145-146)component_group(156-157)component_group(167-168)component_group(178-179)src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)src/nat/data_models/function_dependencies.py (5)
FunctionDependencies(21-80)add_function(61-62)add_function_group(64-65)add_object_store(76-77)add_retriever(79-80)src/nat/data_models/config.py (1)
Config(238-423)src/nat/builder/workflow.py (2)
Workflow(41-160)from_entry_fn(128-160)src/nat/builder/builder.py (11)
get_function_group(79-80)add_function(67-68)add_function_group(71-72)get_function(75-76)get_function_config(90-91)get_function_group_config(94-95)get_function_group_dependencies(280-281)get_tools(110-113)get_tool(116-117)add_object_store(157-158)add_retriever(217-218)src/nat/profiler/decorators/framework_wrapper.py (1)
chain_wrapped_build_fn(108-128)src/nat/data_models/component.py (1)
ComponentGroup(46-56)
src/nat/data_models/function.py (1)
src/nat/data_models/common.py (2)
TypedBaseModel(93-168)BaseModelRegistryTag(88-90)
tests/nat/agent/test_reasoning_agent.py (3)
examples/object_store/user_report/tests/test_user_report_tool.py (1)
builder(33-49)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (2)
get_function_dependencies(284-286)get_function_group_dependencies(288-290)src/nat/builder/workflow_builder.py (4)
get_function_dependencies(514-517)get_function_dependencies(1296-1297)get_function_group_dependencies(520-523)get_function_group_dependencies(1300-1301)
🪛 Ruff (0.12.2)
src/nat/tool/github_tools.py
66-66: Avoid specifying long messages outside the exception class
(TRY003)
173-174: Avoid specifying long messages outside the exception class
(TRY003)
266-266: Avoid specifying long messages outside the exception class
(TRY003)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/builder/function.py
402-402: Avoid specifying long messages outside the exception class
(TRY003)
404-404: Avoid specifying long messages outside the exception class
(TRY003)
406-406: Avoid specifying long messages outside the exception class
(TRY003)
432-433: Avoid specifying long messages outside the exception class
(TRY003)
481-482: Avoid specifying long messages outside the exception class
(TRY003)
506-508: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/cli/type_registry.py
509-511: Avoid specifying long messages outside the exception class
(TRY003)
532-533: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/builder/test_builder.py
292-292: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
413-413: Unused function argument: config
(ARG001)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
157-157: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Unused method argument: name
(ARG002)
179-179: Unused method argument: tool_names
(ARG002)
179-179: Unused method argument: wrapper_type
(ARG002)
288-288: Unused method argument: fn_name
(ARG002)
src/nat/builder/workflow_builder.py
406-407: Prefer TypeError exception for invalid type
(TRY004)
406-407: Avoid specifying long messages outside the exception class
(TRY003)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
431-431: Avoid specifying long messages outside the exception class
(TRY003)
442-442: Avoid specifying long messages outside the exception class
(TRY003)
455-455: Avoid specifying long messages outside the exception class
(TRY003)
464-464: Avoid specifying long messages outside the exception class
(TRY003)
473-473: Avoid specifying long messages outside the exception class
(TRY003)
482-482: Avoid specifying long messages outside the exception class
(TRY003)
536-536: Avoid specifying long messages outside the exception class
(TRY003)
541-541: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/data_models/function.py
48-48: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/agent/test_reasoning_agent.py
141-141: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
142-142: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
305-305: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
380-380: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
381-381: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (20)
ci/scripts/path_checks.py (1)
107-107: Allowlist term addition looks good."delete/recreate" is consistent with existing literal/regex entries and will be correctly ignored by ALLOWLISTED_WORDS_REGEX.
src/nat/builder/function.py (1)
514-526: LGTM: preserves insertion order and returns fully qualified names.src/nat/agent/reasoning_agent/reasoning_agent.py (1)
116-121: LGTM: safe merge of function and group deps without mutating builder state.Creating a fresh set and updating per group fixes the prior aliasing issue.
src/nat/tool/register.py (1)
27-28: Consolidated GitHub tool exports look good.Importing
github_toolandgithub_files_toolaligns with the Function Groups refactor and preserves auto-registration via the module import above.src/nat/builder/component_utils.py (3)
33-33: Correct: addFunctionGroupBaseConfigimport for grouping support.
52-53: Right order: process function groups before functions.Ensures group-scoped deps/contexts are available before individual functions.
112-113: Mapping for FunctionGroup configs is correct.Order with
FunctionBaseConfigis fine since these base classes are siblings (no misclassification risk per current hierarchy).src/nat/data_models/function.py (1)
30-43: Solid include/exclude design; uniqueness check is correct.Clean separation from FunctionBaseConfig and clear erroring on duplicates. LGTM.
Also applies to: 44-50
src/nat/builder/builder.py (1)
109-113: API surface for group-aware get_tools looks good.Signature aligns with FunctionRef/FunctionGroupRef usage.
src/nat/data_models/config.py (1)
239-268: Nice: default_factory for dicts, print_summary, and annotations for function_groups.Good defensive defaults and visibility.
Also applies to: 286-288, 339-341, 384-388
src/nat/tool/github_tools.py (1)
263-339: Commit flow looks correct and hardened.Good path traversal guard, base64 blobs, parent/tree/commit/ref sequence, and error handling.
src/nat/cli/type_registry.py (7)
91-114: FunctionGroup callable type aliases look good.The added type aliases align with the existing Function callables and keep symmetry across the API surface.
186-194: RegisteredFunctionGroupInfo is well-scoped and documented.Clear docstring, correct type parameters, and consistent with the other Registered* containers.
331-333: State slot for function groups added correctly.Initialization mirrors other component registries; no issues spotted.
499-541: Add/get/list API for function groups integrates cleanly.Error messaging matches style of adjacent registrars; _registration_changed hook is invoked. LGTM.
855-857: get_infos_by_type() extension for FUNCTION_GROUP is correct.Return type aligns with other component branches.
922-924: Minor verification request: static_type access on dict keys.You’re iterating dict keys (types) and calling i.static_type(). If static_type is not a classmethod on those types (and instead a utility function), this will fail at runtime. Please confirm it’s a classmethod on all config types.
1014-1016: compute_annotation() support for FunctionGroupBaseConfig is correct.Delegation mirrors the Function branch; no concerns.
src/nat/builder/workflow_builder.py (1)
243-249: Bug: build() uses removedexposefield; duplicates/AttributeError risk.FunctionGroupBaseConfig now uses include/exclude; accessing
v.config.exposewill fail. Also, exposed (globally addressable) functions should come from each group’s included set.Apply:
- # Get all functions which are exposed by function groups - exposed_functions = {f"{k}.{n}" for k, v in self._function_groups.items() for n in v.config.expose} + # Functions globally exposed by function groups = union of each group's included functions + exposed_functions = ( + set().union(*(g.instance.get_included_functions().keys() for g in self._function_groups.values())) + if self._function_groups else set() + ) @@ - # Expose functions that are not in a function group - function_instances = {k: v.instance for k, v in self._functions.items() if k not in exposed_functions} - # Expose all function groups - function_group_instances = {k: v.instance for k, v in self._function_groups.items()} + # Expose functions that are not in a function group + function_instances = {k: v.instance for k, v in self._functions.items() if k not in exposed_functions} + # Expose all function groups + function_group_instances = {k: v.instance for k, v in self._function_groups.items()}Also applies to: 284-287
⛔ Skipped due to learnings
Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: examples/object_store/user_report/configs/config_mysql.yml:38-48 Timestamp: 2025-08-28T14:04:37.688Z Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.Learnt from: willkill07 PR: NVIDIA/NeMo-Agent-Toolkit#684 File: src/nat/data_models/function.py:39-45 Timestamp: 2025-08-28T14:30:12.178Z Learning: In the NAT codebase, stricter validation of function names (like checking for empty/whitespace-only strings) in FunctionGroupBaseConfig.validate_expose is unnecessary because workflow parsing and validation already handle these validation concerns at a different layer in the system.tests/nat/builder/test_builder.py (1)
114-148: Config defaults and registrations look correct.Good use of Field(default_factory=...) and clear function-group coverage.
willkill07
force-pushed
the
wkk_function-groups
branch
from
aea7e1d to
28691cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nat/builder/workflow_builder.py (1)
486-496: Broaden set_workflow to accept FunctionGroup configs (API parity).Matches config/workflow annotation and Builder interface.
- async def set_workflow(self, config: FunctionBaseConfig) -> Function: + async def set_workflow(self, config: FunctionBaseConfig | FunctionGroupBaseConfig) -> Function:tests/nat/builder/test_builder.py (1)
891-934: Fix variable shadowing; current assertion is a tautology.
workflow_configis reassigned toworkflow.config, makingassert workflow_config.workflow == workflow_config.workflowalways true. Use distinct names.-async def test_built_config(): +async def test_built_config(): @@ - workflow_config = FunctionReturningFunctionConfig() + workflow_cfg = FunctionReturningFunctionConfig() @@ - await builder.set_workflow(workflow_config) + await builder.set_workflow(workflow_cfg) @@ - workflow_config = workflow.config + built_cfg = workflow.config @@ - assert workflow_config.general == general_config - assert workflow_config.functions == {"function1": function_config} - assert workflow_config.workflow == workflow_config.workflow - assert workflow_config.llms == {"llm1": llm_config} - assert workflow_config.embedders == {"embedder1": embedder_config} - assert workflow_config.memory == {"memory1": memory_config} - assert workflow_config.retrievers == {"retriever1": retriever_config} - assert workflow_config.object_stores == {"object_store1": object_store_config} - assert workflow_config.ttc_strategies == {"ttc_strategy": ttc_config} + assert built_cfg.general == general_config + assert built_cfg.functions == {"function1": function_config} + assert built_cfg.workflow == workflow_cfg + assert built_cfg.llms == {"llm1": llm_config} + assert built_cfg.embedders == {"embedder1": embedder_config} + assert built_cfg.memory == {"memory1": memory_config} + assert built_cfg.retrievers == {"retriever1": retriever_config} + assert built_cfg.object_stores == {"object_store1": object_store_config} + assert built_cfg.ttc_strategies == {"ttc_strategy": ttc_config}
♻️ Duplicate comments (21)
src/nat/data_models/function_dependencies.py (1)
37-39: Deterministic JSON: sortfunction_groupsduring serialization.Repeat of a prior suggestion: converting a set to a list yields nondeterministic order; sort for stable outputs and easier testing.
- def serialize_function_groups(self, v: set[str]) -> list[str]: - return list(v) + def serialize_function_groups(self, v: set[str]) -> list[str]: + return sorted(v)src/nat/profiler/utils.py (1)
36-38: Signature update LGTM; align doc/log text to function groups and support Enum values.Great to accept
RegisteredFunctionGroupInfo. Also:
- Adjust the warning to mention
@register_function_group.- Accept enum instances in
framework_wrappersto be robust (common in internal code).[suggest_minor_issue]
- if registration.framework_wrappers: - results: list[LLMFrameworkEnum] = [] - for fw_str in registration.framework_wrappers: - try: - results.append(LLMFrameworkEnum(fw_str)) - except ValueError: - # If it's not recognized, ignore or log - logger.warning("Unrecognized framework %s in registration.framework_wrappers", fw_str) + if registration.framework_wrappers: + results: list[LLMFrameworkEnum] = [] + for fw in registration.framework_wrappers: + try: + results.append(fw if isinstance(fw, LLMFrameworkEnum) else LLMFrameworkEnum(fw)) + except ValueError: + logger.warning("Unrecognized framework %s in registration.framework_wrappers", fw) return list(set(results)) # uniqueAnd update the guidance below:
- "types in the framework_wrappers argument when calling @register_function.", + "types in the framework_wrappers argument when calling @register_function or @register_function_group.",src/nat/builder/component_utils.py (2)
112-113: Classification forFunctionGroupBaseConfig— LGTM.Given
FunctionGroupBaseConfigandFunctionBaseConfigare siblings, current order is fine per prior discussion.
261-263: Backward compatibility: guard missing attrs in older configs.Direct access to
config.function_groupscan break old configs; mirror the tolerant pattern withgetattr(..., {}).- total_node_count = (len(config.embedders) + len(config.functions) + len(config.function_groups) + len(config.llms) + - len(config.memory) + len(config.object_stores) + len(config.retrievers) + - len(config.ttc_strategies) + len(config.authentication) + 1) # +1 for the workflow + total_node_count = ( + len(getattr(config, "embedders", {})) + + len(getattr(config, "functions", {})) + + len(getattr(config, "function_groups", {})) + + len(getattr(config, "llms", {})) + + len(getattr(config, "memory", {})) + + len(getattr(config, "object_stores", {})) + + len(getattr(config, "retrievers", {})) + + len(getattr(config, "ttc_strategies", {})) + + len(getattr(config, "authentication", {})) + + 1 # +1 for the workflow + )#!/bin/bash # Quick grep to spot any remaining direct attribute reads that could break older configs rg -nP --type=py 'config\.(functions|function_groups|embedders|llms|memory|object_stores|retrievers|ttc_strategies|authentication)\b' -C2examples/object_store/user_report/README.md (3)
124-128: Fix broken docker‑compose MinIO link (missingexamples/path segment).-You can use the [docker-compose.minio.yml](../../deploy/docker-compose.minio.yml) file to start a MinIO server in a local docker container. +You can use the [docker-compose.minio.yml](../../examples/deploy/docker-compose.minio.yml) file to start a MinIO server in a local docker container.
135-147: Fix broken docker‑compose MySQL link (missingexamples/path segment).-If you want to use a MySQL server, you can use the [docker-compose.mysql.yml](../../deploy/docker-compose.mysql.yml) file to start a MySQL server in a local docker container. +If you want to use a MySQL server, you can use the [docker-compose.mysql.yml](../../examples/deploy/docker-compose.mysql.yml) file to start a MySQL server in a local docker container.
154-158: Fix broken docker‑compose Redis link (missingexamples/path segment).-If you want to use a Redis server, you can use the [docker-compose.redis.yml](../../deploy/docker-compose.redis.yml) file to start a Redis server in a local docker container. +If you want to use a Redis server, you can use the [docker-compose.redis.yml](../../examples/deploy/docker-compose.redis.yml) file to start a Redis server in a local docker container.examples/object_store/user_report/configs/config_s3.yml (1)
16-21: Remove duplicate object_store at front_end.Group-level object_store under function_groups.user_report is the source of truth; keep only one to avoid schema validation issues.
general: front_end: _type: fastapi - object_store: report_object_store cors: allow_origins: ['*']tests/nat/agent/test_reasoning_agent.py (3)
141-146: Annotate class attributes as ClassVar (RUF012).- class FakeDeps: - functions = {"SomeTool"} - function_groups = set() + from typing import ClassVar + class FakeDeps: + functions: ClassVar[set[str]] = {"SomeTool"} + function_groups: ClassVar[set[str]] = set()
305-310: Repeat ClassVar annotations here.- class FakeDeps: - functions = {"ToolA", "ToolB"} - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = {"ToolA", "ToolB"} + function_groups: ClassVar[set[str]] = set()
380-385: Repeat ClassVar annotations for empty-set FakeDeps.- class FakeDeps: - functions = set() - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = set() + function_groups: ClassVar[set[str]] = set()docs/source/workflows/function-groups.md (1)
52-53: Fix Builder import path in snippet.-from nat.builder.workflow_builder import Builder +from nat.builder.builder import Builderpackages/nvidia_nat_test/src/nat/test/tool_test_runner.py (4)
147-150: Align add_function_group signature with abstract Builder.- async def add_function_group(self, name: str, config: FunctionGroupBaseConfig) -> FunctionGroup: + async def add_function_group(self, name: str | FunctionGroupRef, config: FunctionGroupBaseConfig) -> FunctionGroup:Additional imports required:
# at top-level imports from nat.data_models.component_ref import FunctionRef, FunctionGroupRef from nat.builder.framework_enum import LLMFrameworkEnum
151-158: Widen get_function_group signature; handle refs safely.- def get_function_group(self, name: str) -> FunctionGroup: + def get_function_group(self, name: str | FunctionGroupRef) -> FunctionGroup: """Return a mock function group if one is configured.""" - if name in self._mocks: - mock_fn_group = MagicMock(spec=FunctionGroup) - mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[name]) + key = str(name) + if key in self._mocks: + mock_fn_group = MagicMock(spec=FunctionGroup) + mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[key]) return mock_fn_group raise ValueError(f"Function group '{name}' not mocked. Use mock_function_group() to add it.")
159-162: Match abstract get_function_group_config signature.- def get_function_group_config(self, name: str) -> FunctionGroupBaseConfig: + def get_function_group_config(self, name: str | FunctionGroupRef) -> FunctionGroupBaseConfig: """Mock implementation.""" return FunctionGroupBaseConfig()
179-182: Match Builder.get_tools signature and types; silence ARG002.- def get_tools(self, tool_names: Sequence[str], wrapper_type): - """Mock implementation.""" - return [] + def get_tools( + self, + tool_names: Sequence[str | FunctionRef | FunctionGroupRef], + wrapper_type: LLMFrameworkEnum | str, + ) -> list[typing.Any]: + """Mock implementation.""" + _ = tool_names; _ = wrapper_type + return []examples/object_store/user_report/configs/config_mysql.yml (1)
16-21: Remove duplicate object_store at front_end.general: front_end: _type: fastapi - object_store: report_object_store cors: allow_origins: ['*']examples/object_store/user_report/src/nat_user_report/user_report_tools.py (1)
46-47: Annotate function-group builder to return AsyncIterator[FunctionGroup].Conforms to repo typing rules for public APIs.
@register_function_group(config_type=UserReportConfig) -async def user_report_group(config: UserReportConfig, builder: Builder): +async def user_report_group(config: UserReportConfig, builder: Builder) -> "AsyncIterator[FunctionGroup]": """Register a function group for user report operations.Add import:
# top of file from typing import AsyncIteratorsrc/nat/builder/function.py (1)
371-378: Public API: add explicit return type and tighten converter typing.Complies with repo typing rules and improves clarity.
- def add_function(self, + def add_function(self, name: str, fn: Callable, *, input_schema: type[BaseModel] | None = None, description: str | None = None, - converters: list[Callable] | None = None): + converters: list[Callable[[typing.Any], typing.Any]] | None = None) -> None:src/nat/data_models/config.py (1)
61-66: Workflow suggestions must include function groups.Returning only functions for
workflowbreaks UX and the PR intent.- if (info.field_name in ('workflow', 'functions')): - registered_keys = GlobalTypeRegistry.get().get_registered_functions() - elif (info.field_name == "function_groups"): - registered_keys = GlobalTypeRegistry.get().get_registered_function_groups() + if info.field_name == 'workflow': + registered_keys = ( + list(GlobalTypeRegistry.get().get_registered_functions()) + + list(GlobalTypeRegistry.get().get_registered_function_groups()) + ) + elif info.field_name == 'functions': + registered_keys = GlobalTypeRegistry.get().get_registered_functions() + elif info.field_name == 'function_groups': + registered_keys = GlobalTypeRegistry.get().get_registered_function_groups()src/nat/tool/github_tools.py (1)
424-431: Fix raw.githubusercontent.com URL (remove refs/heads).Current URL 404s; correct format is owner/repo/branch/path.
- # The following URL is the raw URL of the file. refs/heads/ always points to the top commit of the branch - raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/refs/heads/{file_metadata.file_path}" + # Build raw URL: owner/repo/branch/path + raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/{file_metadata.file_path}"
🧹 Nitpick comments (22)
src/nat/data_models/component_ref.py (1)
105-114: Add return type for the overridden property (consistency with abstract signature).The abstract
component_groupis annotated to returnComponentGroup; mirror that here for clarity and pyright.@property @override - def component_group(self): + def component_group(self) -> ComponentGroup: return ComponentGroup.FUNCTION_GROUPSsrc/nat/data_models/function_dependencies.py (2)
33-59: Apply the same deterministic sort to all set serializers.Keep JSON stable across runs by sorting every set serializer, not only
function_groups.- def serialize_functions(self, v: set[str]) -> list[str]: - return list(v) + def serialize_functions(self, v: set[str]) -> list[str]: + return sorted(v) - def serialize_llms(self, v: set[str]) -> list[str]: - return list(v) + def serialize_llms(self, v: set[str]) -> list[str]: + return sorted(v) - def serialize_embedders(self, v: set[str]) -> list[str]: - return list(v) + def serialize_embedders(self, v: set[str]) -> list[str]: + return sorted(v) - def serialize_memory_clients(self, v: set[str]) -> list[str]: - return list(v) + def serialize_memory_clients(self, v: set[str]) -> list[str]: + return sorted(v) - def serialize_object_stores(self, v: set[str]) -> list[str]: - return list(v) + def serialize_object_stores(self, v: set[str]) -> list[str]: + return sorted(v) - def serialize_retrievers(self, v: set[str]) -> list[str]: - return list(v) + def serialize_retrievers(self, v: set[str]) -> list[str]: + return sorted(v)
64-66: Type‑hint return and drop unnecessary pylint disable.Methods should be fully typed; the attribute exists so the pylint suppression is unnecessary.
- def add_function_group(self, function_group: str): - self.function_groups.add(function_group) # pylint: disable=no-member + def add_function_group(self, function_group: str) -> None: + self.function_groups.add(function_group)examples/object_store/user_report/README.md (2)
53-71: Optional: clarify order semantics.If the config sorts
include/exclude, add a note that exposure order is alphabetical for determinism.-**Naming Convention**: Functions are referenced as `user_report.get`, `user_report.put`, etc. +**Naming Convention**: Functions are referenced as `user_report.get`, `user_report.put`, etc. +**Order**: When using `include`/`exclude`, functions are expanded in deterministic (alphabetical) order.
20-21: Grammar nit: “An example tool …”-And example tool in the NeMo Agent toolkit that makes use of an Object Store to retrieve data. +An example tool in the NeMo Agent toolkit that makes use of an Object Store to retrieve data.examples/object_store/user_report/configs/config_s3.yml (1)
31-36: Use nat_test_llm in examples to follow repo YAML guidance.Stubs make examples deterministic and CI-friendly.
llms: nim_llm: - _type: nim - model_name: meta/llama-3.1-70b-instruct - temperature: 0.0 + _type: nat_test_llm + response_seq: [] + delay_ms: 0src/nat/data_models/function.py (1)
47-55: Shorten error messages or define exception class (TRY003).Ruff TRY003 suggests shorter messages outside custom exceptions.
- raise ValueError("Function names must be unique") + raise ValueError("Names must be unique") @@ - raise ValueError("include and exclude cannot be used together") + raise ValueError("Cannot set both include and exclude")docs/source/workflows/function-groups.md (2)
108-119: Make include/exclude options mutually exclusive in YAML example.Current snippet shows both; the model forbids using them together.
math: _type: math_group # Option A: include and exclude no functions globally, but still usable as a group # # omit include and exclude lists # Option B: exclude selected functions globally - exclude: [mul] + # exclude: [mul] # Option C: include selected functions globally (accessible as a function named math.add, math.mul) - include: [add, mul] + include: [add, mul]
48-56: Annotate example builder with AsyncIterator return type.-from pydantic import Field +from pydantic import Field +from typing import AsyncIterator @@ -@register_function_group(config_type=MathGroupConfig) -async def build_math_group(config: MathGroupConfig, _builder: Builder): +@register_function_group(config_type=MathGroupConfig) +async def build_math_group(config: MathGroupConfig, _builder: Builder) -> AsyncIterator[FunctionGroup]:Also applies to: 72-96
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (1)
288-291: Use fn_name to avoid ARG002 (or explicitly discard).- def get_function_group_dependencies(self, fn_name: str) -> FunctionDependencies: - """Mock implementation.""" - return FunctionDependencies() + def get_function_group_dependencies(self, fn_name: str) -> FunctionDependencies: + """Mock implementation.""" + _ = fn_name + return FunctionDependencies()src/nat/builder/builder.py (1)
279-282: Widen get_function_group_dependencies to accept refs.Keeps parity with other APIs (get_function_group, get_tools).
- def get_function_group_dependencies(self, fn_name: str) -> FunctionDependencies: + def get_function_group_dependencies(self, fn_name: str | FunctionGroupRef) -> FunctionDependencies: passexamples/object_store/user_report/configs/config_mysql.yml (1)
32-36: Use nat_test_llm in examples to follow repo YAML guidance.llms: nim_llm: - _type: nim - model_name: meta/llama-3.1-70b-instruct - temperature: 0.0 + _type: nat_test_llm + response_seq: [] + delay_ms: 0src/nat/builder/function.py (1)
401-406: Shorten exception messages and precompile the name regex (fixes TRY003).Concise errors and a compiled pattern avoid ruff warnings and repeated compilation.
+NAME_RE = re.compile(r"^[A-Za-z0-9_-]+$") @@ - if not name.strip(): - raise ValueError("Function name cannot be empty or blank") - if not re.match(r"^[a-zA-Z0-9_-]+$", name): - raise ValueError(f"Function name can only contain letters, numbers, underscores, and hyphens: {name}") - if name in self._functions: - raise ValueError(f"Function {name} already exists in function group {self._instance_name}") + name = name.strip() + if not name: + raise ValueError("Empty function name.") + if not NAME_RE.match(name): + raise ValueError(f"Invalid function name: {name!r}.") + if name in self._functions: + raise ValueError(f"Duplicate function: {name!r}.")Add near the imports (after
import re):+NAME_RE = re.compile(r"^[A-Za-z0-9_-]+$")src/nat/tool/github_tools.py (2)
176-181: Add recommended GitHub API version header.Improves compatibility with the GitHub API.
headers = { "Authorization": f"Bearer {token}", "Accept": "application/vnd.github+json", "User-Agent": "NeMo-Agent-Toolkit", + "X-GitHub-Api-Version": "2022-11-28", }
195-204: Encode labels as comma-separated string in GET /issues.GitHub expects labels in a single comma-separated param.
for issue in issues_list.filter_parameters: params = issue.model_dump(exclude_unset=True, exclude_none=True) + if isinstance(params.get("labels"), list): + params["labels"] = ",".join(params["labels"]) response = await client.get(url, params=params)src/nat/builder/workflow_builder.py (1)
396-404: Normalize function-group dependency key (use instance name consistently).Avoids stray entries under
config.type.- # Empty set of dependencies for the current function group - self.function_group_dependencies[config.type] = FunctionDependencies() + # Empty set of dependencies for the current function group + self.function_group_dependencies[name] = FunctionDependencies() @@ - self.function_group_dependencies[name] = inner_builder.dependencies + self.function_group_dependencies[name] = inner_builder.dependenciestests/nat/builder/test_builder.py (6)
114-148: Config defaults done right; fix a misleading docstring.
Field(default_factory=...)avoids mutable-default pitfalls. One nit: theAllExcludesFunctionGroupConfigdocstring says “includes all functions” but this config excludes all.Apply this diff to correct the docstring:
class AllExcludesFunctionGroupConfig(FunctionGroupBaseConfig, name="all_excludes_function_group"): - """Test configuration that includes all functions.""" + """Test configuration that excludes all functions."""
175-189: Add missing type hints in test helper class.Keep tests type-complete per guidelines.
Apply this diff:
class DerivedFunction(Function[str, str, str]): @@ - def some_method(self, val): + def some_method(self, val: str) -> str: return "some_method" + val @@ - async def _astream(self, value: str): + async def _astream(self, value: str): yield value + "!"Also add this import near the top if you choose to annotate
_astream’s return type later:from collections.abc import AsyncIterator
413-419: Silence ARG001 by marking unusedconfigparam.Rename the unused argument to
_config.-async def register_failing_function_group(config: FailingFunctionGroupConfig, _builder: Builder): +async def register_failing_function_group(_config: FailingFunctionGroupConfig, _builder: Builder):
291-293: TRY003 nit: long exception messages.Ruff flags “Avoid specifying long messages outside the exception class.” Fine to ignore in tests; if you prefer clean runs, either shorten messages, add a tiny custom exception, or annotate lines with
# noqa: TRY003.Also applies to: 318-320, 345-347, 362-364, 389-391
800-808: Annotate return type for the test retriever.Make the stub explicit for readability and tooling.
- async def search(self, query: str, **kwargs): + async def search(self, query: str, **kwargs) -> RetrieverOutput: return RetrieverOutput(results=[Document(page_content="page content", metadata={})])
1374-1390: Fixture naming: addname=and usefixture_prefix per guidelines.Keeps param names stable while satisfying style rules.
-@pytest.fixture -def caplog_fixture(caplog): +@pytest.fixture(name="caplog_fixture") +def fixture_caplog_fixture(caplog): @@ -@pytest.fixture -def mock_component_data(): +@pytest.fixture(name="mock_component_data") +def fixture_mock_component_data():
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/nat/builder/test_builder.py (6)
139-142: Fix docstring: “excludes” not “includes”.The class documents “includes all functions” but this config uses exclude to hide all.
class AllExcludesFunctionGroupConfig(FunctionGroupBaseConfig, name="all_excludes_function_group"): - """Test configuration that includes all functions.""" + """Test configuration that excludes all functions.""" exclude: list[str] = Field(default_factory=lambda: ["add", "multiply", "subtract"]) raise_error: bool = False
412-419: Silence unused-arg lint on config param.Rename to _config to satisfy Ruff ARG001 without changing behavior.
- async def register_failing_function_group(config: FailingFunctionGroupConfig, _builder: Builder): + async def register_failing_function_group(_config: FailingFunctionGroupConfig, _builder: Builder): """Register a function group that always fails during initialization.""" # This function group always raises an exception during initialization raise ValueError("Function group initialization failed") yield # This line will never be reached, but needed for the AsyncGenerator type
1054-1065: Use the config type as the function-name prefix, not the builder name.Function names are prefixed by the group’s instance_name (defaults to config.type, here “default_function_group”), so checking for "empty_group." is misleading even though the test currently passes.
- included_functions = [k for k in builder._functions.keys() if k.startswith("empty_group.")] + included_functions = [k for k in builder._functions.keys() if k.startswith("default_function_group.")]
921-934: Fix variable shadowing and tautological assertion.Avoid reusing workflow_config and assert against the originally set workflow config.
- workflow = builder.build() - - workflow_config = workflow.config - - assert workflow_config.general == general_config - assert workflow_config.functions == {"function1": function_config} - assert workflow_config.workflow == workflow_config.workflow - assert workflow_config.llms == {"llm1": llm_config} - assert workflow_config.embedders == {"embedder1": embedder_config} - assert workflow_config.memory == {"memory1": memory_config} - assert workflow_config.retrievers == {"retriever1": retriever_config} - assert workflow_config.object_stores == {"object_store1": object_store_config} - assert workflow_config.ttc_strategies == {"ttc_strategy": ttc_config} + workflow = builder.build() + + built_config = workflow.config + + assert built_config.general == general_config + assert built_config.functions == {"function1": function_config} + assert built_config.workflow == workflow_config + assert built_config.llms == {"llm1": llm_config} + assert built_config.embedders == {"embedder1": embedder_config} + assert built_config.memory == {"memory1": memory_config} + assert built_config.retrievers == {"retriever1": retriever_config} + assert built_config.object_stores == {"object_store1": object_store_config} + assert built_config.ttc_strategies == {"ttc_strategy": ttc_config}
1367-1368: Prefer isinstance over issubclass(type(...), ...).Clearer and idiomatic.
- assert issubclass(type(exporter1_instance), BaseExporter) + assert isinstance(exporter1_instance, BaseExporter)
150-152: Fixture naming consistency (optional).For consistency with repo guidelines, consider using a fixture_ prefix and setting name=... on the decorator, even for autouse fixtures. Behavior unchanged.
-@pytest.fixture(scope="module", autouse=True) -async def _register(): +@pytest.fixture(scope="module", autouse=True, name="fixture_register") +async def fixture_register():
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/nat/builder/test_builder.py(16 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)
Files:
tests/nat/builder/test_builder.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/builder/test_builder.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
Files:
tests/nat/builder/test_builder.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
tests/nat/builder/test_builder.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
tests/nat/builder/test_builder.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
tests/nat/builder/test_builder.py
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
tests/nat/builder/test_builder.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
tests/nat/builder/test_builder.py
🧠 Learnings (6)
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.
Applied to files:
tests/nat/builder/test_builder.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.
Applied to files:
tests/nat/builder/test_builder.py
📚 Learning: 2025-08-28T14:30:12.178Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/data_models/function.py:39-45
Timestamp: 2025-08-28T14:30:12.178Z
Learning: In the NAT codebase, stricter validation of function names (like checking for empty/whitespace-only strings) in FunctionGroupBaseConfig.validate_expose is unnecessary because workflow parsing and validation already handle these validation concerns at a different layer in the system.
Applied to files:
tests/nat/builder/test_builder.py
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
tests/nat/builder/test_builder.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are separate class hierarchies that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other.
Applied to files:
tests/nat/builder/test_builder.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are sibling classes that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other and the order doesn't matter for classification purposes.
Applied to files:
tests/nat/builder/test_builder.py
🧬 Code graph analysis (1)
tests/nat/builder/test_builder.py (4)
src/nat/builder/function.py (8)
Function(45-300)FunctionGroup(350-512)add_function(371-411)get_accessible_functions(437-459)get_all_functions(503-512)get_included_functions(481-501)get_excluded_functions(461-479)get_config(413-422)src/nat/cli/register_workflow.py (2)
register_function_group(180-211)register_function(148-177)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (11)
add_function(412-423)add_function(1111-1112)add_function_group(426-448)add_function_group(1115-1116)get_function_group(460-466)get_function_group(1128-1134)get_function_group_config(478-484)get_function_group_config(1141-1142)get_function(451-457)get_function(1119-1125)dependencies(1107-1108)
🪛 Ruff (0.12.2)
tests/nat/builder/test_builder.py
292-292: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
413-413: Unused function argument: config
(ARG001)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
tests/nat/builder/test_builder.py (2)
20-23: Pydantic import corrections — LGTM.Switching to Pydantic’s BaseModel and Field aligns with the codebase and fixes prior import issues.
114-148: FunctionGroup config defaults — LGTM.Using Field(default_factory=...) for include/exclude matches FunctionGroupBaseConfig semantics (empty lists by default) and avoids mutable default pitfalls.
willkill07
force-pushed
the
wkk_function-groups
branch
from
a6ab6f6 to
e8c227c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nat/agent/tool_calling_agent/register.py (1)
53-55: Add return type annotation (public API).Annotate the async generator per project guidelines.
-from nat.builder.builder import Builder +from nat.builder.builder import Builder +from collections.abc import AsyncIterator @@ -async def tool_calling_agent_workflow(config: ToolCallAgentWorkflowConfig, builder: Builder): +async def tool_calling_agent_workflow( + config: ToolCallAgentWorkflowConfig, builder: Builder +) -> AsyncIterator[FunctionInfo]:
♻️ Duplicate comments (8)
src/nat/agent/tool_calling_agent/register.py (1)
39-40: Clarify field description to include groups.Update to reflect that entries can be functions or function groups.
- tool_names: list[FunctionRef | FunctionGroupRef] = Field( - default_factory=list, description="The list of tools to provide to the tool calling agent.") + tool_names: list[FunctionRef | FunctionGroupRef] = Field( + default_factory=list, + description="The list of functions and/or function groups to provide to the tool calling agent.", + )src/nat/data_models/function.py (1)
44-50: Preserve caller order for include/exclude (don’t sort).Sorting alters user-specified order and can affect deterministic tool exposure.
@field_validator("include", "exclude") @classmethod def _validate_fields_include_exclude(cls, value: list[str]) -> list[str]: if len(set(value)) != len(value): raise ValueError("Function names must be unique") - return sorted(value) + return valuepackages/nvidia_nat_test/src/nat/test/tool_test_runner.py (3)
147-150: Align add_function_group signature with Builder API.- async def add_function_group(self, name: str, config: FunctionGroupBaseConfig) -> FunctionGroup: + async def add_function_group(self, name: str | FunctionGroupRef, config: FunctionGroupBaseConfig) -> FunctionGroup: """Mock implementation - not used in tool testing.""" raise NotImplementedError("Mock implementation does not support add_function_group")
159-162: Match get_function_group_config signature with Builder.- def get_function_group_config(self, name: str) -> FunctionGroupBaseConfig: + def get_function_group_config(self, name: str | FunctionGroupRef) -> FunctionGroupBaseConfig: """Mock implementation.""" return FunctionGroupBaseConfig()
151-158: Widen get_function_group signature; index mocks by str(name).- def get_function_group(self, name: str) -> FunctionGroup: + def get_function_group(self, name: str | FunctionGroupRef) -> FunctionGroup: """Return a mock function group if one is configured.""" - if name in self._mocks: + key = str(name) + if key in self._mocks: - mock_fn_group = MagicMock(spec=FunctionGroup) - mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[name]) + mock_fn_group = MagicMock(spec=FunctionGroup) + mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[key]) return mock_fn_group raise ValueError(f"Function group '{name}' not mocked. Use mock_function_group() to add it.")examples/object_store/user_report/configs/config_mem.yml (1)
70-71: Add tests to verify tool_names expansion order (get → put → update → delete).Ensure [user_report] expands to namespaced functions in expose/registration order.
Run:
#!/bin/bash rg -nP -C2 'tool_names:\s*\[\s*user_report\s*\]' examples tests rg -nP -C2 '\bexposes:\s*\[\s*get.*put.*update.*delete\s*\]' examples || true # If a test exists, it should resolve to: # ["user_report.get","user_report.put","user_report.update","user_report.delete"] in that order.src/nat/tool/github_tools.py (1)
424-429: Build correct raw URLs (remove refs/heads, handle 1-based lines).raw.githubusercontent.com should be owner/repo/branch/path.
- # The following URL is the raw URL of the file. refs/heads/ always points to the top commit of the branch - raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/refs/heads/{file_metadata.file_path}" + # Build raw URL: owner/repo/branch/path + raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/{file_metadata.file_path}" try: response = await client.get(raw_url) response.raise_for_status()src/nat/builder/workflow_builder.py (1)
1166-1176: Dependency tracking misses FunctionGroupRef/FunctionRef due to no normalization.Membership tests against dict[str, …] fail for ComponentRef objects; also dependency keys should be strings.
- tools = self._workflow_builder.get_tools(tool_names, wrapper_type) - for tool_name in tool_names: - if tool_name in self._workflow_builder._function_groups: - self._dependencies.add_function_group(tool_name) - else: - self._dependencies.add_function(tool_name) + tools = self._workflow_builder.get_tools(tool_names, wrapper_type) + for tool_name in tool_names: + key = str(tool_name) + if key in self._workflow_builder._function_groups: + self._dependencies.add_function_group(key) + else: + self._dependencies.add_function(key)
🧹 Nitpick comments (27)
src/nat/agent/tool_calling_agent/register.py (2)
45-45: Fix typo in description.- max_iterations: int = Field(default=15, description="Number of tool calls before stoping the tool calling agent.") + max_iterations: int = Field(default=15, description="Number of tool calls before stopping the tool calling agent.")
68-71: Tighten error message.Message references llm_name while checking tools; make it tool-centric.
- if not tools: - raise ValueError(f"No tools specified for Tool Calling Agent '{config.llm_name}'") + if not tools: + raise ValueError("No tools specified for Tool Calling Agent workflow.")src/nat/data_models/function.py (1)
51-56: Annotate model validator return type.- @model_validator(mode="after") - def _validate_include_exclude(self): + @model_validator(mode="after") + def _validate_include_exclude(self) -> "FunctionGroupBaseConfig": if self.include and self.exclude: raise ValueError("include and exclude cannot be used together") return selfsrc/nat/experimental/test_time_compute/functions/plan_select_execute_function.py (2)
100-104: Guard missing/absent group dependencies to avoid KeyError/AttributeError.- function_dependencies = builder.get_function_dependencies(config.augmented_fn) - function_used_tools = set(function_dependencies.functions) - for function_group in function_dependencies.function_groups: - function_used_tools.update(builder.get_function_group_dependencies(function_group).functions) + function_dependencies = builder.get_function_dependencies(config.augmented_fn) + function_used_tools = set(function_dependencies.functions) + for function_group in function_dependencies.function_groups: + try: + group_deps = builder.get_function_group_dependencies(function_group) + except KeyError: + group_deps = None + if group_deps and getattr(group_deps, "functions", None): + function_used_tools.update(group_deps.functions)
107-110: Make prompt/tool listing deterministic.Stable ordering improves reproducibility and testing.
- for tool in function_used_tools: + for tool in sorted(function_used_tools): tool_impl = builder.get_function(tool) tool_list += f"- {tool}: {tool_impl.description if hasattr(tool_impl, 'description') else ''}\n"src/nat/data_models/function_dependencies.py (2)
33-39: Serialize sets in sorted order for deterministic JSON.@field_serializer("functions", when_used="json") def serialize_functions(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @@ @field_serializer("function_groups", when_used="json") def serialize_function_groups(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @@ @field_serializer("llms", when_used="json") def serialize_llms(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @@ @field_serializer("embedders", when_used="json") def serialize_embedders(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @@ @field_serializer("memory_clients", when_used="json") def serialize_memory_clients(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @@ @field_serializer("object_stores", when_used="json") def serialize_object_stores(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @@ @field_serializer("retrievers", when_used="json") def serialize_retrievers(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v)Also applies to: 41-59
64-66: Remove unnecessary pylint disable.No dynamic attribute; field exists.
- def add_function_group(self, function_group: str): - self.function_groups.add(function_group) # pylint: disable=no-member + def add_function_group(self, function_group: str): + self.function_groups.add(function_group)examples/object_store/user_report/README.md (2)
20-20: Fix article typo.-And example tool in the NeMo Agent toolkit that makes use of an Object Store to retrieve data. +An example tool in the NeMo Agent toolkit that makes use of an Object Store to retrieve data.
124-128: Fix broken docker-compose links (missing “examples/” path segment).-You can use the [docker-compose.minio.yml](../../deploy/docker-compose.minio.yml) file to start a MinIO server in a local docker container. +You can use the [docker-compose.minio.yml](../../examples/deploy/docker-compose.minio.yml) file to start a MinIO server in a local docker container. @@ -If you want to use a MySQL server, you can use the [docker-compose.mysql.yml](../../deploy/docker-compose.mysql.yml) file to start a MySQL server in a local docker container. +If you want to use a MySQL server, you can use the [docker-compose.mysql.yml](../../examples/deploy/docker-compose.mysql.yml) file to start a MySQL server in a local docker container. @@ -If you want to use a Redis server, you can use the [docker-compose.redis.yml](../../deploy/docker-compose.redis.yml) file to start a Redis server in a local docker container. +If you want to use a Redis server, you can use the [docker-compose.redis.yml](../../examples/deploy/docker-compose.redis.yml) file to start a Redis server in a local docker container.Also applies to: 135-137, 154-157
tests/nat/agent/test_reasoning_agent.py (3)
141-146: Annotate mutable class attributes as ClassVar (ruff RUF012).class FakeDeps: - functions = {"SomeTool"} - function_groups = set() + functions: ClassVar[set[str]] = {"SomeTool"} + function_groups: ClassVar[set[str]] = set()Add import (outside this hunk):
from typing import ClassVar
305-310: Repeat ClassVar annotations for this FakeDeps.class FakeDeps: - functions = {"ToolA", "ToolB"} - function_groups = set() + functions: ClassVar[set[str]] = {"ToolA", "ToolB"} + function_groups: ClassVar[set[str]] = set()
380-385: Repeat ClassVar annotations for empty-set FakeDeps.class FakeDeps: - functions = set() - function_groups = set() + functions: ClassVar[set[str]] = set() + function_groups: ClassVar[set[str]] = set()examples/object_store/user_report/configs/config_mem.yml (1)
68-75: Stub the LLM in example configs for CI.Repo rules require nat_test_llm in YAML. Either switch this example to a stub or add a test variant (e.g., config_mem.test.yml) that uses nat_test_llm with response_seq and delay_ms, and point workflow.llm_name to it during tests.
Example test config snippet:
llms: test_llm: _type: nat_test_llm response_seq: ["ok"] delay_ms: 0 workflow: _type: react_agent tool_names: [user_report] llm_name: test_llm verbose: true handle_parsing_errors: true max_retries: 2examples/object_store/user_report/src/nat_user_report/user_report_tools.py (2)
18-25: Annotate async generator and import AsyncIterator.Conform to public API typing rules.
+from typing import AsyncIterator @@ -@register_function_group(config_type=UserReportConfig) -async def user_report_group(config: UserReportConfig, builder: Builder): +@register_function_group(config_type=UserReportConfig) +async def user_report_group(config: UserReportConfig, builder: Builder) -> AsyncIterator[FunctionGroup]:Also applies to: 46-49
68-80: Align put() behavior with spec (“return a conflict error”).Currently returns a success-like string. Consider raising or returning a structured error to match the description.
- except KeyAlreadyExistsError: - return f"User report for {user_id} with date {date} already exists" + except KeyAlreadyExistsError: + raiseIf raising is undesirable, return a structured error object/string prefixed with “ConflictError”.
src/nat/builder/function.py (2)
16-18: Tighten validation, add return type, and precompile regex (fix Ruff TRY003).Shorten error messages, strip once, and annotate return.
import logging -import re +import re @@ - def add_function(self, + NAME_RE = re.compile(r"^[a-zA-Z0-9_-]+$") + + def add_function(self, name: str, fn: Callable, *, input_schema: type[BaseModel] | None = None, description: str | None = None, - converters: list[Callable] | None = None): + converters: list[Callable[[typing.Any], typing.Any]] | None = None) -> None: @@ - if not name.strip(): - raise ValueError("Function name cannot be empty or blank") - if not re.match(r"^[a-zA-Z0-9_-]+$", name): - raise ValueError(f"Function name can only contain letters, numbers, underscores, and hyphens: {name}") - if name in self._functions: - raise ValueError(f"Function {name} already exists in function group {self._instance_name}") + name = name.strip() + if not name: + raise ValueError("Empty function name.") + if not NAME_RE.match(name): + raise ValueError(f"Invalid function name: {name!r}.") + if name in self._functions: + raise ValueError(f"Duplicate function: {name!r}.")Also applies to: 371-407
431-436: Compact error strings to satisfy Ruff TRY003.Keep semantics; reduce message length.
- missing = set(self._config.exclude) - set(self._functions.keys()) - if missing: - raise ValueError(f"Unknown excluded functions: {sorted(missing)}") + missing = set(self._config.exclude) - set(self._functions.keys()) + if missing: + raise ValueError(f"Unknown excluded: {sorted(missing)}") @@ - missing = set(self._config.include) - set(self._functions.keys()) - if missing: - raise ValueError(f"Unknown included functions: {sorted(missing)}") + missing = set(self._config.include) - set(self._functions.keys()) + if missing: + raise ValueError(f"Unknown included: {sorted(missing)}")Also applies to: 476-501
src/nat/tool/github_tools.py (4)
16-18: Annotate async generators and import AsyncIterator.Required by repo typing rules.
-from typing import Literal +from typing import Literal, AsyncIterator @@ -@register_function_group(config_type=GithubGroupConfig) -async def github_tool(config: GithubGroupConfig, _builder: Builder): +@register_function_group(config_type=GithubGroupConfig) +async def github_tool(config: GithubGroupConfig, _builder: Builder) -> AsyncIterator[FunctionGroup]: @@ -@register_function(config_type=GithubFilesGroupConfig) -async def github_files_tool(config: GithubFilesGroupConfig, _builder: Builder): +@register_function(config_type=GithubFilesGroupConfig) +async def github_files_tool(config: GithubFilesGroupConfig, _builder: Builder) -> AsyncIterator[FunctionInfo]:Also applies to: 151-160, 377-381
172-174: Tighten auth error message and add GitHub API version header.Short message fixes TRY003; adding X-GitHub-Api-Version is recommended.
- if not token: - raise ValueError("No GitHub token found in environment variables. Please set one of the following" - "environment variables: GITHUB_TOKEN, GITHUB_PAT, GH_TOKEN") + if not token: + raise ValueError("Missing GitHub auth token (set GITHUB_TOKEN/GITHUB_PAT/GH_TOKEN).") @@ headers = { "Authorization": f"Bearer {token}", "Accept": "application/vnd.github+json", - "User-Agent": "NeMo-Agent-Toolkit", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "NeMo-Agent-Toolkit", }Also applies to: 176-181
195-203: Encode labels as comma-separated string for list issues.GitHub expects labels CSV, not repeated query params.
- for issue in issues_list.filter_parameters: - params = issue.model_dump(exclude_unset=True, exclude_none=True) + for issue in issues_list.filter_parameters: + params = issue.model_dump(exclude_unset=True, exclude_none=True) + if isinstance(params.get("labels"), list): + params["labels"] = ",".join(params["labels"]) response = await client.get(url, params=params)
359-363: Fix get_pull description (lists PRs, does not fetch files).Clarify user-facing text.
- group.add_function("get_pull", - get_pull, - description="Fetches the files for a particular GitHub pull request" - f"in the repo named {config.repo_name}", - input_schema=GithubGetPullsModelList) + group.add_function("get_pull", + get_pull, + description=f"Lists pull requests in {config.repo_name} with optional filters", + input_schema=GithubGetPullsModelList)src/nat/cli/type_registry.py (1)
186-194: Register/get FunctionGroup API: good structure; shorten error messages (TRY003).Keep behavior; compact strings to satisfy Ruff.
- if (registration.config_type in self._registered_function_groups): - raise ValueError( - f"A function group with the same config type `{registration.config_type}` has already been " - "registered.") + if registration.config_type in self._registered_function_groups: + raise ValueError("Function group already registered for this config type.") @@ - except KeyError as err: - raise KeyError(f"Could not find a registered function group for config `{config_type}`. " - f"Registered configs: {set(self._registered_function_groups.keys())}") from err + except KeyError as err: + raise KeyError("No function group registered for given config type.") from errAlso applies to: 331-333, 499-542
src/nat/builder/workflow_builder.py (5)
246-246: Redundant variable initialization (shadowing).function_instances is initialized twice; drop the first to avoid confusion.
- # Dictionary of function instances - function_instances = dict() ... - function_instances = dict() + # Dictionary of function instances (populated below) + function_instances = dict()Also applies to: 257-257
405-413: Duplicate dependency entries for function groups.You seed dependencies under config.type, then store under name; this leaves a stray key. Seed under the final key.
- self.current_function_group_building = config.type - # Empty set of dependencies for the current function group - self.function_group_dependencies[config.type] = FunctionDependencies() + self.current_function_group_building = config.type + # Seed under the instance key used externally + self.function_group_dependencies[name] = FunctionDependencies() @@ - self.function_group_dependencies[name] = inner_builder.dependencies + self.function_group_dependencies[name] = inner_builder.dependencies
570-586: Add return annotation and preserve traceback details in log.Public API requires return types; also log with exc_info and keep bare raise.
- def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str): + def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any: @@ - except Exception as e: - logger.error("Error fetching tool `%s`: %s", fn_name, e) - raise + except Exception: + logger.error("Error fetching tool `%s`", fn_name, exc_info=True) + raise
604-620: Missing return annotations on framework-dependent getters.These public APIs should declare return types. Use typing.Any for wrapper-dependent returns.
- async def get_llm(self, llm_name: str | LLMRef, wrapper_type: LLMFrameworkEnum | str): + async def get_llm(self, llm_name: str | LLMRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any: @@ - async def get_embedder(self, embedder_name: str | EmbedderRef, wrapper_type: LLMFrameworkEnum | str): + async def get_embedder(self, embedder_name: str | EmbedderRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any: @@ - async def get_retriever(self, + async def get_retriever(self, retriever_name: str | RetrieverRef, wrapper_type: LLMFrameworkEnum | str | None = None): + # Return type depends on wrapper; annotate as Any in the interface if desired.Would you prefer annotating these to concrete protocol types instead of Any (if wrappers implement a shared interface)?
Also applies to: 720-739, 829-851
1178-1184: Normalize dependency key in ChildBuilder.get_tool.Ensure dependencies record string names even when a FunctionRef is passed.
- fn = self._workflow_builder.get_tool(fn_name, wrapper_type) - - self._dependencies.add_function(fn_name) + fn = self._workflow_builder.get_tool(fn_name, wrapper_type) + self._dependencies.add_function(str(fn_name))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
ci/scripts/path_checks.py(1 hunks)docs/source/extend/function-groups.md(1 hunks)docs/source/index.md(2 hunks)docs/source/workflows/function-groups.md(1 hunks)examples/object_store/user_report/README.md(8 hunks)examples/object_store/user_report/configs/config_mem.yml(1 hunks)examples/object_store/user_report/configs/config_mysql.yml(1 hunks)examples/object_store/user_report/configs/config_redis.yml(1 hunks)examples/object_store/user_report/configs/config_s3.yml(1 hunks)examples/object_store/user_report/src/nat_user_report/user_report_tools.py(3 hunks)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py(5 hunks)src/nat/agent/react_agent/register.py(2 hunks)src/nat/agent/reasoning_agent/reasoning_agent.py(1 hunks)src/nat/agent/rewoo_agent/register.py(2 hunks)src/nat/agent/tool_calling_agent/register.py(2 hunks)src/nat/builder/builder.py(5 hunks)src/nat/builder/component_utils.py(4 hunks)src/nat/builder/function.py(3 hunks)src/nat/builder/workflow.py(5 hunks)src/nat/builder/workflow_builder.py(17 hunks)src/nat/cli/register_workflow.py(4 hunks)src/nat/cli/type_registry.py(10 hunks)src/nat/data_models/component.py(2 hunks)src/nat/data_models/component_ref.py(1 hunks)src/nat/data_models/config.py(10 hunks)src/nat/data_models/function.py(2 hunks)src/nat/data_models/function_dependencies.py(3 hunks)src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py(1 hunks)src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py(1 hunks)src/nat/profiler/utils.py(2 hunks)src/nat/tool/github_tools.py(1 hunks)src/nat/tool/github_tools/create_github_commit.py(0 hunks)src/nat/tool/github_tools/create_github_issue.py(0 hunks)src/nat/tool/github_tools/create_github_pr.py(0 hunks)src/nat/tool/github_tools/get_github_file.py(0 hunks)src/nat/tool/github_tools/get_github_issue.py(0 hunks)src/nat/tool/github_tools/get_github_pr.py(0 hunks)src/nat/tool/github_tools/update_github_issue.py(0 hunks)src/nat/tool/register.py(1 hunks)tests/nat/agent/test_reasoning_agent.py(3 hunks)tests/nat/builder/test_builder.py(16 hunks)tests/nat/front_ends/mcp/test_mcp_custom_routes.py(2 hunks)
💤 Files with no reviewable changes (7)
- src/nat/tool/github_tools/create_github_pr.py
- src/nat/tool/github_tools/get_github_file.py
- src/nat/tool/github_tools/create_github_commit.py
- src/nat/tool/github_tools/update_github_issue.py
- src/nat/tool/github_tools/create_github_issue.py
- src/nat/tool/github_tools/get_github_issue.py
- src/nat/tool/github_tools/get_github_pr.py
✅ Files skipped from review due to trivial changes (1)
- docs/source/workflows/function-groups.md
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/nat/front_ends/mcp/test_mcp_custom_routes.py
- docs/source/index.md
- src/nat/agent/reasoning_agent/reasoning_agent.py
- ci/scripts/path_checks.py
- examples/object_store/user_report/configs/config_s3.yml
- src/nat/data_models/component_ref.py
- src/nat/agent/react_agent/register.py
- src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py
- src/nat/profiler/utils.py
- src/nat/cli/register_workflow.py
- src/nat/tool/register.py
- docs/source/extend/function-groups.md
- src/nat/builder/builder.py
- examples/object_store/user_report/configs/config_mysql.yml
- examples/object_store/user_report/configs/config_redis.yml
- src/nat/builder/workflow.py
- src/nat/agent/rewoo_agent/register.py
- src/nat/builder/component_utils.py
🧰 Additional context used
📓 Path-based instructions (15)
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pysrc/nat/agent/tool_calling_agent/register.pysrc/nat/builder/function.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Core functionality under src/nat should prioritize backward compatibility when changed
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pysrc/nat/agent/tool_calling_agent/register.pysrc/nat/builder/function.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pysrc/nat/agent/tool_calling_agent/register.pysrc/nat/builder/function.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pysrc/nat/agent/tool_calling_agent/register.pytests/nat/builder/test_builder.pysrc/nat/builder/function.pyexamples/object_store/user_report/src/nat_user_report/user_report_tools.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pyexamples/object_store/user_report/README.mdsrc/nat/agent/tool_calling_agent/register.pytests/nat/builder/test_builder.pysrc/nat/builder/function.pyexamples/object_store/user_report/src/nat_user_report/user_report_tools.pyexamples/object_store/user_report/configs/config_mem.ymlsrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pyexamples/object_store/user_report/README.mdsrc/nat/agent/tool_calling_agent/register.pytests/nat/builder/test_builder.pysrc/nat/builder/function.pyexamples/object_store/user_report/src/nat_user_report/user_report_tools.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.pytests/nat/agent/test_reasoning_agent.py
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pysrc/nat/agent/tool_calling_agent/register.pytests/nat/builder/test_builder.pysrc/nat/builder/function.pyexamples/object_store/user_report/src/nat_user_report/user_report_tools.pyexamples/object_store/user_report/configs/config_mem.ymlsrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.pytests/nat/agent/test_reasoning_agent.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.pysrc/nat/cli/type_registry.pysrc/nat/data_models/component.pysrc/nat/data_models/function_dependencies.pyexamples/object_store/user_report/README.mdsrc/nat/agent/tool_calling_agent/register.pytests/nat/builder/test_builder.pysrc/nat/builder/function.pyexamples/object_store/user_report/src/nat_user_report/user_report_tools.pyexamples/object_store/user_report/configs/config_mem.ymlsrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pypackages/nvidia_nat_test/src/nat/test/tool_test_runner.pysrc/nat/data_models/function.pysrc/nat/tool/github_tools.pytests/nat/agent/test_reasoning_agent.py
**/README.{md,ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Each documentation README must follow the NeMo Agent toolkit naming rules and must not use deprecated names
Files:
examples/object_store/user_report/README.md
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/object_store/user_report/README.mdexamples/object_store/user_report/src/nat_user_report/user_report_tools.pyexamples/object_store/user_report/configs/config_mem.yml
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
tests/nat/builder/test_builder.pytests/nat/agent/test_reasoning_agent.py
**/configs/**/*.y?(a)ml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must live in a neighbouring configs/ directory
Files:
examples/object_store/user_report/configs/config_mem.yml
**/*.{yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
In workflow/config YAML, set llms.._type: nat_test_llm to stub responses.
Files:
examples/object_store/user_report/configs/config_mem.yml
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
🧠 Learnings (8)
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are sibling classes that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other and the order doesn't matter for classification purposes.
Applied to files:
src/nat/cli/type_registry.pytests/nat/builder/test_builder.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are separate class hierarchies that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other.
Applied to files:
src/nat/cli/type_registry.pytests/nat/builder/test_builder.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
examples/object_store/user_report/README.mdtests/nat/builder/test_builder.pysrc/nat/builder/function.pyexamples/object_store/user_report/src/nat_user_report/user_report_tools.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.
Applied to files:
tests/nat/builder/test_builder.pysrc/nat/builder/function.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None, so there are no None-handling concerns for this field.
Applied to files:
tests/nat/builder/test_builder.pysrc/nat/builder/function.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:30:12.178Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/data_models/function.py:39-45
Timestamp: 2025-08-28T14:30:12.178Z
Learning: In the NAT codebase, stricter validation of function names (like checking for empty/whitespace-only strings) in FunctionGroupBaseConfig.validate_expose is unnecessary because workflow parsing and validation already handle these validation concerns at a different layer in the system.
Applied to files:
tests/nat/builder/test_builder.pysrc/nat/builder/function.pysrc/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/data_models/function.py
📚 Learning: 2025-08-28T14:29:34.897Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/workflow_builder.py:373-410
Timestamp: 2025-08-28T14:29:34.897Z
Learning: ComponentRef objects (like FunctionGroupRef) automatically convert to strings when used as dictionary keys in Python, so explicit str() conversions are not required when using them as dict keys.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T23:22:41.742Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-28T23:22:41.742Z
Learning: Applies to **/*.py : When re-raising exceptions, use bare `raise` and log with logger.error(), not logger.exception()
Applied to files:
src/nat/builder/workflow_builder.py
🧬 Code graph analysis (13)
src/nat/experimental/test_time_compute/functions/plan_select_execute_function.py (3)
src/nat/builder/workflow_builder.py (4)
get_function_dependencies(523-526)get_function_dependencies(1305-1306)get_function_group_dependencies(529-532)get_function_group_dependencies(1309-1310)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (2)
get_function_dependencies(284-286)get_function_group_dependencies(288-290)src/nat/builder/builder.py (2)
get_function_dependencies(276-277)get_function_group_dependencies(280-281)
src/nat/cli/type_registry.py (8)
src/nat/builder/function.py (1)
FunctionGroup(350-512)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/builder.py (2)
Builder(64-281)get_function_group(79-80)src/nat/cli/register_workflow.py (1)
register_function_group(180-211)src/nat/builder/workflow_builder.py (2)
get_function_group(469-475)get_function_group(1137-1143)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (1)
get_function_group(151-157)src/nat/data_models/component.py (1)
ComponentEnum(22-43)src/nat/data_models/common.py (1)
static_type(154-155)
src/nat/data_models/function_dependencies.py (3)
src/nat/builder/workflow_builder.py (2)
add_function_group(435-457)add_function_group(1124-1125)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (1)
add_function_group(147-149)src/nat/builder/builder.py (1)
add_function_group(71-72)
src/nat/agent/tool_calling_agent/register.py (1)
src/nat/data_models/component_ref.py (2)
FunctionGroupRef(105-113)FunctionRef(94-102)
tests/nat/builder/test_builder.py (6)
src/nat/builder/builder.py (6)
Builder(64-281)add_function(67-68)add_function_group(71-72)get_function_group(79-80)get_function_group_config(94-95)get_function(75-76)src/nat/builder/function.py (8)
Function(45-300)FunctionGroup(350-512)add_function(371-411)get_accessible_functions(437-459)get_all_functions(503-512)get_included_functions(481-501)get_excluded_functions(461-479)get_config(413-422)src/nat/cli/register_workflow.py (2)
register_function_group(180-211)register_function(148-177)src/nat/cli/type_registry.py (4)
register_function_group(499-515)register_function(477-485)get_function_group(517-533)get_function(487-493)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (12)
add_function(421-432)add_function(1120-1121)WorkflowBuilder(141-1104)add_function_group(435-457)add_function_group(1124-1125)get_function_group(469-475)get_function_group(1137-1143)get_function_group_config(487-493)get_function_group_config(1150-1151)get_function(460-466)get_function(1128-1134)dependencies(1116-1117)
src/nat/builder/function.py (2)
src/nat/data_models/function.py (3)
EmptyFunctionConfig(58-59)FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/context.py (1)
Context(93-277)
examples/object_store/user_report/src/nat_user_report/user_report_tools.py (6)
examples/object_store/user_report/tests/test_user_report_tool.py (2)
builder(33-49)object_store(53-55)src/nat/builder/builder.py (3)
Builder(64-281)get_object_store_client(167-168)add_function(67-68)src/nat/builder/function.py (2)
FunctionGroup(350-512)add_function(371-411)src/nat/cli/register_workflow.py (1)
register_function_group(180-211)src/nat/data_models/function.py (1)
FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (4)
get_object_store_client(796-800)get_object_store_client(1250-1258)add_function(421-432)add_function(1120-1121)
src/nat/data_models/config.py (3)
src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)src/nat/cli/type_registry.py (3)
get(1049-1050)get_registered_function_groups(535-541)compute_annotation(997-1041)src/nat/data_models/common.py (2)
TypedBaseModel(93-168)discriminator(162-168)
src/nat/builder/workflow_builder.py (8)
src/nat/builder/function.py (5)
FunctionGroup(350-512)add_function(371-411)Function(45-300)get_included_functions(481-501)get_accessible_functions(437-459)src/nat/data_models/component_ref.py (12)
FunctionGroupRef(105-113)FunctionRef(94-102)component_group(69-76)component_group(90-91)component_group(101-102)component_group(112-113)component_group(123-124)component_group(134-135)component_group(145-146)component_group(156-157)component_group(167-168)component_group(178-179)src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)src/nat/data_models/function_dependencies.py (5)
FunctionDependencies(21-80)add_function(61-62)add_function_group(64-65)add_object_store(76-77)add_retriever(79-80)src/nat/data_models/config.py (1)
Config(239-426)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (11)
get_function_group(151-157)add_function(131-133)add_function_group(147-149)get_function(135-141)get_function_config(143-145)get_function_group_config(159-161)get_function_group_dependencies(288-290)get_tools(179-181)get_tool(183-185)add_object_store(258-260)add_retriever(241-243)src/nat/builder/builder.py (11)
get_function_group(79-80)add_function(67-68)add_function_group(71-72)get_function(75-76)get_function_config(90-91)get_function_group_config(94-95)get_function_group_dependencies(280-281)get_tools(110-113)get_tool(116-117)add_object_store(157-158)add_retriever(217-218)src/nat/profiler/decorators/framework_wrapper.py (1)
chain_wrapped_build_fn(108-128)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (5)
src/nat/builder/builder.py (6)
Builder(64-281)add_function_group(71-72)get_function_group(79-80)get_function_group_config(94-95)get_tools(110-113)get_function_group_dependencies(280-281)src/nat/builder/function.py (2)
Function(45-300)FunctionGroup(350-512)src/nat/data_models/function.py (2)
FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)src/nat/builder/workflow_builder.py (10)
add_function_group(435-457)add_function_group(1124-1125)get_function_group(469-475)get_function_group(1137-1143)get_function_group_config(487-493)get_function_group_config(1150-1151)get_tools(535-568)get_tools(1166-1175)get_function_group_dependencies(529-532)get_function_group_dependencies(1309-1310)src/nat/data_models/function_dependencies.py (2)
add_function_group(64-65)FunctionDependencies(21-80)
src/nat/data_models/function.py (1)
src/nat/data_models/common.py (2)
TypedBaseModel(93-168)BaseModelRegistryTag(88-90)
src/nat/tool/github_tools.py (4)
src/nat/builder/builder.py (1)
Builder(64-281)src/nat/builder/function.py (2)
FunctionGroup(350-512)add_function(371-411)src/nat/builder/function_info.py (2)
FunctionInfo(290-625)from_fn(552-625)src/nat/data_models/function.py (2)
FunctionBaseConfig(26-27)FunctionGroupBaseConfig(30-55)
tests/nat/agent/test_reasoning_agent.py (2)
src/nat/builder/workflow_builder.py (4)
get_function_dependencies(523-526)get_function_dependencies(1305-1306)get_function_group_dependencies(529-532)get_function_group_dependencies(1309-1310)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (2)
get_function_dependencies(284-286)get_function_group_dependencies(288-290)
🪛 Ruff (0.12.2)
src/nat/cli/type_registry.py
509-511: Avoid specifying long messages outside the exception class
(TRY003)
532-533: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/builder/test_builder.py
292-292: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
413-413: Unused function argument: config
(ARG001)
417-417: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/builder/function.py
402-402: Avoid specifying long messages outside the exception class
(TRY003)
404-404: Avoid specifying long messages outside the exception class
(TRY003)
406-406: Avoid specifying long messages outside the exception class
(TRY003)
433-433: Avoid specifying long messages outside the exception class
(TRY003)
478-478: Avoid specifying long messages outside the exception class
(TRY003)
500-500: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/builder/workflow_builder.py
415-416: Prefer TypeError exception for invalid type
(TRY004)
415-416: Avoid specifying long messages outside the exception class
(TRY003)
426-426: Avoid specifying long messages outside the exception class
(TRY003)
440-440: Avoid specifying long messages outside the exception class
(TRY003)
451-451: Avoid specifying long messages outside the exception class
(TRY003)
464-464: Avoid specifying long messages outside the exception class
(TRY003)
473-473: Avoid specifying long messages outside the exception class
(TRY003)
482-482: Avoid specifying long messages outside the exception class
(TRY003)
491-491: Avoid specifying long messages outside the exception class
(TRY003)
545-545: Avoid specifying long messages outside the exception class
(TRY003)
550-550: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_test/src/nat/test/tool_test_runner.py
157-157: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Unused method argument: name
(ARG002)
179-179: Unused method argument: tool_names
(ARG002)
179-179: Unused method argument: wrapper_type
(ARG002)
288-288: Unused method argument: fn_name
(ARG002)
src/nat/data_models/function.py
48-48: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
src/nat/tool/github_tools.py
66-66: Avoid specifying long messages outside the exception class
(TRY003)
173-174: Avoid specifying long messages outside the exception class
(TRY003)
266-266: Avoid specifying long messages outside the exception class
(TRY003)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
tests/nat/agent/test_reasoning_agent.py
141-141: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
142-142: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
305-305: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
380-380: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
381-381: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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
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 (2)
src/nat/builder/workflow_builder.py (2)
570-586: Add return type to get_tool to meet public API typing contract.
Missing return annotation can fail pyright and violates repo guidelines.- def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str): + def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any:
1178-1185: Normalize dependency key and add return annotation in ChildBuilder.get_tool.
Ensures consistent dependency tracking and satisfies typing rules.- def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str): + def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any: # If a function tries to get another function as a tool, we assume it uses it fn = self._workflow_builder.get_tool(fn_name, wrapper_type) - - self._dependencies.add_function(fn_name) + self._dependencies.add_function(str(fn_name)) return fn
♻️ Duplicate comments (2)
src/nat/builder/workflow_builder.py (2)
406-413: Normalize function-group dependency key to instance name; avoid stray entry.
Seed and store under name only (not config.type) to prevent duplicate keys.- # Empty set of dependencies for the current function group - self.function_group_dependencies[config.type] = FunctionDependencies() + # Empty set of dependencies for the current function group (by instance key) + self.function_group_dependencies[name] = FunctionDependencies() @@ - self.function_group_dependencies[name] = inner_builder.dependencies + self.function_group_dependencies[name] = inner_builder.dependencies
1165-1175: Fix FunctionGroupRef membership check; normalize keys before recording deps.
Currently misses groups and records as functions.- tools = self._workflow_builder.get_tools(tool_names, wrapper_type) - for tool_name in tool_names: - if tool_name in self._workflow_builder._function_groups: - self._dependencies.add_function_group(tool_name) - else: - self._dependencies.add_function(tool_name) + tools = self._workflow_builder.get_tools(tool_names, wrapper_type) + for tool_name in tool_names: + key = str(tool_name) + if key in self._workflow_builder._function_groups: + self._dependencies.add_function_group(key) + else: + self._dependencies.add_function(key) return tools
🧹 Nitpick comments (6)
src/nat/builder/workflow_builder.py (6)
241-262: Remove redundant initialization of function_instances.
Declaring function_instances twice is confusing; keep the latter.- # Dictionary of function instances - function_instances = dict() @@ - function_configs = dict() - function_instances = dict() + function_configs = dict() + function_instances = dict()
415-416: Prefer TypeError for invalid type from builder.
Aligns with Ruff TRY004 and idiomatic usage.- if not isinstance(build_result, FunctionGroup): - raise ValueError("Expected a FunctionGroup object to be returned from the function group builder. " + if not isinstance(build_result, FunctionGroup): + raise TypeError("Expected a FunctionGroup object to be returned from the function group builder. " f"Got {type(build_result)}")
435-457: Avoid recomputing included functions; compute once.
Reduces duplicate calls and keeps a consistent snapshot.- # If the function group exposes functions, add them to the global function registry - # If the function group exposes functions, record and add them to the registry - for k in build_result.instance.get_included_functions(): + # If the function group exposes functions, record and add them to the registry + included = build_result.instance.get_included_functions() + for k in included: if k in self._functions: raise ValueError(f"Exposed function `{k}` from group `{name}` conflicts with an existing function") - self._functions.update({ - k: ConfiguredFunction(config=v.config, instance=v) - for k, v in build_result.instance.get_included_functions().items() - }) + self._functions.update({k: ConfiguredFunction(config=v.config, instance=v) for k, v in included.items()})
529-533: Double-check dependency keys are only instance names.
Given seeding under config.type earlier, ensure callers don’t rely on that stray key.
535-568: Tweak logging when re-raising to avoid duplicate stack traces.
Use logger.error without exc_info=True since you re-raise.- except Exception: - logger.error("Error fetching tool `%s`", fn_name, exc_info=True) - raise + except Exception: + logger.error("Error fetching tool `%s`", fn_name) + raise
1056-1056: Grammar nit: “an object store.”- # Instantiate a object store client + # Instantiate an object store client
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nat/builder/workflow_builder.py(17 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
src/**/*.py: All importable Python code must live under src/
All public APIs in src/ require Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise with a period; surround code entities with backticks
Files:
src/nat/builder/workflow_builder.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Core functionality under src/nat should prioritize backward compatibility when changed
Files:
src/nat/builder/workflow_builder.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/builder/workflow_builder.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
Files:
src/nat/builder/workflow_builder.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
src/nat/builder/workflow_builder.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
src/nat/builder/workflow_builder.py
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/builder/workflow_builder.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/builder/workflow_builder.py
🧠 Learnings (7)
📚 Learning: 2025-08-28T14:30:12.178Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/data_models/function.py:39-45
Timestamp: 2025-08-28T14:30:12.178Z
Learning: In the NAT codebase, stricter validation of function names (like checking for empty/whitespace-only strings) in FunctionGroupBaseConfig.validate_expose is unnecessary because workflow parsing and validation already handle these validation concerns at a different layer in the system.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T14:04:37.688Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: examples/object_store/user_report/configs/config_mysql.yml:38-48
Timestamp: 2025-08-28T14:04:37.688Z
Learning: In FunctionGroupBaseConfig, the expose field is defined with `expose: list[str] = Field(default_factory=list, ...)`, which means it defaults to an empty list rather than None. The get_exposed_functions() method directly accesses self._config.expose without None checks because it's guaranteed to be a list.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T14:29:34.897Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/workflow_builder.py:373-410
Timestamp: 2025-08-28T14:29:34.897Z
Learning: ComponentRef objects (like FunctionGroupRef) automatically convert to strings when used as dictionary keys in Python, so explicit str() conversions are not required when using them as dict keys.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T23:22:41.742Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-28T23:22:41.742Z
Learning: Applies to **/*.py : When re-raising exceptions, use bare `raise` and log with logger.error(), not logger.exception()
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T20:03:37.650Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/front_ends/mcp/mcp_front_end_plugin_worker.py:0-0
Timestamp: 2025-08-28T20:03:37.650Z
Learning: Function groups in NAT automatically namespace their function names with the group name (e.g., "user_report.get", "user_report.put"), making function name collisions impossible by design. No collision detection is needed when merging functions from function groups.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are sibling classes that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other and the order doesn't matter for classification purposes.
Applied to files:
src/nat/builder/workflow_builder.py
📚 Learning: 2025-08-28T14:28:34.779Z
Learnt from: willkill07
PR: NVIDIA/NeMo-Agent-Toolkit#684
File: src/nat/builder/component_utils.py:110-114
Timestamp: 2025-08-28T14:28:34.779Z
Learning: FunctionGroupBaseConfig and FunctionBaseConfig are separate class hierarchies that both inherit from (TypedBaseModel, BaseModelRegistryTag). FunctionGroupBaseConfig does not inherit from FunctionBaseConfig, so isinstance checks between them won't interfere with each other.
Applied to files:
src/nat/builder/workflow_builder.py
🧬 Code graph analysis (1)
src/nat/builder/workflow_builder.py (9)
src/nat/builder/function.py (5)
FunctionGroup(350-512)get_included_functions(481-501)add_function(371-411)Function(45-300)get_accessible_functions(437-459)src/nat/data_models/component_ref.py (12)
FunctionGroupRef(105-113)FunctionRef(94-102)component_group(69-76)component_group(90-91)component_group(101-102)component_group(112-113)component_group(123-124)component_group(134-135)component_group(145-146)component_group(156-157)component_group(167-168)component_group(178-179)src/nat/data_models/function.py (2)
FunctionGroupBaseConfig(30-55)FunctionBaseConfig(26-27)src/nat/data_models/function_dependencies.py (5)
FunctionDependencies(21-80)add_function(61-62)add_function_group(64-65)add_object_store(76-77)add_retriever(79-80)src/nat/data_models/config.py (1)
Config(239-426)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (11)
get_function_group(151-157)add_function(131-133)add_function_group(147-149)get_function(135-141)get_function_config(143-145)get_function_group_config(159-161)get_function_group_dependencies(288-290)get_tools(179-181)get_tool(183-185)add_object_store(258-260)add_retriever(241-243)src/nat/builder/builder.py (11)
get_function_group(79-80)add_function(67-68)add_function_group(71-72)get_function(75-76)get_function_config(90-91)get_function_group_config(94-95)get_function_group_dependencies(280-281)get_tools(110-113)get_tool(116-117)add_object_store(157-158)add_retriever(217-218)src/nat/cli/type_registry.py (2)
get_function_group(517-533)get_function(487-493)src/nat/profiler/decorators/framework_wrapper.py (1)
chain_wrapped_build_fn(108-128)
🪛 Ruff (0.12.2)
src/nat/builder/workflow_builder.py
415-416: Prefer TypeError exception for invalid type
(TRY004)
415-416: Avoid specifying long messages outside the exception class
(TRY003)
426-426: Avoid specifying long messages outside the exception class
(TRY003)
440-440: Avoid specifying long messages outside the exception class
(TRY003)
451-451: Avoid specifying long messages outside the exception class
(TRY003)
464-464: Avoid specifying long messages outside the exception class
(TRY003)
473-473: Avoid specifying long messages outside the exception class
(TRY003)
482-482: Avoid specifying long messages outside the exception class
(TRY003)
491-491: Avoid specifying long messages outside the exception class
(TRY003)
545-545: Avoid specifying long messages outside the exception class
(TRY003)
550-550: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (12)
src/nat/builder/workflow_builder.py (12)
19-19: LGTM: imports add required typing and FunctionGroup symbols.Also applies to: 21-21, 36-36, 48-48, 59-59
93-97: LGTM: ConfiguredFunctionGroup dataclass is clear and consistent with others.
159-159: LGTM: state added for function groups and deps.Also applies to: 176-176, 178-178
265-266: LGTM: wiring function_groups into Config and Workflow.Also applies to: 300-301
460-466: LGTM: FunctionRef normalization in get_function.
469-476: LGTM: FunctionGroupRef normalization in get_function_group.
479-485: LGTM: FunctionRef normalization in get_function_config.
487-494: LGTM: FunctionGroupRef normalization in get_function_group_config.
1123-1126: LGTM: ChildBuilder delegates add_function_group.
1136-1144: LGTM: ChildBuilder get_function_group tracks dependency.
1149-1152: LGTM: ChildBuilder get_function_group_config delegation.
1308-1311: LGTM: ChildBuilder forwards get_function_group_dependencies.
lvojtku
left a comment
lvojtku
left a comment
There was a problem hiding this comment.
added some comments but approved.
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Will Killian <2007799+willkill07@users.noreply.github.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
willkill07
force-pushed
the
wkk_function-groups
branch
from
c664db5 to
6150fa6
Compare
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 (1)
tests/nat/builder/test_builder.py (1)
893-934: Fix shadowed variable and tautological assertion in test_built_config.workflow_config is reused, causing
assert workflow_config.workflow == workflow_config.workflowto be a no-op.Apply:
- general_config = GeneralConfig() - function_config = FunctionReturningFunctionConfig() - workflow_config = FunctionReturningFunctionConfig() + general_config = GeneralConfig() + function_config = FunctionReturningFunctionConfig() + workflow_cfg = FunctionReturningFunctionConfig() @@ - await builder.set_workflow(workflow_config) + await builder.set_workflow(workflow_cfg) @@ - workflow_config = workflow.config - - assert workflow_config.general == general_config - assert workflow_config.functions == {"function1": function_config} - assert workflow_config.workflow == workflow_config.workflow - assert workflow_config.llms == {"llm1": llm_config} - assert workflow_config.embedders == {"embedder1": embedder_config} - assert workflow_config.memory == {"memory1": memory_config} - assert workflow_config.retrievers == {"retriever1": retriever_config} - assert workflow_config.object_stores == {"object_store1": object_store_config} - assert workflow_config.ttc_strategies == {"ttc_strategy": ttc_config} + built_cfg = workflow.config + + assert built_cfg.general == general_config + assert built_cfg.functions == {"function1": function_config} + assert built_cfg.workflow == workflow_cfg + assert built_cfg.llms == {"llm1": llm_config} + assert built_cfg.embedders == {"embedder1": embedder_config} + assert built_cfg.memory == {"memory1": memory_config} + assert built_cfg.retrievers == {"retriever1": retriever_config} + assert built_cfg.object_stores == {"object_store1": object_store_config} + assert built_cfg.ttc_strategies == {"ttc_strategy": ttc_config}
♻️ Duplicate comments (19)
src/nat/data_models/function_dependencies.py (1)
33-35: Deterministic JSON: sort all set serializers.Returning list(v) yields nondeterministic order; sort for stable outputs and easier testing/caching. Apply to all serializers for consistency.
@field_serializer("functions", when_used="json") def serialize_functions(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @field_serializer("function_groups", when_used="json") def serialize_function_groups(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @field_serializer("llms", when_used="json") def serialize_llms(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @field_serializer("embedders", when_used="json") def serialize_embedders(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @field_serializer("memory_clients", when_used="json") def serialize_memory_clients(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @field_serializer("object_stores", when_used="json") def serialize_object_stores(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v) @field_serializer("retrievers", when_used="json") def serialize_retrievers(self, v: set[str]) -> list[str]: - return list(v) + return sorted(v)Also applies to: 37-40, 41-59
examples/object_store/user_report/configs/config_s3.yml (2)
37-41: Duplicate object_store setting — remove the front_end one.Group-level object_store is authoritative; keeping general.front_end.object_store risks schema validation failures.
general: front_end: _type: fastapi - object_store: report_object_store cors: allow_origins: ['*']
73-74: Ensure tests cover tool_names group expansion and ordering.Add/verify a test that [user_report] expands to ["user_report.get","user_report.put","user_report.update","user_report.delete"] in that order.
examples/object_store/user_report/configs/config_mem.yml (1)
70-71: Test group expansion/order for tool_names.Same as other configs: assert expansion and exact order.
src/nat/builder/component_utils.py (1)
261-263: Back-compat: guard missing attrs in older Configs.Direct access to config.function_groups breaks older configs. Use getattr(..., {}) consistently.
- total_node_count = (len(config.embedders) + len(config.functions) + len(config.function_groups) + len(config.llms) + - len(config.memory) + len(config.object_stores) + len(config.retrievers) + - len(config.ttc_strategies) + len(config.authentication) + 1) # +1 for the workflow + total_node_count = ( + len(getattr(config, "embedders", {})) + + len(getattr(config, "functions", {})) + + len(getattr(config, "function_groups", {})) + + len(getattr(config, "llms", {})) + + len(getattr(config, "memory", {})) + + len(getattr(config, "object_stores", {})) + + len(getattr(config, "retrievers", {})) + + len(getattr(config, "ttc_strategies", {})) + + len(getattr(config, "authentication", {})) + + 1 # +1 for the workflow + )Additionally, harden config_to_dependency_objects similarly (outside changed hunk):
# In config_to_dependency_objects(...) component_dict = getattr(config, group.value, {})src/nat/tool/github_tools.py (2)
195-203: Fix labels filter encoding for GitHub issues API.GitHub expects a comma-separated labels string, not repeated params.
async def get_issue(issues_list: GithubGetIssueModelList) -> str: url = f"https://api.github.com/repos/{config.repo_name}/issues" results = [] for issue in issues_list.filter_parameters: - params = issue.model_dump(exclude_unset=True, exclude_none=True) + params = issue.model_dump(exclude_unset=True, exclude_none=True) + if isinstance(params.get("labels"), list): + params["labels"] = ",".join(params["labels"]) response = await client.get(url, params=params) response.raise_for_status() results.append(response.json()) return json.dumps(results)
424-431: Build correct raw.githubusercontent.com URL (remove refs/heads).Current URL is invalid and breaks file retrieval.
- # The following URL is the raw URL of the file. refs/heads/ always points to the top commit of the branch - raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/refs/heads/{file_metadata.file_path}" + # Build raw URL: https://raw.githubusercontent.com/{owner}/{repo}/{branch}/{path} + raw_url = f"https://raw.githubusercontent.com/{file_metadata.repo_path}/{file_metadata.file_path}" try: response = await client.get(raw_url) response.raise_for_status()src/nat/data_models/function.py (1)
44-50: Preserve caller order for include/exclude (don’t sort).Sorting alters user-specified exposure order and can change tool resolution. Keep uniqueness check only.
@field_validator("include", "exclude") @classmethod def _validate_fields_include_exclude(cls, value: list[str]) -> list[str]: if len(set(value)) != len(value): raise ValueError("Function names must be unique") - return sorted(value) + return valuesrc/nat/agent/tool_calling_agent/register.py (1)
39-40: Clarifytool_namesdescription to mention groups.Matches the new accepted types and avoids confusion.
- tool_names: list[FunctionRef | FunctionGroupRef] = Field( - default_factory=list, description="The list of tools to provide to the tool calling agent.") + tool_names: list[FunctionRef | FunctionGroupRef] = Field( + default_factory=list, + description="The list of functions and/or function groups to provide to the tool calling agent.", + )tests/nat/agent/test_reasoning_agent.py (3)
141-146: Annotate mutable class attrs with ClassVar (RUF012).Avoids instance-level mutation and silences ruff.
- class FakeDeps: - functions = {"SomeTool"} - function_groups = set() + from typing import ClassVar + class FakeDeps: + functions: ClassVar[set[str]] = {"SomeTool"} + function_groups: ClassVar[set[str]] = set()If
from typing import ClassVaralready exists at file top, drop the inline import here.
305-310: Repeat ClassVar annotations here.- class FakeDeps: - functions = {"ToolA", "ToolB"} - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = {"ToolA", "ToolB"} + function_groups: ClassVar[set[str]] = set()
380-385: Repeat ClassVar annotations for empty-set case.- class FakeDeps: - functions = set() - function_groups = set() + class FakeDeps: + functions: ClassVar[set[str]] = set() + function_groups: ClassVar[set[str]] = set()packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (5)
20-20: Import missing types to match Builder API.Aligns signatures and quiets type-checkers.
-from collections.abc import Sequence +from collections.abc import Sequence +from nat.builder.framework_enum import LLMFrameworkEnum +from nat.data_models.component_ref import FunctionRef, FunctionGroupRef
147-150: Widenadd_function_groupsignature (match abstract base expectations across codebase).- async def add_function_group(self, name: str, config: FunctionGroupBaseConfig) -> FunctionGroup: + async def add_function_group(self, name: str | FunctionGroupRef, config: FunctionGroupBaseConfig) -> FunctionGroup:
151-158: AcceptFunctionGroupRefand index mocks bystr(name); return spec’ed mock.- def get_function_group(self, name: str) -> FunctionGroup: + def get_function_group(self, name: str | FunctionGroupRef) -> FunctionGroup: """Return a mock function group if one is configured.""" - if name in self._mocks: - mock_fn_group = MagicMock(spec=FunctionGroup) - mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[name]) + key = str(name) + if key in self._mocks: + mock_fn_group = MagicMock(spec=FunctionGroup) + mock_fn_group.ainvoke = AsyncMock(return_value=self._mocks[key]) return mock_fn_group raise ValueError(f"Function group '{name}' not mocked. Use mock_function_group() to add it.")
159-162: Match config getter signature with wider ref type.- def get_function_group_config(self, name: str) -> FunctionGroupBaseConfig: + def get_function_group_config(self, name: str | FunctionGroupRef) -> FunctionGroupBaseConfig: """Mock implementation.""" return FunctionGroupBaseConfig()
179-182: Broadenget_toolssignature to accept FunctionRefs/GroupRefs; type wrapper.- def get_tools(self, tool_names: Sequence[str], wrapper_type): + def get_tools( + self, + tool_names: Sequence[str | FunctionRef | FunctionGroupRef], + wrapper_type: LLMFrameworkEnum | str, + ) -> list[typing.Any]: """Mock implementation.""" - return [] + return []docs/source/workflows/function-groups.md (1)
52-54: FixBuilderimport path in snippet.
Builderlives innat.builder.builder.-from nat.builder.workflow_builder import Builder +from nat.builder.builder import Buildersrc/nat/builder/workflow_builder.py (1)
1165-1175: Dependency tracking bug for FunctionGroupRef inputs.Membership checks use raw values; FunctionGroupRef won't match string keys, so group deps aren’t recorded.
@@ - tools = self._workflow_builder.get_tools(tool_names, wrapper_type) - for tool_name in tool_names: - if tool_name in self._workflow_builder._function_groups: - self._dependencies.add_function_group(tool_name) - else: - self._dependencies.add_function(tool_name) + tools = self._workflow_builder.get_tools(tool_names, wrapper_type) + for tool_name in tool_names: + key = str(tool_name) + if key in self._workflow_builder._function_groups: + self._dependencies.add_function_group(key) + else: + self._dependencies.add_function(key) return tools
🧹 Nitpick comments (25)
src/nat/data_models/function_dependencies.py (1)
64-66: Remove unnecessary pylint disable; add a brief docstring.The attribute exists; the suppression is noise. Consider a one-liner docstring to meet public API docs.
- def add_function_group(self, function_group: str): - self.function_groups.add(function_group) # pylint: disable=no-member + def add_function_group(self, function_group: str): + """Add a function group dependency by name.""" + self.function_groups.add(function_group)examples/object_store/user_report/configs/config_mem.yml (1)
34-38: Remove duplicate object_store from front_end; group-level is the source of truth.general: front_end: _type: fastapi - object_store: report_object_store cors: allow_origins: ['*']src/nat/tool/github_tools.py (2)
176-181: Send recommended GitHub headers (API version).Add X-GitHub-Api-Version for better forward compatibility.
headers = { "Authorization": f"Bearer {token}", "Accept": "application/vnd.github+json", "User-Agent": "NeMo-Agent-Toolkit", + "X-GitHub-Api-Version": "2022-11-28", }
184-216: Add concise Google-style docstrings to exposed handlers.These are public surface via the function group; add one-liners for clarity.
# Issues async def create_issue(issues_list: GithubCreateIssueModelList) -> str: + """Create issues via POST /repos/{owner}/{repo}/issues. Returns JSON string.""" @@ async def get_issue(issues_list: GithubGetIssueModelList) -> str: + """List issues via GET /repos/{owner}/{repo}/issues. Returns JSON string.""" @@ async def update_issue(issues_list: GithubUpdateIssueModelList) -> str: + """Update issues via PATCH /repos/{owner}/{repo}/issues/{number}. Returns JSON string.""" @@ # Pull requests async def create_pull(pull_list: GithubCreatePullList) -> str: + """Create PRs and optionally assign/request reviews. Returns JSON string.""" @@ async def get_pull(pull_list: GithubGetPullsModelList) -> str: + """List pull requests via GET /repos/{owner}/{repo}/pulls. Returns JSON string.""" @@ # Commits (commit updated files) async def commit(updated_file_list: GithubCommitCodeModelList) -> str: + """Commit local files: blob → tree → commit → update ref. Returns JSON string."""Also applies to: 217-251, 252-262, 263-339
src/nat/agent/tool_calling_agent/register.py (1)
114-114: Fix copy/paste log message.Log refers to react_agent.
- logger.debug("%s Cleaning up react_agent workflow.", AGENT_LOG_PREFIX) + logger.debug("%s Cleaning up tool_calling_agent workflow.", AGENT_LOG_PREFIX)packages/nvidia_nat_test/src/nat/test/tool_test_runner.py (1)
288-291: Optional: mirror WorkflowBuilder and acceptFunctionGroupRefhere too.Keeps parity with concrete implementation.
- def get_function_group_dependencies(self, fn_name: str) -> FunctionDependencies: + def get_function_group_dependencies(self, fn_name: str | FunctionGroupRef) -> FunctionDependencies: """Mock implementation.""" return FunctionDependencies()docs/source/workflows/function-groups.md (2)
20-21: Use official first mention: “NVIDIA NeMo Agent toolkit”.-Function groups let you package multiple related functions together so they can share configuration, context, and resources within the NeMo Agent toolkit. +Function groups let you package multiple related functions together so they can share configuration, context, and resources within the NVIDIA NeMo Agent toolkit.
75-77: Consider replacing placeholder context manager with a real one (or add a note).Prevents readers from copying a non-existent symbol.
- async with SomeAsyncContextManager() as context: + # Example: share resources across functions + from contextlib import AsyncExitStack + async with AsyncExitStack() as context:examples/object_store/user_report/configs/config_mysql.yml (1)
74-74: Provide a test stub variant for CI.Repo guidelines recommend nat_test_llm in YAML. Keep this example as-is, but add a sibling test config (e.g., config_mysql.test.yml) switching llms.nim_llm to nat_test_llm with response_seq/delay_ms for deterministic tests.
examples/object_store/user_report/src/nat_user_report/user_report_tools.py (3)
46-47: Add explicit return type and import AsyncIterator.Public API requires return annotations.
@@ -@register_function_group(config_type=UserReportConfig) -async def user_report_group(config: UserReportConfig, builder: Builder): +from typing import AsyncIterator +@register_function_group(config_type=UserReportConfig) +async def user_report_group(config: UserReportConfig, builder: Builder) -> AsyncIterator[FunctionGroup]:
68-90: Validate report JSON before storing with application/json.Currently any string gets stored with JSON content-type; validate or adjust content-type.
@@ - async def put(report: str, user_id: str, date: str | None = None) -> str: + async def put(report: str, user_id: str, date: str | None = None) -> str: """Store a new user report in the object store.""" date = date or "latest" key = f"reports/{user_id}/{date}.json" logger.info("Putting new report into %s for user %s with date %s", key, user_id, date) try: - await object_store.put_object(key=key, - item=ObjectStoreItem(data=report.encode("utf-8"), - content_type="application/json")) + # Ensure valid JSON, else return a clear error + import json + json.loads(report) + await object_store.put_object( + key=key, + item=ObjectStoreItem(data=report.encode("utf-8"), content_type="application/json"), + ) return f"User report for {user_id} with date {date} added successfully" except KeyAlreadyExistsError: return f"User report for {user_id} with date {date} already exists" @@ - async def update(report: str, user_id: str, date: str | None = None) -> str: + async def update(report: str, user_id: str, date: str | None = None) -> str: """Update or create a user report in the object store.""" date = date or "latest" key = f"reports/{user_id}/{date}.json" logger.info("Update or insert report into %s for user %s with date %s", key, user_id, date) - await object_store.upsert_object(key=key, - item=ObjectStoreItem(data=report.encode("utf-8"), - content_type="application/json")) + import json + json.loads(report) + await object_store.upsert_object( + key=key, + item=ObjectStoreItem(data=report.encode("utf-8"), content_type="application/json"), + ) return f"User report for {user_id} with date {date} updated"
60-67: Avoid logging full PII at info level.Consider masking user_id or using debug to reduce PII exposure in examples.
src/nat/builder/function.py (2)
370-377: Add return annotation and tighten converter typing.Public API should be explicit; also prefer concrete callable signatures.
- def add_function(self, + def add_function(self, name: str, fn: Callable, *, input_schema: type[BaseModel] | None = None, description: str | None = None, - converters: list[Callable] | None = None): + converters: list[Callable[[typing.Any], typing.Any]] | None = None) -> None:
400-406: Precompile name regex and shorten error strings (fix TRY003 and micro-opt).Reduces per-call regex cost and silences Ruff on long messages.
@@ - if not name.strip(): - raise ValueError("Function name cannot be empty or blank") - if not re.match(r"^[a-zA-Z0-9_-]+$", name): - raise ValueError(f"Function name can only contain letters, numbers, underscores, and hyphens: {name}") - if name in self._functions: - raise ValueError(f"Function {name} already exists in function group {self._instance_name}") + NAME_RE = re.compile(r"^[a-zA-Z0-9_-]+$") + name = name.strip() + if not name: + raise ValueError("Empty function name.") + if not NAME_RE.match(name): + raise ValueError(f"Invalid function name: {name!r}.") + if name in self._functions: + raise ValueError(f"Duplicate function: {name!r}.")src/nat/cli/type_registry.py (2)
499-516: Shorten duplicate-registration error (fix TRY003).Concise messages avoid Ruff warnings.
- if (registration.config_type in self._registered_function_groups): - raise ValueError( - f"A function group with the same config type `{registration.config_type}` has already been " - "registered.") + if registration.config_type in self._registered_function_groups: + raise ValueError("Function group already registered.")
517-541: Shorten not-found error (fix TRY003).Keep messages compact and actionable.
- except KeyError as err: - raise KeyError(f"Could not find a registered function group for config `{config_type}`. " - f"Registered configs: {set(self._registered_function_groups.keys())}") from err + except KeyError as err: + raise KeyError("No registered function group for config.") from errsrc/nat/builder/workflow_builder.py (4)
382-419: Seed group dependencies under the instance key, not config.type.Avoids stray entries and keeps keys consistent with lookups.
@@ - self.current_function_group_building = config.type - # Empty set of dependencies for the current function group - self.function_group_dependencies[config.type] = FunctionDependencies() + self.current_function_group_building = config.type + # Track dependencies under the instance key + self.function_group_dependencies[name] = FunctionDependencies() @@ - self.function_group_dependencies[name] = inner_builder.dependencies + self.function_group_dependencies[name] = inner_builder.dependencies
535-569: Handle duplicate function exposure when a group and a member are both requested.If tool_names contains both a group and one of its functions, duplicate tools may be returned. Consider de-duping by fn_name.
@@ - tools = [] + tools = [] + seen_fns: set[str] = set() @@ - for fn_name, fn_instance in current_function_group.instance.get_accessible_functions().items(): + for fn_name, fn_instance in current_function_group.instance.get_accessible_functions().items(): + if fn_name in seen_fns: + continue try: # Wrap in the correct wrapper and add to tools list tools.append(tool_wrapper_reg.build_fn(fn_name, fn_instance, self)) + seen_fns.add(fn_name)
570-586: Add return annotation to get_tool.Aligns with Builder interface and repo rules.
- def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str): + def get_tool(self, fn_name: str | FunctionRef, wrapper_type: LLMFrameworkEnum | str) -> typing.Any:
1178-1185: Normalize fn_name before recording dependency.Ensures FunctionRef keys match the string map.
@@ - fn = self._workflow_builder.get_tool(fn_name, wrapper_type) - - self._dependencies.add_function(fn_name) + fn = self._workflow_builder.get_tool(fn_name, wrapper_type) + self._dependencies.add_function(str(fn_name)) return fntests/nat/builder/test_builder.py (5)
286-417: Silence TRY003 in tests and fix unused-arg to satisfy Ruff.
- Several intentional “descriptive” raises trip TRY003; prefer a file-level ignore for tests.
- register_failing_function_group doesn’t use config; rename to _config to fix ARG001.
Apply:
@@ -import logging +import logging +# ruff: noqa: TRY003 # allow descriptive exception messages in this test moduleAnd:
- async def register_failing_function_group(config: FailingFunctionGroupConfig, _builder: Builder): + async def register_failing_function_group(_config: FailingFunctionGroupConfig, _builder: Builder):
1374-1390: Align fixtures with project guidelines (name=..., fixture_ prefix).Rename functions and set name=... to satisfy the stated fixture conventions.
-@pytest.fixture -def caplog_fixture(caplog): +@pytest.fixture(name="caplog_fixture") +def fixture_caplog_fixture(caplog): @@ -@pytest.fixture -def mock_component_data(): +@pytest.fixture(name="mock_component_data") +def fixture_mock_component_data():
1365-1369: Prefer isinstance over issubclass(type(...), ...).More idiomatic and clearer.
- assert issubclass(type(exporter1_instance), BaseExporter) + assert isinstance(exporter1_instance, BaseExporter)
1054-1065: Avoid reaching into builder._functions in tests when possible.These assertions depend on private internals; prefer public APIs (e.g., group.get_included_functions()/get_all_functions()) for stability. Keep as-is if you intentionally validate registry internals.
Also applies to: 1162-1168
1195-1209: Brittle message matching in add_function validation tests.These rely on exact ValueError strings. If implementation text diverges, tests will fail needlessly. Consider relaxing to regex fragments or dropping match.
|
/merge |
4 participants
Description
Overview
This PR introduces Function Groups, a new feature in the NVIDIA NeMo Agent toolkit that enables bundling related functions together to share configuration, context, and resources. This feature provides better organization, resource efficiency, and more flexible function management within workflows.
Key Features Added
1. Function Group Core Implementation
FunctionGroupclass insrc/nat/builder/function.py- Manages collections of related functions with shared contextFunctionGroupBaseConfigsupportinginclude/excludelists for selective function exposure2. Selective Function Exposure
group_name.function_name(e.g.,math.add)3. Workflow Integration
function_groupsconfiguration section in workflow YAML filesadd_function_group()method4. Registration and CLI Support
@register_function_groupdecorator for registering function groupsMajor Changes
Configuration Format Changes
functionssectionfunction_groupssection with shared configurationExample transformation:
GitHub Tools Consolidation
github_tools.pyfunction groupFiles Modified
Core Implementation (18 files):
src/nat/builder/function.py- AddedFunctionGroupclass (+172 lines)src/nat/data_models/function.py- AddedFunctionGroupBaseConfig(+34 lines)src/nat/builder/workflow_builder.py- Added function group support (+220 lines)src/nat/cli/register_workflow.py- Added@register_function_groupdecorator (+43 lines)src/nat/cli/type_registry.py- Added type registry support (+71 lines)GitHub Tools Refactor (8 files):
src/nat/tool/github_tools.py(+450 lines) - Consolidated implementationDocumentation (3 files):
docs/source/extend/function-groups.md(+136 lines) - Developer guidedocs/source/workflows/function-groups.md(+162 lines) - User guidedocs/source/index.md- Added navigation linksExamples and Tests (13 files):
Benefits
Testing
Breaking Changes
The GitHub tools are now exposed as two function groups.
Documentation
Commit Summary: 3 commits implementing function groups with include/exclude functionality, GitHub tools consolidation, and comprehensive documentation.
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Configuration
Agents
Documentation
Tests
Chores