Feature: Microsoft AutoGen Integration [Synopsys] #859
Conversation
WalkthroughAdds Microsoft AutoGen support: new autogen plugin (LLM clients, tool wrapper, profiler), AUTOGEN enum and profiler hook, packaging/workspace updates, comprehensive tests, and a runnable AutoGen demo example (config, tool, README, and registrations). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant WF as autogen_team Workflow
participant B as Builder
participant L as AutoGen LLM Client
participant A1 as WeatherAndTimeAgent
participant A2 as FinalResponseAgent
participant T as Tools (weather_update, MCP)
participant GC as RoundRobinGroupChat
U->>WF: user_input
WF->>B: resolve LLM (llm_name)
B-->>WF: AutoGen LLM client
WF->>B: resolve tools + MCP adapter
B-->>WF: tool handles
WF->>GC: init with A1,A2 termination="APPROVE"
GC->>A1: start turn
A1->>L: LLM call
A1->>T: call tools
T-->>A1: tool results
A1-->>GC: message
GC->>A2: next turn
A2->>L: LLM call (finalize)
A2-->>GC: "APPROVE" + final content
GC-->>WF: result
WF-->>U: final content or error
sequenceDiagram
autonumber
participant FW as Framework Wrapper
participant PH as AutoGenProfilerHandler
participant AC as AutoGen LLM Client.create
participant BT as AutoGen BaseTool.run_json
participant SM as IntermediateStepManager
FW->>PH: instantiate + instrument()
PH->>AC: monkey-patch create()
PH->>BT: monkey-patch run_json()
note right of PH: instrumentation active
AC->>SM: LLM_START (inputs, metadata)
AC-->>SM: LLM_END (outputs, usage)
BT->>SM: TOOL_START (inputs)
BT-->>SM: TOOL_END (outputs, usage)
FW->>PH: uninstrument()
PH->>AC: restore create()
PH->>BT: restore run_json()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
saglave
force-pushed
the
autogen_integration
branch
2 times, most recently
from
74a2ca8 to
4231323
Compare
onkkul
force-pushed
the
autogen_integration
branch
from
4231323 to
831e019
Compare
coderabbitai
bot
added
external
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py (1)
118-120: Re-raise withraiseto keep the original traceback.Per the repo’s exception-handling guideline, use a bare
raiseafter logging;raise eresets the stack and makes diagnostics harder to read. As per coding guidelines.except Exception as e: logger.exception("Failed to initialize AutoGen workflow") - raise e + raise
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
examples/frameworks/nat_autogen_demo/README.md(1 hunks)examples/frameworks/nat_autogen_demo/config.yml(1 hunks)examples/frameworks/nat_autogen_demo/configs/config.yml(1 hunks)examples/frameworks/nat_autogen_demo/pyproject.toml(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/__init__.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py(1 hunks)packages/nvidia_nat_autogen/pyproject.toml(1 hunks)packages/nvidia_nat_autogen/src/nat/meta/pypi.md(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)packages/nvidia_nat_autogen/tests/__init__.py(1 hunks)packages/nvidia_nat_autogen/tests/conftest.py(1 hunks)packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/tests/test_llm.py(1 hunks)packages/nvidia_nat_autogen/tests/test_register.py(1 hunks)packages/nvidia_nat_autogen/tests/test_tool_wrapper.py(1 hunks)pyproject.toml(5 hunks)src/nat/builder/framework_enum.py(1 hunks)src/nat/profiler/decorators/framework_wrapper.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{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:
packages/nvidia_nat_autogen/tests/__init__.pysrc/nat/builder/framework_enum.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/conftest.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/__init__.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pyexamples/frameworks/nat_autogen_demo/config.ymlpackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.pysrc/nat/profiler/decorators/framework_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/configs/config.ymlpackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/tests/__init__.pysrc/nat/builder/framework_enum.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/conftest.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/__init__.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.pysrc/nat/profiler/decorators/framework_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/__init__.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/conftest.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.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:
packages/nvidia_nat_autogen/tests/__init__.pysrc/nat/builder/framework_enum.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/conftest.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/__init__.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pyexamples/frameworks/nat_autogen_demo/config.ymlpackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pyexamples/frameworks/nat_autogen_demo/README.mdpackages/nvidia_nat_autogen/pyproject.tomlexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.pysrc/nat/profiler/decorators/framework_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/configs/config.ymlpyproject.tomlexamples/frameworks/nat_autogen_demo/pyproject.tomlpackages/nvidia_nat_autogen/src/nat/meta/pypi.mdpackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.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_autogen/tests/__init__.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/conftest.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/pyproject.tomlpackages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/meta/pypi.mdpackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/builder/framework_enum.pysrc/nat/profiler/decorators/framework_wrapper.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/builder/framework_enum.pysrc/nat/profiler/decorators/framework_wrapper.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/builder/framework_enum.pysrc/nat/profiler/decorators/framework_wrapper.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
src/nat/builder/framework_enum.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.pysrc/nat/profiler/decorators/framework_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.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/frameworks/nat_autogen_demo/src/nat_autogen_demo/__init__.pyexamples/frameworks/nat_autogen_demo/config.ymlexamples/frameworks/nat_autogen_demo/README.mdexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.pyexamples/frameworks/nat_autogen_demo/configs/config.ymlexamples/frameworks/nat_autogen_demo/pyproject.tomlexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
**/*.{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/frameworks/nat_autogen_demo/config.ymlexamples/frameworks/nat_autogen_demo/configs/config.yml
**/README.@(md|ipynb)
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Ensure READMEs follow the naming convention; avoid deprecated names; use “NeMo Agent Toolkit” (capital T) in headings
Files:
examples/frameworks/nat_autogen_demo/README.md
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/pyproject.toml: Each package must contain a pyproject.toml
In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Use ~= version constraints (e.g., ~=1.0) for dependencies
Files:
packages/nvidia_nat_autogen/pyproject.toml
{packages/*/pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Add new dependencies to both pyproject.toml (alphabetically) and uv.lock via uv pip install --sync
Files:
packages/nvidia_nat_autogen/pyproject.toml
**/configs/**
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must be stored next to that code in a configs/ folder
Files:
examples/frameworks/nat_autogen_demo/configs/config.yml
🧠 Learnings (1)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to packages/*/pyproject.toml : In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Applied to files:
packages/nvidia_nat_autogen/pyproject.tomlpyproject.tomlexamples/frameworks/nat_autogen_demo/pyproject.toml
🧬 Code graph analysis (12)
packages/nvidia_nat_autogen/tests/test_register.py (1)
tests/nat/builder/test_builder.py (1)
tool_wrapper(554-564)
packages/nvidia_nat_autogen/tests/conftest.py (1)
packages/nvidia_nat_data_flywheel/src/nat/plugins/data_flywheel/observability/schema/sink/elasticsearch/dfw_es_record.py (1)
SystemMessage(74-78)
packages/nvidia_nat_autogen/tests/test_tool_wrapper.py (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
autogen_tool_wrapper(59-169)resolve_type(39-55)
packages/nvidia_nat_autogen/tests/test_llm.py (6)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (4)
_patch_autogen_client_based_on_config(39-100)openai_autogen(104-142)azure_openai_autogen(146-189)nim_autogen(193-231)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
src/nat/builder/context.py (3)
Context(115-299)intermediate_step_manager(169-180)output(58-59)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/profiler/decorators/function_tracking.py (1)
push_intermediate_step(60-79)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py (1)
tests/nat/builder/test_builder.py (1)
tool_wrapper(554-564)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py (4)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/builder/function_info.py (2)
FunctionInfo(290-625)from_fn(552-625)src/nat/data_models/function.py (1)
FunctionBaseConfig(26-27)
src/nat/profiler/decorators/framework_wrapper.py (2)
src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (2)
AutoGenProfilerHandler(36-326)instrument(63-92)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (5)
AutoGenProfilerHandler(36-326)instrument(63-92)uninstrument(94-108)_llm_call_monkey_patch(110-236)_tool_call_monkey_patch(238-326)src/nat/builder/context.py (1)
intermediate_step_manager(169-180)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (8)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)src/nat/llm/utils/thinking.py (3)
BaseThinkingInjector(57-81)FunctionArgumentWrapper(34-53)patch_with_thinking(121-215)src/nat/utils/exception_handlers/automatic_retries.py (1)
patch_with_retry(269-342)src/nat/utils/type_utils.py (1)
override(56-57)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py (5)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/builder/function_info.py (2)
FunctionInfo(290-625)create(351-549)src/nat/data_models/component_ref.py (1)
LLMRef(116-124)src/nat/data_models/function.py (1)
FunctionBaseConfig(26-27)
🪛 Ruff (0.13.2)
packages/nvidia_nat_autogen/tests/test_llm.py
36-36: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
50-50: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
100-100: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
198-198: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
258-258: Unused function argument: args
(ARG001)
258-258: Unused function argument: kwargs
(ARG001)
292-292: Unused function argument: args
(ARG001)
292-292: Unused function argument: kwargs
(ARG001)
330-330: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
356-356: Unused function argument: args
(ARG001)
356-356: Unused function argument: kwargs
(ARG001)
404-404: Unused function argument: args
(ARG001)
404-404: Unused function argument: kwargs
(ARG001)
447-447: Unused function argument: args
(ARG001)
447-447: Unused function argument: kwargs
(ARG001)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
182-182: Use explicit conversion flag
Replace with conversion flag
(RUF010)
300-300: Use explicit conversion flag
Replace with conversion flag
(RUF010)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
120-120: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
149-149: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
160-160: Unused function argument: args
(ARG001)
160-160: Unused function argument: kwargs
(ARG001)
206-206: Unused function argument: args
(ARG001)
206-206: Unused function argument: kwargs
(ARG001)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
78-78: Consider [system_message, *messages] instead of concatenation
Replace with [system_message, *messages]
(RUF005)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
28-28: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
29-29: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
107-107: Consider moving this statement to an else block
(TRY300)
111-111: Use explicit conversion flag
Replace with conversion flag
(RUF010)
120-120: Use raise without specifying exception name
Remove exception name
(TRY201)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
Show resolved
Hide resolved
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
Show resolved
Hide resolved
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
Show resolved
Hide resolved
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
Outdated
Show resolved
Hide resolved
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
Outdated
Show resolved
Hide resolved
|
Hello @AnuradhaKaruppiah, @willkill07, PR is ready. Addressing CodeRabbit Comments. Please take a look. |
|
This is slightly deprioritized due to work for the upcoming release. I will try to review by EOD Monday. |
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
Outdated
Show resolved
Hide resolved
|
Addressed comments, @willkill07 |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (3)
49-55: Preserve all non-Noneunion members.The current implementation returns only the first non-
Nonemember of a union, dropping other types. For example,str | int | Nonecollapses tostr, losingint. This causes AutoGen to advertise an incomplete schema for tools that accept multiple types.Apply this diff to preserve all non-
Noneunion members:- if origin is types.UnionType: - for arg in get_args(t): - if arg is not type(None): - return arg - return t + if origin is types.UnionType: + non_none_args = tuple(arg for arg in get_args(t) if arg is not type(None)) + if not non_none_args: + return t + if len(non_none_args) == 1: + return non_none_args[0] + # Reconstruct union with remaining types + resolved = non_none_args[0] + for arg in non_none_args[1:]: + resolved = resolved | arg + return resolved
123-124: Fix dataclass-to-BaseModel conversion.
BaseModel.model_validate()expects a data instance, not a type. Passing the dataclass type here raises a validation error before the wrapper reaches AutoGen.Apply this diff to convert the dataclass type into a BaseModel class:
-from pydantic import BaseModel +from pydantic import BaseModel +from pydantic.dataclasses import create_model_from_dataclassThen update the conversion:
# If input_schema is a dataclass, convert it to a Pydantic model if input_schema is not None and is_dataclass(input_schema): - input_schema = BaseModel.model_validate(input_schema) + input_schema = create_model_from_dataclass(input_schema)
140-154: Prevent erasing the original function signature.Lines 153-154 execute even when
input_schema is None, setting an empty signature and empty annotations. This overwrites the original function's signature and type hints.Move the signature and annotations assignment inside the
if input_schema is not None:block:if input_schema is not None: annotations = {} params = [] model_fields = getattr(input_schema, "model_fields", {}) for param_name, model_field in model_fields.items(): resolved_type = resolve_type(model_field.annotation) default = model_field.default if model_field.is_required() is False else inspect._empty params.append( inspect.Parameter(param_name, inspect.Parameter.POSITIONAL_OR_KEYWORD, annotation=resolved_type, default=default)) annotations[param_name] = resolved_type - setattr(func_to_wrap, "__signature__", inspect.Signature(parameters=params)) - setattr(func_to_wrap, "__annotations__", annotations) + func_to_wrap.__signature__ = inspect.Signature(parameters=params) + func_to_wrap.__annotations__ = annotations
🧹 Nitpick comments (15)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
120-120: Consider extracting the exception message.Static analysis flags this as TRY003 - long messages should be defined outside the exception class for better maintainability.
Apply this diff:
+ _MISSING_FUNC_MSG = "'func' must be provided." + if func is None: - raise ValueError("'func' must be provided.") + raise ValueError(_MISSING_FUNC_MSG)
153-154: Replacesetattrwith direct assignment.Static analysis flags B010 -
setattrwith a constant attribute is no safer than direct assignment and reduces readability.(This change is already included in the diff above for lines 140-154.)
packages/nvidia_nat_autogen/tests/test_llm.py (10)
36-37: Annotate mutable class attributes withClassVar.Class attributes that are mutable (lists, dicts, sets) and intended to be shared across all instances should be annotated with
typing.ClassVarto avoid unintended mutation and to clarify intent.Apply this diff:
+from typing import ClassVar + class TestRetryConfig(LLMBaseConfig, RetryMixin): """Test config with retry mixin.""" num_retries: int = 3 - retry_on_status_codes: list = [500, 502, 503] - retry_on_errors: list[Any] | None = ["timeout"] + retry_on_status_codes: ClassVar[list] = [500, 502, 503] + retry_on_errors: ClassVar[list[Any] | None] = ["timeout"]
50-51: Annotate mutable class attributes withClassVar.Similarly, annotate these mutable class attributes in
TestCombinedConfigwithtyping.ClassVar.Apply this diff:
class TestCombinedConfig(LLMBaseConfig, RetryMixin, ThinkingMixin): """Test config with both mixins.""" num_retries: int = 3 - retry_on_status_codes: list = [500, 502, 503] - retry_on_errors: list[Any] | None = ["timeout"] + retry_on_status_codes: ClassVar[list] = [500, 502, 503] + retry_on_errors: ClassVar[list[Any] | None] = ["timeout"] thinking_system_prompt: str = "Think step by step"
100-100: Prefix unused unpacked variable with underscore.The
kwargsvariable fromcall_argsis unpacked but never used. Prefix it with an underscore to indicate it's intentionally unused.Apply this diff:
mock_patch_thinking.assert_called_once() - args, kwargs = mock_patch_thinking.call_args + args, _kwargs = mock_patch_thinking.call_args assert args[0] == mock_client
198-198: Prefix unused unpacked variable with underscore.The
kwargsvariable is unpacked but never used. Prefix it with an underscore.Apply this diff:
# Verify the injector is passed correctly - args, kwargs = mock_patch.call_args + args, _kwargs = mock_patch.call_args assert args[0] == mock_client
261-280: Remove unused function arguments or prefix with underscore.The
argsandkwargsparameters inimport_side_effectare documented but never used. Either prefix them with underscores or remove the parameters if the signature allows.Apply this diff:
- def import_side_effect(name, *args, **kwargs): + def import_side_effect(name, *_args, **_kwargs): """Side effect function to mock imports. Args: name (str): The name of the module being imported. - *args: Additional positional arguments. - **kwargs: Additional keyword arguments. + *_args: Additional positional arguments (unused). + **_kwargs: Additional keyword arguments (unused). Returns: Mock: A mock module or object based on the import name. """
305-305: Remove unused function arguments or prefix with underscore.The
argsparameter is typed but never used. Prefix it with an underscore or remove it if the signature allows.Apply this diff:
- def import_side_effect(name, *args: Any, **kwargs: Any) -> Mock: + def import_side_effect(name, *_args: Any, **_kwargs: Any) -> Mock: """Side effect function to mock imports. Args: name (str): The name of the module being imported. - *args (Any): Additional positional arguments. - **kwargs (Any): Additional keyword arguments. + *_args (Any): Additional positional arguments (unused). + **_kwargs (Any): Additional keyword arguments (unused). Returns: Mock: A mock module or object based on the import name. """
353-353: Prefix unused unpacked variable with underscore.The
kwargsvariable is unpacked but never used. Prefix it with an underscore.Apply this diff:
# Verify patch_with_thinking was called with injector mock_patch.assert_called_once() - args, kwargs = mock_patch.call_args + args, _kwargs = mock_patch.call_args assert args[0] == mock_client
379-379: Remove unused function arguments or prefix with underscore.The
argsandkwargsparameters are never used. Prefix them with underscores.Apply this diff:
- def side_effect(name, *args, **kwargs): + def side_effect(name, *_args, **_kwargs): if name == 'autogen_core.models': return mock_autogen_core
427-427: Remove unused function arguments or prefix with underscore.The
argsandkwargsparameters are never used. Prefix them with underscores.Apply this diff:
- def side_effect(name, *args, **kwargs): + def side_effect(name, *_args, **_kwargs): if name == 'autogen_core.models': return mock_autogen_core
470-470: Remove unused function arguments or prefix with underscore.The
argsandkwargsparameters are never used. Prefix them with underscores.Apply this diff:
- def side_effect(name, *args, **kwargs): + def side_effect(name, *_args, **_kwargs): if name == 'autogen_core.models': return mock_autogen_coreexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (2)
101-104: Move fallback return toelseblock for clarity.Per Ruff hint TRY300, moving the fallback return statement to an
elseblock improves readability by clearly showing the successful path vs. the fallback.Apply this diff:
result = await team.run(task=user_input) if hasattr(result, 'messages') and result.messages: return result.messages[-1].content - - return "The workflow finished but no output was generated." + else: + return "The workflow finished but no output was generated." except Exception as e:
108-108: Use explicit conversion flag in f-string.Per Ruff hint RUF010, use an explicit conversion flag (
!s,!r, or!a) when callingstr()in an f-string to clarify intent and avoid redundancy.Apply this diff:
except Exception as e: logger.exception("Error in AutoGen workflow") - return f"Error occurred during AutoGen workflow: {str(e)}" + return f"Error occurred during AutoGen workflow: {e!s}"examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py (1)
16-17: Remove unnecessarynoqadirectives.The
noqa: F401directives are unnecessary because F401 (unused import) is not enabled in your Ruff configuration. These imports are correctly marked with# pylint: disable=W0611for pylint, which is sufficient.Apply this diff:
-from . import autogen_team # noqa: F401 # pylint: disable=W0611 #imported for side effects (registration) -from . import weather_update_tool # noqa: F401 # pylint: disable=W0611 #imported for side effects (registration) +from . import autogen_team # pylint: disable=W0611 # imported for side effects (registration) +from . import weather_update_tool # pylint: disable=W0611 # imported for side effects (registration)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/frameworks/nat_autogen_demo/README.md(1 hunks)examples/frameworks/nat_autogen_demo/configs/config.yml(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py(1 hunks)packages/nvidia_nat_autogen/pyproject.toml(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)packages/nvidia_nat_autogen/tests/test_llm.py(1 hunks)packages/nvidia_nat_autogen/tests/test_tool_wrapper.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/frameworks/nat_autogen_demo/README.md
- examples/frameworks/nat_autogen_demo/configs/config.yml
- packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/tests/test_llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/tests/test_llm.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/pyproject.tomlexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/tests/test_llm.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_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/pyproject.tomlpackages/nvidia_nat_autogen/tests/test_llm.py
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/pyproject.toml: Each package must contain a pyproject.toml
In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Use ~= version constraints (e.g., ~=1.0) for dependencies
Files:
packages/nvidia_nat_autogen/pyproject.toml
{packages/*/pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Add new dependencies to both pyproject.toml (alphabetically) and uv.lock via uv pip install --sync
Files:
packages/nvidia_nat_autogen/pyproject.toml
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/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/test_llm.py
🧠 Learnings (1)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to packages/*/pyproject.toml : In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Applied to files:
packages/nvidia_nat_autogen/pyproject.toml
🧬 Code graph analysis (4)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (5)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/builder/function_info.py (2)
FunctionInfo(290-625)create(351-549)src/nat/data_models/component_ref.py (1)
LLMRef(116-124)src/nat/data_models/function.py (1)
FunctionBaseConfig(26-27)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py (1)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (1)
autogen_team(39-119)
packages/nvidia_nat_autogen/tests/test_llm.py (6)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (4)
_patch_autogen_client_based_on_config(39-100)openai_autogen(104-142)azure_openai_autogen(146-189)nim_autogen(193-231)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
120-120: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
154-154: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
104-104: Consider moving this statement to an else block
(TRY300)
108-108: Use explicit conversion flag
Replace with conversion flag
(RUF010)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
17-17: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
packages/nvidia_nat_autogen/tests/test_llm.py
36-36: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
50-50: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
100-100: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
198-198: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
261-261: Unused function argument: args
(ARG001)
261-261: Unused function argument: kwargs
(ARG001)
305-305: Unused function argument: args
(ARG001)
305-305: Unused function argument: kwargs
(ARG001)
353-353: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
379-379: Unused function argument: args
(ARG001)
379-379: Unused function argument: kwargs
(ARG001)
427-427: Unused function argument: args
(ARG001)
427-427: Unused function argument: kwargs
(ARG001)
470-470: Unused function argument: args
(ARG001)
470-470: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (3)
1-36: LGTM!License header is correct with current copyright year, imports are well-organized, and the module docstring follows guidelines.
77-98: LGTM!The async invoke and stream wrappers correctly delegate to the NAT function's async methods.
164-174: LGTM!Tool creation correctly selects between streaming and non-streaming wrappers based on function capabilities, and returns a properly configured
FunctionTool.
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
Show resolved
Hide resolved
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
Outdated
Show resolved
Hide resolved
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
Outdated
Show resolved
Hide resolved
|
/ok to test 418cde6 |
|
Fixed tests, @willkill07 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
46-53: Fix constructor docstring.Docstring still documents
context/**kwargs, but__init__takes onlyself. Please describe the actual initialization work (lock setup, timestamp, step manager lookup) so the public API stays accurate.
180-235: Do not coerce LLM failures into strings.Catching
Exceptionand returningf"LLM call failed …"violates the AutoGen contract—the caller expects the real response type or the original exception. This masks production bugs and will explode the next time code dereferencesoutput.content. Log withlogger.error(...), emit any telemetry you need, and re-raise with a bareraiseso we preserve stack traces per coding guidelines. Also guard the post-call processing so it only runs on successful responses. For example:- try: - output = await original_func(*args, **kwargs) - except Exception as _e: - output = f"LLM call failed with error: {str(_e)}" - logger.exception("Error during LLM call") + try: + output = await original_func(*args, **kwargs) + except Exception as exc: + logger.error("Error during LLM call: %s", exc) + with self._lock: + self.last_call_ts = time.time() + raise + else: + ... + self.step_manager.push_intermediate_step(output_stats) + with self._lock: + self.last_call_ts = now + return output
296-325: Propagate tool execution errors.Same problem on the tool path: converting exceptions to strings breaks AutoGen tool expectations and hides real failures. Log the error and re-raise so callers can handle it (or fail fast) while telemetry still records the failure. Wrap the success-only logic in an
elseto avoid touchingoutputafter the re-raise.packages/nvidia_nat_autogen/tests/test_llm.py (1)
335-338: Consume the async generator instead ofasync with.
nim_autogenreturns a bare async generator; it does not implement__aenter__/__aexit__.async withwill raiseAttributeError. Match the pattern you already use above:- # Test the async context manager - async with nim_autogen(config, mock_builder) as client: - # Just verify we got a client back without errors - assert client is not None + gen = nim_autogen(config, mock_builder) + client = await gen.__anext__() + assert client is not None + with pytest.raises(StopAsyncIteration): + await gen.__anext__()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/frameworks/nat_autogen_demo/README.md(1 hunks)examples/frameworks/nat_autogen_demo/pyproject.toml(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py(1 hunks)packages/nvidia_nat_autogen/pyproject.toml(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py(1 hunks)packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/tests/test_llm.py(1 hunks)packages/nvidia_nat_autogen/tests/test_register.py(1 hunks)packages/nvidia_nat_autogen/tests/test_tool_wrapper.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/nvidia_nat_autogen/pyproject.toml
- examples/frameworks/nat_autogen_demo/README.md
- examples/frameworks/nat_autogen_demo/pyproject.toml
- packages/nvidia_nat_autogen/tests/test_register.py
- examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
- packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.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:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.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_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py
🧬 Code graph analysis (3)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (5)
AutoGenProfilerHandler(37-327)instrument(64-93)uninstrument(95-109)_llm_call_monkey_patch(111-237)_tool_call_monkey_patch(239-327)packages/nvidia_nat_autogen/tests/test_llm.py (3)
side_effect(383-398)side_effect(441-456)side_effect(494-509)src/nat/builder/context.py (1)
intermediate_step_manager(169-180)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
src/nat/builder/context.py (3)
Context(115-299)intermediate_step_manager(169-180)output(58-59)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/profiler/decorators/function_tracking.py (1)
push_intermediate_step(60-79)
packages/nvidia_nat_autogen/tests/test_llm.py (6)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (4)
_patch_autogen_client_based_on_config(39-100)openai_autogen(104-142)azure_openai_autogen(146-189)nim_autogen(193-231)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
161-161: Unused function argument: args
(ARG001)
161-161: Unused function argument: kwargs
(ARG001)
207-207: Unused function argument: args
(ARG001)
207-207: Unused function argument: kwargs
(ARG001)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
183-183: Use explicit conversion flag
Replace with conversion flag
(RUF010)
301-301: Use explicit conversion flag
Replace with conversion flag
(RUF010)
packages/nvidia_nat_autogen/tests/test_llm.py
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
39-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
54-54: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
55-55: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
309-309: Unused function argument: args
(ARG001)
309-309: Unused function argument: kwargs
(ARG001)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
Outdated
Show resolved
Hide resolved
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
Outdated
Show resolved
Hide resolved
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
Outdated
Show resolved
Hide resolved
packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
examples/frameworks/nat_autogen_demo/README.md (1)
44-51: Clarify environment variables; only API_KEY is sensitive, add export examples.OPENAI_MODEL_NAME and OPENAI_API_BASE aren’t API keys. Provide explicit export examples and note that API_BASE is optional.
Apply this edit:
-### Export required ENV variables -If you have not already done so, follow the [Obtaining API Keys](../../../docs/source/quick-start/installing.md#obtaining-api-keys) instructions to obtain API keys. - -For OpenAI Export these: -- OPENAI_MODEL_NAME -- OPENAI_API_KEY -- OPENAI_API_BASE +### Set environment variables +If you have not already done so, follow the [Obtaining API Keys](../../../docs/source/quick-start/installing.md#obtaining-api-keys) instructions to obtain API keys. + +For OpenAI, set: + +```bash +# Required (sensitive) +export OPENAI_API_KEY="<your-api-key>" + +# Required (model selection) +export OPENAI_MODEL_NAME="gpt-4o-mini" + +# Optional: non-default endpoint (e.g., Azure OpenAI) +export OPENAI_API_BASE="https://<your-endpoint>/" +```As per coding guidelines
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
39-55: Critical: Union type resolution drops all but first non-None member.The current implementation returns only the first non-None type from a union, discarding others. For
str | int | None, this returnsstr, losingintentirely. AutoGen will then advertise an incorrect schema, rejecting valid integer inputs.As noted in past review, consider reusing
nat.utils.type_utilsinstead of implementing custom type resolution. If a custom implementation is needed, preserve all non-None union members:def resolve_type(t: Any) -> Any: - """Return the non-None member of a Union/PEP 604 union; - otherwise return the type unchanged. + """Filter out None from a Union/PEP 604 union, preserving other members; + otherwise return the type unchanged. Args: t (Any): The type to resolve. Returns: Any: The resolved type. """ origin = get_origin(t) if origin is types.UnionType: - for arg in get_args(t): - if arg is not type(None): - return arg - return t + non_none_args = tuple(arg for arg in get_args(t) if arg is not type(None)) + if not non_none_args: + return t + if len(non_none_args) == 1: + return non_none_args[0] + # Reconstruct union of all non-None members + result = non_none_args[0] + for arg in non_none_args[1:]: + result = result | arg + return result return t
140-154: Move signature assignment inside schema check.Lines 153-154 execute even when
input_schema is None, overwriting the function's original signature with empty parameters. This erases signatures for functions without schemas.Apply this diff to preserve original signatures:
if input_schema is not None: annotations = {} params = [] model_fields = getattr(input_schema, "model_fields", {}) for param_name, model_field in model_fields.items(): resolved_type = resolve_type(model_field.annotation) default = model_field.default if model_field.is_required() is False else inspect._empty params.append( inspect.Parameter(param_name, inspect.Parameter.POSITIONAL_OR_KEYWORD, annotation=resolved_type, default=default)) annotations[param_name] = resolved_type - setattr(func_to_wrap, "__signature__", inspect.Signature(parameters=params)) - setattr(func_to_wrap, "__annotations__", annotations) + func_to_wrap.__signature__ = inspect.Signature(parameters=params) + func_to_wrap.__annotations__ = annotationsNote: The direct attribute assignment (without
setattr) also addresses the B010 static analysis hint.packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
46-53: Update docstring to match actual signature.The docstring documents
contextand**kwargsparameters that don't exist in the__init__signature. Update to describe the actual initialization behavior.Apply this diff:
def __init__(self) -> None: """ - Initializes the AutogenProfilerHandler. + Initialize the AutogenProfilerHandler. - Args: - context (Context): The context object containing configuration and state information. - **kwargs: Additional keyword arguments. + Creates a threading lock, retrieves the intermediate step manager from Context, + and initializes internal state for instrumentation tracking. """
180-184: Critical: Re-raise LLM exceptions instead of returning error strings.Converting exceptions to strings and returning them violates AutoGen's contract. Callers expect either a valid response object or an exception, not a string. Downstream code attempting
output.contentwill fail withAttributeError.Apply this diff to log, capture telemetry, and re-raise:
try: output = await original_func(*args, **kwargs) - except Exception as _e: - output = f"LLM call failed with error: {str(_e)}" - logger.exception("Error during LLM call") + except Exception as exc: + logger.error("Error during LLM call: %s", exc) + self.step_manager.push_intermediate_step( + IntermediateStepPayload( + event_type=IntermediateStepType.LLM_END, + span_event_timestamp=time.time(), + framework=LLMFrameworkEnum.AUTOGEN, + name=model_name, + data=StreamEventData(input=model_input, output=str(exc)), + metadata=TraceMetadata(error=str(exc)), + usage_info=UsageInfo(token_usage=TokenUsageBaseModel()), + UUID=llm_start_uuid, + )) + with self._lock: + self.last_call_ts = time.time() + raiseAs per coding guidelines: use
logger.error()(notlogger.exception()) when re-raising to avoid duplicate stack traces.
296-303: Add telemetry and remove wasteful assignment before re-raising.The exception handling correctly re-raises but misses telemetry capture and includes an unnecessary string assignment that's never used.
Apply this diff:
try: - # Call the original BaseTool.run_json(...) - # output = await original_func(*args, **kwargs) output = await original_func(*args, **kwargs) - except Exception as _e: - output = f"Tool execution failed with error: {str(_e)}" + except Exception as exc: logger.error("Error during tool execution") + self.step_manager.push_intermediate_step( + IntermediateStepPayload( + event_type=IntermediateStepType.TOOL_END, + span_event_timestamp=time.time(), + framework=LLMFrameworkEnum.AUTOGEN, + name=tool_name, + data=StreamEventData(input=tool_input, output=str(exc)), + metadata=TraceMetadata(error=str(exc)), + usage_info=UsageInfo(token_usage=TokenUsageBaseModel()), + UUID=tool_start_uuid, + )) + with self._lock: + self.last_call_ts = time.time() raiseThis ensures failed tool calls are captured in telemetry while preserving proper exception propagation.
packages/nvidia_nat_autogen/tests/test_llm.py (1)
332-335: Critical: Use async generator pattern instead of async context manager.
nim_autogenreturns anAsyncGenerator, not an async context manager. Usingasync withwill raiseAttributeError: __aenter__at runtime. Match the pattern fromtest_openai_autogen_generator(lines 246-249).Apply this diff:
- # Test the async context manager - async with nim_autogen(config, mock_builder) as client: - # Just verify we got a client back without errors - assert client is not None + # Test the async generator + gen = nim_autogen(config, mock_builder) + client = await gen.__anext__() + + assert client is not None + with pytest.raises(StopAsyncIteration): + await gen.__anext__()The same issue exists in
test_azure_openai_autogen_generator(lines 291-292).
🧹 Nitpick comments (2)
packages/nvidia_nat_test/src/nat/test/llm.py (1)
36-205: Consider adding AUTOGEN framework support.The
LLMFrameworkEnum.AUTOGENwas added to the framework enum (as shown in relevant code snippets), but this file doesn't provide a test LLM implementation for AUTOGEN. Adding support would ensure consistency with other frameworks and enable testing of AUTOGEN-based integrations.You would need to add a function similar to the other framework implementations:
@register_llm_client(config_type=MockLLMConfig, wrapper_type=LLMFrameworkEnum.AUTOGEN) async def test_llm_autogen(config: MockLLMConfig, builder: Builder): """LLM client for AutoGen.""" chooser = _ResponseChooser(response_seq=config.response_seq, delay_ms=config.delay_ms) class AutoGenTestLLM: # Implement AutoGen-specific methods here pass yield AutoGenTestLLM()examples/frameworks/nat_autogen_demo/README.md (1)
102-110: Remove duplicate “Workflow Result” block.The section repeats immediately; prune the second block to avoid confusion.
--------------------------------------------------- -Workflow Result: -['New York weather: Sunny, around 25°C (77°F).\nCurrent local time in New York: 5:34 PM EDT (UTC−4) on October 7, 2025.\n\nAPPROVE'] --------------------------------------------------
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
examples/frameworks/nat_autogen_demo/README.md(1 hunks)examples/frameworks/nat_autogen_demo/pyproject.toml(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py(1 hunks)packages/nvidia_nat_autogen/pyproject.toml(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)packages/nvidia_nat_autogen/tests/test_llm.py(1 hunks)packages/nvidia_nat_autogen/tests/test_register.py(1 hunks)packages/nvidia_nat_autogen/tests/test_tool_wrapper.py(1 hunks)packages/nvidia_nat_test/src/nat/test/llm.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/frameworks/nat_autogen_demo/pyproject.toml
- packages/nvidia_nat_autogen/tests/test_register.py
- examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_test/src/nat/test/llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_test/src/nat/test/llm.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_test/src/nat/test/llm.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_test/src/nat/test/llm.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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/README.mdpackages/nvidia_nat_autogen/pyproject.tomlpackages/nvidia_nat_test/src/nat/test/llm.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_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/register.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/pyproject.tomlpackages/nvidia_nat_test/src/nat/test/llm.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_llm.py
**/README.@(md|ipynb)
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Ensure READMEs follow the naming convention; avoid deprecated names; use “NeMo Agent Toolkit” (capital T) in headings
Files:
examples/frameworks/nat_autogen_demo/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/frameworks/nat_autogen_demo/README.md
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/pyproject.toml: Each package must contain a pyproject.toml
In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Use ~= version constraints (e.g., ~=1.0) for dependencies
Files:
packages/nvidia_nat_autogen/pyproject.toml
{packages/*/pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Add new dependencies to both pyproject.toml (alphabetically) and uv.lock via uv pip install --sync
Files:
packages/nvidia_nat_autogen/pyproject.toml
🧠 Learnings (2)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to packages/*/pyproject.toml : In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Applied to files:
packages/nvidia_nat_autogen/pyproject.toml
📚 Learning: 2025-09-09T20:32:39.016Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/nat-test-llm.mdc:0-0
Timestamp: 2025-09-09T20:32:39.016Z
Learning: Applies to **/*.py : Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("<name>", cfg).
Applied to files:
packages/nvidia_nat_test/src/nat/test/llm.py
🧬 Code graph analysis (5)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
src/nat/builder/context.py (3)
Context(115-299)intermediate_step_manager(169-180)output(58-59)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/profiler/decorators/function_tracking.py (1)
push_intermediate_step(60-79)
packages/nvidia_nat_autogen/tests/test_tool_wrapper.py (3)
src/nat/builder/builder.py (1)
Builder(68-290)tests/nat/builder/test_builder.py (1)
tool_wrapper(554-564)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
autogen_tool_wrapper(59-174)resolve_type(39-55)
packages/nvidia_nat_autogen/tests/test_llm.py (6)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (4)
_patch_autogen_client_based_on_config(39-100)openai_autogen(104-142)azure_openai_autogen(146-189)nim_autogen(193-231)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)
packages/nvidia_nat_test/src/nat/test/llm.py (2)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
183-183: Use explicit conversion flag
Replace with conversion flag
(RUF010)
301-301: Use explicit conversion flag
Replace with conversion flag
(RUF010)
302-302: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
packages/nvidia_nat_autogen/tests/test_llm.py
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Unused function argument: args
(ARG001)
306-306: Unused function argument: kwargs
(ARG001)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
120-120: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
154-154: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
packages/nvidia_nat_test/src/nat/test/llm.py
70-70: Unused function argument: builder
(ARG001)
76-76: Unused function argument: builder
(ARG001)
103-103: Unused function argument: builder
(ARG001)
138-138: Unused function argument: builder
(ARG001)
153-153: Unused function argument: builder
(ARG001)
182-182: Unused function argument: builder
(ARG001)
🔇 Additional comments (11)
packages/nvidia_nat_test/src/nat/test/llm.py (1)
36-205: LGTM! Class rename and decorator updates are consistent.The rename from
TestLLMConfigtoMockLLMConfigis implemented consistently across all framework registrations. The registry name"nat_test_llm"remains unchanged, preserving backward compatibility for YAML configurations. All type hints, docstrings, and implementations follow the coding guidelines.examples/frameworks/nat_autogen_demo/README.md (3)
1-16: License header is correct and complete.SPDX notice and Apache-2.0 terms are present and up to date. Good.
39-42: Install steps LGTM.Clear, minimal commands; prerelease flag is gone. Mentions simple_calculator dependency purpose.
17-17: Verify necessity of path-check directive.Is
<!-- path-check-skip-file -->required here? If not, remove to avoid masking path issues.packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py (1)
1-22: LGTM!The registration module is complete with all required imports (llm, tool_wrapper, and autogen_callback_handler). The file correctly serves as the entry point for the AutoGen plugin as specified in the package configuration.
packages/nvidia_nat_autogen/pyproject.toml (2)
19-27: LGTM!The dependency list correctly places
nvidia-natfirst, followed by autogen dependencies in alphabetical order. This ordering aligns with the maintainer's guidance thatnvidia-natshould always be listed first in package dependencies.
48-49: LGTM!The entry point configuration correctly maps
nat_autogento the registration module, enabling the AutoGen plugin to be discovered by the NAT component system.packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
58-63: LGTM!The return type change to
Anyis appropriate and well-documented, allowing the function to returnFunctionToolinstances while maintaining flexibility for the AutoGen integration.
123-124: LGTM!The dataclass-to-Pydantic conversion using
pydantic_dataclassis correct. This properly converts dataclass schemas to Pydantic models for AutoGen compatibility.packages/nvidia_nat_autogen/tests/test_tool_wrapper.py (1)
1-296: LGTM!The test module provides comprehensive coverage of the AutoGen tool wrapper functionality, including:
- Type resolution for various union and complex types
- Wrapper creation for streaming and non-streaming functions
- Async invoke and stream operations
- Decorator behavior and metadata handling
- Signature construction with and without schemas
The tests are well-structured and clearly document expected behavior.
packages/nvidia_nat_autogen/tests/test_llm.py (1)
1-635: LGTM overall with one critical fix needed!The test suite provides comprehensive coverage of AutoGen LLM integration including patching, configuration, thinking injection, and multiple client types. Most tests are well-structured and thorough.
The critical issue with
async withusage for async generators (flagged in separate comment) needs to be addressed before merging.
|
/ok to test 7ee669f |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
examples/frameworks/nat_autogen_demo/configs/config.yml (2)
50-54: Consider using YAML multiline syntax for readability.The multi-line instructions use implicit string concatenation, which works but reduces readability. Consider using YAML's literal block scalar (
|) or folded block scalar (>) syntax.Apply this diff for better readability:
- query_processing_agent_instructions: You are an agent that provides the current weather and time information. - When asked about the weather, provide the current weather conditions. - When asked about the time, provide the current local time. - If asked about anything else, respond with 'I can only provide weather and time information.' - Once you are done, reply with the text 'DONE'. + query_processing_agent_instructions: | + You are an agent that provides the current weather and time information. + When asked about the weather, provide the current weather conditions. + When asked about the time, provide the current local time. + If asked about anything else, respond with 'I can only provide weather and time information.' + Once you are done, reply with the text 'DONE'.
56-58: Consider using YAML multiline syntax for readability.Same issue as the query processing agent instructions - consider using YAML's literal block scalar syntax.
Apply this diff:
- final_response_agent_instructions: You are the final response agent. - Your role is to provide a concise and clear answer based on the information provided by other agents. - Once you are done, reply with the final answer and then say 'APPROVE'. + final_response_agent_instructions: | + You are the final response agent. + Your role is to provide a concise and clear answer based on the information provided by other agents. + Once you are done, reply with the final answer and then say 'APPROVE'.packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (1)
146-147: Consider direct assignment instead ofsetattr()for constant attributes.Static analysis flags that
setattr()with constant attribute names is unnecessary and less idiomatic than direct assignment. While this works correctly, direct assignment is clearer.Apply this diff:
- setattr(func_to_wrap, "__signature__", inspect.Signature(parameters=params)) - setattr(func_to_wrap, "__annotations__", annotations) + func_to_wrap.__signature__ = inspect.Signature(parameters=params) + func_to_wrap.__annotations__ = annotationspackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (1)
77-79: Consider using unpacking for list construction.Static analysis suggests using
[system_message, *messages]instead of[system_message] + messagesfor better performance and idiomaticity.Apply this diff:
system_message = SystemMessage(content=self.system_prompt) - new_messages = [system_message] + messages + new_messages = [system_message, *messages] return FunctionArgumentWrapper(new_messages, *args, **kwargs)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/frameworks/nat_autogen_demo/README.md(1 hunks)examples/frameworks/nat_autogen_demo/configs/config.yml(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/tests/test_llm.py(1 hunks)packages/nvidia_nat_autogen/tests/test_tool_wrapper.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/frameworks/nat_autogen_demo/README.md
- packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pyexamples/frameworks/nat_autogen_demo/configs/config.ymlpackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pyexamples/frameworks/nat_autogen_demo/configs/config.ymlpackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.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_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.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/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pyexamples/frameworks/nat_autogen_demo/configs/config.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/frameworks/nat_autogen_demo/configs/config.yml
**/configs/**
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must be stored next to that code in a configs/ folder
Files:
examples/frameworks/nat_autogen_demo/configs/config.yml
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.py
🧬 Code graph analysis (6)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
src/nat/builder/context.py (3)
Context(115-299)intermediate_step_manager(169-180)output(58-59)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/profiler/decorators/function_tracking.py (1)
push_intermediate_step(60-79)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (9)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)src/nat/llm/utils/thinking.py (3)
BaseThinkingInjector(57-81)FunctionArgumentWrapper(34-53)patch_with_thinking(121-215)src/nat/utils/exception_handlers/automatic_retries.py (1)
patch_with_retry(269-342)src/nat/utils/type_utils.py (1)
override(56-57)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (3)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/utils/type_utils.py (1)
DecomposedType(60-488)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (5)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/builder/function_info.py (2)
FunctionInfo(290-625)create(351-549)src/nat/data_models/component_ref.py (1)
LLMRef(116-124)src/nat/data_models/function.py (1)
FunctionBaseConfig(26-27)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (5)
AutoGenProfilerHandler(37-391)instrument(60-89)uninstrument(91-105)_llm_call_monkey_patch(107-274)_tool_call_monkey_patch(276-391)src/nat/builder/context.py (1)
intermediate_step_manager(169-180)
packages/nvidia_nat_autogen/tests/test_llm.py (3)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (4)
_patch_autogen_client_based_on_config(39-100)openai_autogen(104-142)azure_openai_autogen(146-189)nim_autogen(193-231)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
136-136: Do not catch blind exception: Exception
(BLE001)
137-137: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
153-153: Do not catch blind exception: Exception
(BLE001)
154-154: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
179-179: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
201-201: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
227-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
305-305: Do not catch blind exception: Exception
(BLE001)
306-306: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
331-331: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
352-352: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
78-78: Consider [system_message, *messages] instead of concatenation
Replace with [system_message, *messages]
(RUF005)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
113-113: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
147-147: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
161-161: Unused function argument: args
(ARG001)
161-161: Unused function argument: kwargs
(ARG001)
207-207: Unused function argument: args
(ARG001)
207-207: Unused function argument: kwargs
(ARG001)
326-326: Do not assert blind exception: Exception
(B017)
packages/nvidia_nat_autogen/tests/test_llm.py
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Unused function argument: args
(ARG001)
306-306: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (18)
examples/frameworks/nat_autogen_demo/configs/config.yml (2)
16-30: LGTM!The general configuration is well-structured with appropriate defaults for a demo application. The permissive CORS setting is acceptable for demo purposes, and the telemetry configuration provides good observability.
39-42: LGTM!The function configuration is clear and follows the expected structure for tool definitions.
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
46-53: LGTM!Initialization is correct with proper thread-safety via
threading.Lock, and correctly retrieves the step manager from context.
60-89: LGTM!The instrumentation logic correctly handles the idempotency check, gracefully handles import failures, and properly saves original references for later restoration.
91-105: LGTM!The uninstrumentation logic properly restores the original methods and handles failures gracefully with appropriate logging.
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (3)
31-43: LGTM!The configuration class is well-structured with appropriate fields to make the workflow configurable. The MCP server URL is now properly exposed as a configuration option.
60-87: LGTM!The workflow setup correctly retrieves resources from the builder, configures the MCP adapter, and creates agents with user-configurable names and instructions. The use of
TextMentionTermination("APPROVE")provides clear termination semantics.
89-119: LGTM!The workflow execution logic properly handles the async generator pattern, provides appropriate error messages to users, and includes proper cleanup logging. The error handling is appropriate for a workflow that returns string results.
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
37-48: LGTM!The function correctly delegates to the existing
DecomposedTypeutility for type resolution, avoiding code duplication.
157-167: LGTM!The tool wrapper correctly selects between streaming and non-streaming implementations based on the function's capabilities and properly constructs the AutoGen
FunctionTool.packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (3)
29-128: LGTM!The test class provides comprehensive coverage of initialization, instrumentation, and state management with proper mocking and assertions.
294-330: LGTM!The test correctly verifies exception propagation using
pytest.raises, which aligns with the implementation's error handling that re-raises exceptions after logging and pushing telemetry.
461-497: LGTM!The test correctly verifies that tool execution failures propagate exceptions as expected, with proper logging verification.
packages/nvidia_nat_autogen/tests/test_llm.py (2)
208-249: LGTM!The test correctly consumes the async generator using the
__anext__()pattern, which properly handles the generator protocol.
251-336: LGTM!Both Azure OpenAI and NIM tests correctly use the async generator consumption pattern with proper mocking.
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (3)
103-142: LGTM!The OpenAI client creation correctly extracts configuration, sets up model info, and applies NAT mixins before yielding the client.
145-189: LGTM!The Azure OpenAI client creation correctly constructs the Azure-specific base URL and uses the deployment name as the model identifier, following Azure OpenAI API conventions.
192-231: LGTM!The NIM client creation correctly leverages OpenAI API compatibility, which is the standard approach for NIM integration with frameworks that don't have native NIM clients.
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
Outdated
Show resolved
Hide resolved
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (2)
154-154: Remove redundant exception object fromlogger.exception()call.When using
logger.exception(), the exception and traceback are automatically captured, so passing the exception object explicitly is redundant.Based on coding guidelines.
Apply this diff:
- logger.exception("Error getting model input: %s", _e) + logger.exception("Error getting model input")
306-306: Remove redundant exception object fromlogger.exception()call.When using
logger.exception(), the exception and traceback are automatically captured, so passing the exception object explicitly is redundant.Based on coding guidelines.
Apply this diff:
- logger.exception("Error getting tool name: %s", _e) + logger.exception("Error getting tool name")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.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:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.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_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
🧠 Learnings (1)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to **/*.py : Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Applied to files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
🧬 Code graph analysis (2)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (5)
AutoGenProfilerHandler(37-391)instrument(60-89)uninstrument(91-105)_llm_call_monkey_patch(107-274)_tool_call_monkey_patch(276-391)src/nat/builder/context.py (1)
intermediate_step_manager(169-180)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (2)
src/nat/builder/context.py (3)
Context(115-299)intermediate_step_manager(169-180)output(58-59)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
161-161: Unused function argument: args
(ARG001)
161-161: Unused function argument: kwargs
(ARG001)
207-207: Unused function argument: args
(ARG001)
207-207: Unused function argument: kwargs
(ARG001)
326-326: Do not assert blind exception: Exception
(B017)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
154-154: Redundant exception object included in logging.exception call
(TRY401)
179-179: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
201-201: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
227-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
306-306: Redundant exception object included in logging.exception call
(TRY401)
331-331: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
352-352: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (1)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (1)
1-614: Comprehensive test coverage for AutoGen profiler handler.The test suite is well-structured and provides excellent coverage of the AutoGenProfilerHandler functionality, including:
- Initialization and state management
- Instrumentation/uninstrumentation lifecycles with various scenarios (already instrumented, import failures, success paths)
- Lock mechanism and timestamp tracking
- LLM call monkey-patch with multiple test cases (basic flow, exceptions, model name fallback, complex content)
- Tool call monkey-patch with various input formats
- Error handling paths for model name retrieval and input processing
- Integration with Context and step manager
- Exception propagation verification (properly validates that exceptions are raised rather than swallowed)
The tests properly validate that the implementation now correctly propagates exceptions rather than returning error strings, which aligns with the coding guidelines.
saglave
force-pushed
the
autogen_integration
branch
2 times, most recently
from
021ba3a to
8881294
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (1)
26-27: Restore proper schema conversion and preserve original signatures.Calling
pydantic_dataclasson the schema type still leaves you with a dataclass that lacksmodel_fields, so the loop never runs and we overwrite the wrapped callable’s signature with an empty parameter list. We also still stomp the signature/annotations even when no schema was supplied, repeating the regression flagged earlier. Convert dataclasses withcreate_model_from_dataclassand only assign a new signature when we actually build one.-from pydantic.dataclasses import dataclass as pydantic_dataclass +from pydantic.dataclasses import create_model_from_dataclass @@ - if input_schema is not None and is_dataclass(input_schema): - input_schema = pydantic_dataclass(input_schema) + if input_schema is not None and is_dataclass(input_schema): + input_schema = create_model_from_dataclass(input_schema) @@ - if input_schema is not None: - annotations = {} - params = [] - model_fields = getattr(input_schema, "model_fields", {}) - for param_name, model_field in model_fields.items(): - resolved_type = resolve_type(model_field.annotation) - default = model_field.default if model_field.is_required() is False else inspect._empty - params.append( - inspect.Parameter(param_name, - inspect.Parameter.POSITIONAL_OR_KEYWORD, - annotation=resolved_type, - default=default)) - annotations[param_name] = resolved_type - setattr(func_to_wrap, "__signature__", inspect.Signature(parameters=params)) - setattr(func_to_wrap, "__annotations__", annotations) + if input_schema is not None: + params = [] + annotations: dict[str, Any] = {} + model_fields = getattr(input_schema, "model_fields", {}) + for param_name, model_field in model_fields.items(): + resolved_type = resolve_type(model_field.annotation) + default = model_field.default if model_field.is_required() is False else inspect._empty + params.append( + inspect.Parameter(param_name, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + annotation=resolved_type, + default=default)) + annotations[param_name] = resolved_type + func_to_wrap.__signature__ = inspect.Signature(parameters=params) + func_to_wrap.__annotations__ = annotationsAlso applies to: 116-147
🧹 Nitpick comments (1)
packages/nvidia_nat_test/src/nat/test/llm.py (1)
102-134: Add docstring for consistency.This function is missing a docstring while all other test LLM client registration functions in this file have them (lines 77, 139, 154, 183). For consistency, add a brief docstring.
Apply this diff to add a docstring:
@register_llm_client(config_type=MockLLMConfig, wrapper_type=LLMFrameworkEnum.LLAMA_INDEX) async def test_llm_llama_index(config: MockLLMConfig, builder: Builder): + """LLM client for LlamaIndex."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
examples/frameworks/nat_autogen_demo/README.md(1 hunks)examples/frameworks/nat_autogen_demo/configs/config.yml(1 hunks)examples/frameworks/nat_autogen_demo/pyproject.toml(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/__init__.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py(1 hunks)packages/nvidia_nat_autogen/pyproject.toml(1 hunks)packages/nvidia_nat_autogen/src/nat/meta/pypi.md(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)packages/nvidia_nat_autogen/tests/__init__.py(1 hunks)packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/tests/test_llm.py(1 hunks)packages/nvidia_nat_autogen/tests/test_register.py(1 hunks)packages/nvidia_nat_autogen/tests/test_tool_wrapper.py(1 hunks)packages/nvidia_nat_test/src/nat/test/llm.py(6 hunks)pyproject.toml(5 hunks)src/nat/builder/framework_enum.py(1 hunks)src/nat/profiler/decorators/framework_wrapper.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/nvidia_nat_autogen/src/nat/plugins/autogen/init.py
- packages/nvidia_nat_autogen/tests/init.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/nat/builder/framework_enum.py
- packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py
- packages/nvidia_nat_autogen/pyproject.toml
- examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/init.py
- src/nat/profiler/decorators/framework_wrapper.py
- packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
- packages/nvidia_nat_autogen/src/nat/meta/pypi.md
- examples/frameworks/nat_autogen_demo/README.md
- examples/frameworks/nat_autogen_demo/configs/config.yml
- examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py
- examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
🧰 Additional context used
📓 Path-based instructions (8)
**/*
⚙️ 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/frameworks/nat_autogen_demo/pyproject.tomlpackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypyproject.tomlpackages/nvidia_nat_autogen/tests/test_register.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.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/frameworks/nat_autogen_demo/pyproject.tomlexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_register.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_register.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.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_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/test_llm.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
🧠 Learnings (3)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to packages/*/pyproject.toml : In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Applied to files:
examples/frameworks/nat_autogen_demo/pyproject.tomlpyproject.toml
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to **/*.py : Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Applied to files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
📚 Learning: 2025-09-09T20:32:39.016Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/nat-test-llm.mdc:0-0
Timestamp: 2025-09-09T20:32:39.016Z
Learning: Applies to **/*.py : Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("<name>", cfg).
Applied to files:
packages/nvidia_nat_test/src/nat/test/llm.py
🧬 Code graph analysis (8)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
src/nat/builder/context.py (3)
Context(117-315)intermediate_step_manager(171-182)output(58-59)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/profiler/decorators/function_tracking.py (1)
push_intermediate_step(60-79)
packages/nvidia_nat_autogen/tests/test_llm.py (6)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (4)
_patch_autogen_client_based_on_config(39-100)openai_autogen(104-142)azure_openai_autogen(146-189)nim_autogen(193-231)
packages/nvidia_nat_test/src/nat/test/llm.py (2)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (9)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)src/nat/llm/utils/thinking.py (3)
BaseThinkingInjector(57-81)FunctionArgumentWrapper(34-53)patch_with_thinking(121-215)src/nat/utils/exception_handlers/automatic_retries.py (1)
patch_with_retry(269-342)src/nat/utils/type_utils.py (1)
override(56-57)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (3)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/utils/type_utils.py (1)
DecomposedType(60-488)
packages/nvidia_nat_autogen/tests/test_register.py (1)
tests/nat/builder/test_builder.py (1)
tool_wrapper(554-564)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py (1)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (1)
autogen_team(47-119)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (5)
AutoGenProfilerHandler(37-391)instrument(60-89)uninstrument(91-105)_llm_call_monkey_patch(107-274)_tool_call_monkey_patch(276-391)src/nat/builder/context.py (1)
intermediate_step_manager(171-182)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
179-179: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
201-201: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
227-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
331-331: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
352-352: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
packages/nvidia_nat_autogen/tests/test_llm.py
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Unused function argument: args
(ARG001)
306-306: Unused function argument: kwargs
(ARG001)
packages/nvidia_nat_test/src/nat/test/llm.py
70-70: Unused function argument: builder
(ARG001)
76-76: Unused function argument: builder
(ARG001)
103-103: Unused function argument: builder
(ARG001)
138-138: Unused function argument: builder
(ARG001)
153-153: Unused function argument: builder
(ARG001)
182-182: Unused function argument: builder
(ARG001)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
78-78: Consider [system_message, *messages] instead of concatenation
Replace with [system_message, *messages]
(RUF005)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
113-113: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
147-147: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
17-17: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
161-161: Unused function argument: args
(ARG001)
161-161: Unused function argument: kwargs
(ARG001)
207-207: Unused function argument: args
(ARG001)
207-207: Unused function argument: kwargs
(ARG001)
326-326: Do not assert blind exception: Exception
(B017)
🔇 Additional comments (14)
packages/nvidia_nat_test/src/nat/test/llm.py (8)
1-34: LGTM!License header, copyright, and imports are all correct and properly formatted.
36-43: LGTM!The
MockLLMConfigclass is properly defined with clear field descriptions and appropriate constraints. The rename fromTestLLMConfigis complete while preserving backward compatibility via thename="nat_test_llm"metadata.
46-67: LGTM!The
_ResponseChooserhelper class correctly implements response cycling and delay injection. The logic properly handles the empty response sequence case by returning an empty string.
69-72: LGTM!Provider registration is correctly implemented with proper metadata. The unused
builderparameter is intentional for this test infrastructure.
75-99: LGTM!LangChain test LLM implementation is correct with proper sync/async method pairs and streaming support. The methods correctly apply delays and return/yield responses according to the configured sequence.
137-149: LGTM!CrewAI test LLM implementation correctly provides the synchronous
callmethod required by the framework.
152-178: LGTM!Semantic Kernel test LLM implementation correctly provides async methods that return properly formatted
ChatMessageContentobjects with the assistant role.
181-205: LGTM!Agno test LLM implementation is correct with proper sync/async method pairs and streaming support. The implementation follows the expected pattern for Agno framework integration.
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (6)
1-35: LGTM!The license header, module docstring, and imports are properly structured and follow coding guidelines.
37-59: LGTM!The class definition and constructor are well-structured with accurate docstrings and proper type hints.
60-90: LGTM!The instrumentation logic is well-designed with:
- Idempotent behavior (early return if already instrumented)
- Graceful degradation when AutoGen dependencies are unavailable
- Proper preservation of original methods for later restoration
The exception handling on line 75 correctly uses
logger.exception()since the exception is not re-raised.
91-106: LGTM!The uninstrumentation method properly restores original AutoGen methods and correctly handles exceptions with
logger.exception()since errors are not re-raised.
107-274: LGTM!The LLM call wrapper is well-implemented with:
- Comprehensive telemetry collection (input, output, usage, timing)
- Proper error handling that preserves stack traces with bare
raise- Thread-safe timestamp management
- Graceful degradation for non-critical metadata extraction failures
The exception handling correctly follows coding guidelines: using
logger.error()when re-raising (lines 179, 201, 227) andlogger.exception()when not re-raising (lines 137, 154, 252).Note: The static analysis tool incorrectly flags lines 179, 201, and 227, but these are false positives—each of these error logs is followed by a bare
raisestatement, sologger.error()is the appropriate choice per coding guidelines.
276-391: LGTM!The tool call wrapper is well-implemented with:
- Comprehensive telemetry collection (input, output, timing)
- Multiple fallback strategies for extracting tool input (lines 327-329)
- Proper error handling that preserves stack traces with bare
raise- Thread-safe timestamp management
The exception handling correctly follows coding guidelines: using
logger.error()when re-raising (lines 331, 352) andlogger.exception()when not re-raising (line 306).Note: The static analysis tool incorrectly flags lines 331 and 352, but these are false positives—each of these error logs is followed by a bare
raisestatement, sologger.error()is the appropriate choice per coding guidelines.
saglave
force-pushed
the
autogen_integration
branch
from
6640bc2 to
b34f5d4
Compare
Signed-off-by: Onkar Kulkarni <[email protected]> Co-authored-by: Sangharsh Aglave <[email protected]> Co-authored-by: Bhuvnesh Kumar <[email protected]> Co-authored-by: Pasan Udawatta <[email protected]>
onkkul
force-pushed
the
autogen_integration
branch
from
b34f5d4 to
5cb22d6
Compare
onkkul
changed the title
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)
packages/nvidia_nat_test/src/nat/test/llm.py (1)
208-236: Fix the remainingTestLLMConfigreference.
TestLLMConfigno longer exists, so importing this module now raisesNameError. Update the ADK registration to useMockLLMConfiglike the rest of the file.-@register_llm_client(config_type=TestLLMConfig, wrapper_type=LLMFrameworkEnum.ADK) -async def test_llm_adk(config: TestLLMConfig, builder: Builder): +@register_llm_client(config_type=MockLLMConfig, wrapper_type=LLMFrameworkEnum.ADK) +async def test_llm_adk(config: MockLLMConfig, builder: Builder):
♻️ Duplicate comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (1)
118-140: Avoid duplicatemodel=when constructing AutoGen clients.Line 139 (and Line 228) unpack
config_objthat still contains"model", soOpenAIChatCompletionClientsees twomodelkeywords and raisesTypeError. Please drop the alias before unpacking (same fix for the NIM block).config_obj = { **llm_config.model_dump( exclude={"type", "model_name", "thinking"}, by_alias=True, exclude_none=True, ), } + config_obj.pop("model", None) @@ config_obj = { **llm_config.model_dump( exclude={"type", "model_name", "thinking"}, by_alias=True, exclude_none=True, ), } + config_obj.pop("model", None)Also applies to: 207-229
🧹 Nitpick comments (2)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py (1)
16-17: Optional: Remove unusednoqadirectives.Static analysis indicates that
noqa: F401directives are unused because the F401 rule is not enabled. While these imports are legitimately used for side effects (registration), thenoqadirectives may be unnecessary. Consider removing them if they don't serve other linter configurations in your environment.Apply this diff if confirmed unnecessary:
-from . import autogen_team # noqa: F401 # pylint: disable=W0611 #imported for side effects (registration) -from . import weather_update_tool # noqa: F401 # pylint: disable=W0611 #imported for side effects (registration) +from . import autogen_team # imported for side effects (registration) +from . import weather_update_tool # imported for side effects (registration)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (1)
66-119: Consider making hardcoded values configurable.The tool name
"current_datetime"(line 75) and termination keyword"APPROVE"(line 87) are hardcoded. While acceptable for an example, making these configurable viaAutoGenFunctionConfigwould provide additional flexibility for users who want to customize the workflow behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
examples/frameworks/nat_autogen_demo/README.md(1 hunks)examples/frameworks/nat_autogen_demo/configs/config.yml(1 hunks)examples/frameworks/nat_autogen_demo/pyproject.toml(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/__init__.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py(1 hunks)examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py(1 hunks)packages/nvidia_nat_autogen/pyproject.toml(1 hunks)packages/nvidia_nat_autogen/src/nat/meta/pypi.md(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/__init__.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)packages/nvidia_nat_autogen/tests/__init__.py(1 hunks)packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py(1 hunks)packages/nvidia_nat_autogen/tests/test_llm.py(1 hunks)packages/nvidia_nat_autogen/tests/test_register.py(1 hunks)packages/nvidia_nat_autogen/tests/test_tool_wrapper.py(1 hunks)packages/nvidia_nat_test/src/nat/test/llm.py(6 hunks)pyproject.toml(5 hunks)src/nat/builder/framework_enum.py(1 hunks)src/nat/profiler/decorators/framework_wrapper.py(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/nvidia_nat_autogen/src/nat/plugins/autogen/register.py
- packages/nvidia_nat_autogen/src/nat/plugins/autogen/init.py
- examples/frameworks/nat_autogen_demo/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- src/nat/builder/framework_enum.py
- examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/weather_update_tool.py
- src/nat/profiler/decorators/framework_wrapper.py
- packages/nvidia_nat_autogen/tests/init.py
- examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/init.py
- examples/frameworks/nat_autogen_demo/configs/config.yml
- pyproject.toml
🧰 Additional context used
📓 Path-based instructions (10)
**/*
⚙️ 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:
packages/nvidia_nat_autogen/src/nat/meta/pypi.mdpackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/pyproject.tomlexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pyexamples/frameworks/nat_autogen_demo/pyproject.tomlexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/tests/test_llm.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_autogen/src/nat/meta/pypi.mdpackages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/pyproject.tomlpackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/tests/test_llm.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:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/tests/test_llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pyexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_autogen/tests/test_llm.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, include tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.pypackages/nvidia_nat_autogen/tests/test_tool_wrapper.pypackages/nvidia_nat_autogen/tests/test_register.pypackages/nvidia_nat_autogen/tests/test_llm.py
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/pyproject.toml: Each package must contain a pyproject.toml
In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Use ~= version constraints (e.g., ~=1.0) for dependencies
Files:
packages/nvidia_nat_autogen/pyproject.toml
{packages/*/pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Add new dependencies to both pyproject.toml (alphabetically) and uv.lock via uv pip install --sync
Files:
packages/nvidia_nat_autogen/pyproject.toml
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/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.pyexamples/frameworks/nat_autogen_demo/pyproject.tomlexamples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_test/src/nat/test/llm.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.pypackages/nvidia_nat_test/src/nat/test/llm.py
🧠 Learnings (3)
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to packages/*/pyproject.toml : In packages, declare a dependency on nvidia-nat or packages starting with nvidia-nat-
Applied to files:
packages/nvidia_nat_autogen/pyproject.tomlexamples/frameworks/nat_autogen_demo/pyproject.toml
📚 Learning: 2025-09-23T18:39:15.023Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-09-23T18:39:15.023Z
Learning: Applies to **/*.py : Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Applied to files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
📚 Learning: 2025-09-09T20:32:39.016Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/nat-test-llm.mdc:0-0
Timestamp: 2025-09-09T20:32:39.016Z
Learning: Applies to **/*.py : Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("<name>", cfg).
Applied to files:
packages/nvidia_nat_test/src/nat/test/llm.py
🧬 Code graph analysis (10)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (5)
AutoGenProfilerHandler(37-391)instrument(60-89)uninstrument(91-105)_llm_call_monkey_patch(107-274)_tool_call_monkey_patch(276-391)src/nat/builder/context.py (1)
intermediate_step_manager(171-182)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py (1)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (1)
autogen_team(47-119)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (3)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/utils/type_utils.py (1)
DecomposedType(60-488)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py (3)
src/nat/builder/context.py (3)
Context(117-315)intermediate_step_manager(171-182)output(58-59)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/profiler/decorators/function_tracking.py (1)
push_intermediate_step(60-79)
packages/nvidia_nat_autogen/tests/test_tool_wrapper.py (3)
src/nat/builder/builder.py (1)
Builder(68-290)tests/nat/builder/test_builder.py (1)
tool_wrapper(554-564)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
autogen_tool_wrapper(52-167)resolve_type(37-48)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (5)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/builder/function_info.py (2)
FunctionInfo(290-625)create(351-549)src/nat/data_models/component_ref.py (1)
LLMRef(116-124)src/nat/data_models/function.py (1)
FunctionBaseConfig(26-27)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (9)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)src/nat/llm/utils/thinking.py (3)
BaseThinkingInjector(57-81)FunctionArgumentWrapper(34-53)patch_with_thinking(121-215)src/nat/utils/exception_handlers/automatic_retries.py (1)
patch_with_retry(269-342)src/nat/utils/type_utils.py (1)
override(56-57)
packages/nvidia_nat_autogen/tests/test_register.py (1)
tests/nat/builder/test_builder.py (1)
tool_wrapper(554-564)
packages/nvidia_nat_test/src/nat/test/llm.py (2)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)
packages/nvidia_nat_autogen/tests/test_llm.py (6)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (4)
_patch_autogen_client_based_on_config(39-100)openai_autogen(104-142)azure_openai_autogen(146-189)nim_autogen(193-231)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/tests/test_autogen_callback_handler.py
161-161: Unused function argument: args
(ARG001)
161-161: Unused function argument: kwargs
(ARG001)
207-207: Unused function argument: args
(ARG001)
207-207: Unused function argument: kwargs
(ARG001)
326-326: Do not assert blind exception: Exception
(B017)
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/register.py
16-16: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
17-17: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
113-113: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
147-147: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/autogen_callback_handler.py
179-179: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
201-201: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
227-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
331-331: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
352-352: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
78-78: Consider [system_message, *messages] instead of concatenation
Replace with [system_message, *messages]
(RUF005)
packages/nvidia_nat_test/src/nat/test/llm.py
70-70: Unused function argument: builder
(ARG001)
76-76: Unused function argument: builder
(ARG001)
103-103: Unused function argument: builder
(ARG001)
138-138: Unused function argument: builder
(ARG001)
153-153: Unused function argument: builder
(ARG001)
182-182: Unused function argument: builder
(ARG001)
packages/nvidia_nat_autogen/tests/test_llm.py
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
306-306: Unused function argument: args
(ARG001)
306-306: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (4)
packages/nvidia_nat_autogen/src/nat/meta/pypi.md (1)
1-23: LGTM!The documentation file is well-structured with proper Apache 2.0 license header, current copyright year, and clear package description. The content is concise and links are appropriate.
examples/frameworks/nat_autogen_demo/pyproject.toml (1)
1-24: LGTM!The project configuration is clean and properly structured. The dependency on
nvidia-nat[autogen]~=1.4correctly pulls in the AutoGen integration, avoiding duplicate dependency declarations. Past review concerns have been addressed.examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py (1)
31-43: LGTM!The configuration class is well-designed with appropriate fields for customizing agent behavior. Making the MCP server URL, agent names, and agent instructions configurable addresses previous review feedback and provides good flexibility for users.
packages/nvidia_nat_autogen/pyproject.toml (1)
19-27: Dependency order is correct. nvidia-nat is first, and the remaining dependencies are alphabetically sorted.
examples/frameworks/nat_autogen_demo/src/nat_autogen_demo/autogen_team.py
Show resolved
Hide resolved
| # If input_schema is a dataclass, convert it to a Pydantic model | ||
| if input_schema is not None and is_dataclass(input_schema): | ||
| input_schema = pydantic_dataclass(input_schema) | ||
|
|
There was a problem hiding this comment.
Dataclass schemas must become BaseModel subclasses before signature extraction.
pydantic_dataclass leaves you with a dataclass (no model_fields), so the subsequent loop never populates parameters. AutoGen then sees an empty signature. Use create_model_from_dataclass so we get a real Pydantic model.
-from pydantic.dataclasses import dataclass as pydantic_dataclass
+from pydantic.dataclasses import create_model_from_dataclass
@@
- if input_schema is not None and is_dataclass(input_schema):
- input_schema = pydantic_dataclass(input_schema)
+ if input_schema is not None and is_dataclass(input_schema):
+ input_schema = create_model_from_dataclass(input_schema)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # If input_schema is a dataclass, convert it to a Pydantic model | |
| if input_schema is not None and is_dataclass(input_schema): | |
| input_schema = pydantic_dataclass(input_schema) | |
| ++ b/packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py | |
| @@ -1,5 +1,5 @@ | |
| from pydantic.dataclasses import create_model_from_dataclass | |
| # If input_schema is a dataclass, convert it to a Pydantic model | |
| - if input_schema is not None and is_dataclass(input_schema): | |
| if input_schema is not None and is_dataclass(input_schema): | |
| input_schema = create_model_from_dataclass(input_schema) |
🤖 Prompt for AI Agents
In packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py around
lines 115 to 118, the code converts dataclass input_schema using
pydantic_dataclass which returns a dataclass (no model_fields) so signature
extraction yields no parameters; replace pydantic_dataclass with
create_model_from_dataclass so the result is a real Pydantic BaseModel with
model_fields; ensure the create_model_from_dataclass symbol is imported from
pydantic (or the proper utility) and use it in place of pydantic_dataclass so
downstream signature extraction works.
There was a problem hiding this comment.
create_model_from_dataclass method does not exist in pydantic model.
> python
Python 3.12.11 (main, Jul 1 2025, 18:37:24) [Clang 20.1.4 ] on linux
Type "help", "copyright", "credits" or "license" for more information.
Ctrl click to launch VS Code Native REPL
>>>from pydantic.dataclasses import create_model_from_dataclassTraceback (most recent call last):
File "<stdin>", line 1, in <module>
ImportError: cannot import name 'create_model_from_dataclass' from 'pydantic.dataclasses'
> uv pip show pydantic
Name: pydantic
Version: 2.11.9
Location: ./.venv/lib/python3.12/site-packages
Requires: annotated-types, pydantic-core, typing-extensions, typing-inspection
Required-by: agno, anthropic, arize-phoenix, arize-phoenix-evals, autogen-core, banks, chromadb, confection, crewai, fastapi, google-adk, google-cloud-aiplatform, google-genai, groq, haystack-ai, instructor, langchain, langchain-aws, langchain-core, langgraph, langsmith, litellm, llama-cloud, llama-cloud-services, llama-index-core, llama-index-instrumentation, llama-index-workflows, mcp, mcp-server-time, mem0ai, nat-profiler-agent, nvidia-nat, openai, pydantic-settings, qdrant-client, ragas, semantic-kernel, spacy, thinc, wandb, weasel, weave, zep-cloudThere was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How to convert a Python dataclass to Pydantic BaseModel in Pydantic v2 with model_fields
💡 Result:
Short recipe + example.
References: Pydantic v2 docs for BaseModel.model_fields [1] and dataclass/TypeAdapter guidance [2].
Code (works in Pydantic v2):
from dataclasses import fields as dc_fields, MISSING
from pydantic import BaseModel, Field
def dataclass_to_basemodel(dc):
# collect annotations and pydantic Field info
annotations = {}
model_fields = {}
for f in dc_fields(dc):
annotations[f.name] = f.type
if f.default is not MISSING:
fi = Field(default=f.default)
elif getattr(f, "default_factory", MISSING) is not MISSING:
# dataclasses.field may provide a default_factory
fi = Field(default_factory=f.default_factory)
else:
fi = Field(...) # required
model_fields[f.name] = fi
attrs = {"__annotations__": annotations, "model_fields": model_fields}
return type(dc.__name__ + "Model", (BaseModel,), attrs)Notes:
- This dynamically creates a BaseModel subclass whose classvar model_fields (dict[str, FieldInfo]) mirrors the dataclass fields — matching Pydantic v2 internals. Use typing.Annotated or Field(...) to carry extra metadata if needed. See BaseModel.model_fields docs. [1]
- If you only need validation/serialization for a dataclass, consider wrapping the dataclass with TypeAdapter instead of converting to a BaseModel (recommended in v2 migration docs). [2]
Sources:
[1] Pydantic BaseModel API (model_fields).
[2] Pydantic v2 migration / dataclasses (TypeAdapter recommendation).
@onkkul, you're absolutely right—I apologize for the incorrect suggestion. create_model_from_dataclass doesn't exist in Pydantic v2.
Here's the correct approach using a helper function to convert a dataclass to a Pydantic BaseModel:
from dataclasses import fields as dc_fields, MISSING
from pydantic import BaseModel, Field
def dataclass_to_basemodel(dc: Any) -> type[BaseModel]:
"""Convert a dataclass to a Pydantic BaseModel with model_fields.
Args:
dc: The dataclass to convert.
Returns:
A dynamically created BaseModel subclass.
"""
annotations = {}
field_definitions = {}
for f in dc_fields(dc):
annotations[f.name] = f.type
if f.default is not MISSING:
field_definitions[f.name] = Field(default=f.default)
elif f.default_factory is not MISSING: # type: ignore[comparison-overlap]
field_definitions[f.name] = Field(default_factory=f.default_factory)
else:
field_definitions[f.name] = Field(...) # required field
# Create a new BaseModel subclass dynamically
return type(
f"{dc.__name__}Model",
(BaseModel,),
{"__annotations__": annotations, **field_definitions}
)Then update the conversion line:
# If input_schema is a dataclass, convert it to a Pydantic model
if input_schema is not None and is_dataclass(input_schema):
input_schema = dataclass_to_basemodel(input_schema)This will create a proper BaseModel subclass with model_fields that the downstream signature extraction code can use.
_ _
( ` )_
( ) `)
(_ (_ . _) _)
[learn from mistakes]
There was a problem hiding this comment.
Why cannot I use what I have right now?: It works perfectly.
from pydantic.dataclasses import dataclass as pydantic_dataclass
if input_schema is not None and is_dataclass(input_schema):
input_schema = pydantic_dataclass(input_schema)There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the complete content of the tool_wrapper.py file to see how input_schema is used
cat packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
Length of output: 6318
🏁 Script executed:
#!/bin/bash
# Check the test file to see if dataclass conversion is tested
cat packages/nvidia_nat_autogen/tests/test_tool_wrapper.py | grep -A 20 -B 5 "dataclass"
Length of output: 1332
🏁 Script executed:
#!/bin/bash
# Search for tests that use MockDataclassSchema to see if dataclass conversion is tested
rg -A 30 "MockDataclassSchema" packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
Length of output: 996
🏁 Script executed:
#!/bin/bash
# Search for test cases that use dataclass schema
rg -B 5 -A 50 "test.*dataclass" packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get the full test file content to see all test cases
cat packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
Length of output: 10863
🌐 Web query:
Pydantic v2 dataclass attributes: does pydantic.dataclasses.dataclass provide model_fields or __pydantic_fields__
💡 Result:
Short answer: No.
Details: In Pydantic v2, pydantic.dataclasses.dataclass instances/types do not expose model_fields or pydantic_fields (and v2 removed pydantic_model / the BaseModel backing). Use stdlib dataclasses.field/ dataclasses.fields or TypeAdapter for validation/metadata instead. (docs.pydantic.dev)
Citations:
🏁 Script executed:
#!/bin/bash
# Let's create a test script to see what attributes pydantic dataclasses have
cat > /tmp/test_pydantic_dataclass.py << 'EOF'
from dataclasses import dataclass, fields as dc_fields
from pydantic.dataclasses import dataclass as pydantic_dataclass
@dataclass
class StandardDataclass:
param1: str
param2: int
@pydantic_dataclass
class PydanticDataclass:
param1: str
param2: int
# Test what attributes each has
print("Standard dataclass attributes:")
print(" hasattr model_fields:", hasattr(StandardDataclass, "model_fields"))
print(" hasattr __pydantic_fields__:", hasattr(StandardDataclass, "__pydantic_fields__"))
print(" dataclasses.fields works:", len(dc_fields(StandardDataclass)))
print("\nPydantic dataclass attributes:")
print(" hasattr model_fields:", hasattr(PydanticDataclass, "model_fields"))
print(" hasattr __pydantic_fields__:", hasattr(PydanticDataclass, "__pydantic_fields__"))
print(" dataclasses.fields works:", len(dc_fields(PydanticDataclass)))
print("\nPydantic dataclass dir (filtered):")
attrs = [a for a in dir(PydanticDataclass) if not a.startswith('_') or a.startswith('__pydantic')]
print(" ", attrs[:10])
EOF
python /tmp/test_pydantic_dataclass.py
Length of output: 505
@onkkul, great question! You can use pydantic_dataclass, but there's a bug in the downstream code (line 136) that prevents it from working correctly.
Here's the issue:
The Problem
At line 136, the code tries to extract parameters using:
model_fields = getattr(input_schema, "model_fields", {})However, as I verified in the sandbox:
- Pydantic dataclasses have
__pydantic_fields__, NOTmodel_fields - Standard dataclasses don't have either
model_fieldsonly exists on PydanticBaseModelclasses
This means when you pass a dataclass (standard or Pydantic), getattr returns an empty dict {}, so no parameters are extracted and you get an empty function signature.
The Fix
You need to handle both dataclasses and BaseModels. Here's the corrected code for lines 133-146:
# Set signature only if input_schema is provided
params: list[inspect.Parameter] = []
annotations: dict[str, Any] = {}
if input_schema is not None:
from dataclasses import fields as dc_fields
annotations = {}
params = []
# Handle dataclasses (both standard and Pydantic)
if is_dataclass(input_schema):
for field in dc_fields(input_schema):
resolved_type = resolve_type(field.type)
default = field.default if field.default is not inspect._empty else inspect._empty
params.append(
inspect.Parameter(field.name,
inspect.Parameter.POSITIONAL_OR_KEYWORD,
annotation=resolved_type,
default=default))
annotations[field.name] = resolved_type
# Handle Pydantic BaseModels
else:
model_fields = getattr(input_schema, "model_fields", {})
for param_name, model_field in model_fields.items():
resolved_type = resolve_type(model_field.annotation)
default = model_field.default if model_field.is_required() is False else inspect._empty
params.append(
inspect.Parameter(param_name,
inspect.Parameter.POSITIONAL_OR_KEYWORD,
annotation=resolved_type,
default=default))
annotations[param_name] = resolved_typeThis approach:
- Uses
dataclasses.fields()for both standard and Pydantic dataclasses (it works for both!) - Uses
model_fieldsonly for Pydantic BaseModel classes - Preserves your existing
pydantic_dataclassconversion at line 118
✏️ Learnings added
Learnt from: onkkul
PR: NVIDIA/NeMo-Agent-Toolkit#859
File: packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py:115-118
Timestamp: 2025-10-09T22:21:15.918Z
Learning: In `packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py`, when extracting parameters from input_schema at line 136, the code must handle both dataclasses (standard and Pydantic) and Pydantic BaseModels differently. Pydantic dataclasses have `__pydantic_fields__` (not `model_fields`), so use `dataclasses.fields()` for dataclasses and `model_fields` for BaseModels to properly extract function parameters.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Onkar Kulkarni <[email protected]>
Signed-off-by: Onkar Kulkarni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (1)
203-223: Missingpop("model")causes duplicate keyword argument.
NIMModelConfig.model_namehasserialization_alias="model", somodel_dump(by_alias=True)emits a"model"key. Without removing it,OpenAIChatCompletionClientreceives bothmodel="..."from the dict andmodel=llm_config.model_nameexplicitly, raisingTypeError.Apply the same fix as
openai_autogen:# Add required AutoGen 0.7.4 parameters config_obj.update({"model_info": model_info}) + config_obj.pop("model", None) # NIM uses OpenAI-compatible API client = OpenAIChatCompletionClient(model=llm_config.model_name, **config_obj)
🧹 Nitpick comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (1)
74-75: Prefer unpacking over concatenation.Using list unpacking is more idiomatic and slightly more efficient than concatenation.
Apply this diff:
- new_messages = [system_message] + messages + new_messages = [system_message, *messages]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py(1 hunks)packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.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_autogen/src/nat/plugins/autogen/tool_wrapper.pypackages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
🧬 Code graph analysis (2)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (2)
src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/utils/type_utils.py (1)
DecomposedType(60-488)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (9)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/data_models/thinking_mixin.py (2)
ThinkingMixin(29-86)thinking_system_prompt(49-86)src/nat/llm/azure_openai_llm.py (2)
azure_openai_llm(55-57)AzureOpenAIModelConfig(30-51)src/nat/llm/nim_llm.py (1)
NIMModelConfig(34-52)src/nat/llm/openai_llm.py (2)
openai_llm(52-54)OpenAIModelConfig(31-48)src/nat/llm/utils/thinking.py (3)
BaseThinkingInjector(57-81)FunctionArgumentWrapper(34-53)patch_with_thinking(121-215)src/nat/utils/exception_handlers/automatic_retries.py (1)
patch_with_retry(269-342)src/nat/utils/type_utils.py (1)
override(56-57)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
148-148: Invalid assignment target
(invalid-syntax)
150-150: Unexpected indentation
(invalid-syntax)
153-153: unindent does not match any outer indentation level
(invalid-syntax)
156-156: unindent does not match any outer indentation level
(invalid-syntax)
158-158: Unexpected indentation
(invalid-syntax)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py
75-75: Consider [system_message, *messages] instead of concatenation
Replace with [system_message, *messages]
(RUF005)
🔇 Additional comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (1)
100-139: LGTM! Duplicatemodelargument issue correctly addressed.The implementation properly excludes
model_nameand pops the aliased"model"key (line 133) before passingmodel=llm_config.model_nameexplicitly to avoid duplicate keyword arguments.
| def resolve_type(t: Any) -> Any: | ||
| """Return the non-None member of a Union/PEP 604 union; | ||
| otherwise return the type unchanged. | ||
| Args: | ||
| t (Any): The type to resolve. | ||
| Returns: | ||
| Any: The resolved type. | ||
| """ | ||
| resolved = DecomposedType(t) | ||
| return resolved.type |
There was a problem hiding this comment.
resolve_type doesn't implement its documented behavior.
The function claims to "Return the non-None member of a Union/PEP 604 union" but just returns DecomposedType(t).type without actually resolving optional unions. For str | int | None, this returns the entire union unchanged instead of str | int.
Use DecomposedType.get_optional_type() to extract the non-None members:
def resolve_type(t: Any) -> Any:
"""Return the non-None member of a Union/PEP 604 union;
otherwise return the type unchanged.
Args:
t (Any): The type to resolve.
Returns:
Any: The resolved type.
"""
resolved = DecomposedType(t)
- return resolved.type
+ if resolved.is_optional:
+ return resolved.get_optional_type().type
+ return resolved.type📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def resolve_type(t: Any) -> Any: | |
| """Return the non-None member of a Union/PEP 604 union; | |
| otherwise return the type unchanged. | |
| Args: | |
| t (Any): The type to resolve. | |
| Returns: | |
| Any: The resolved type. | |
| """ | |
| resolved = DecomposedType(t) | |
| return resolved.type | |
| def resolve_type(t: Any) -> Any: | |
| """Return the non-None member of a Union/PEP 604 union; | |
| otherwise return the type unchanged. | |
| Args: | |
| t (Any): The type to resolve. | |
| Returns: | |
| Any: The resolved type. | |
| """ | |
| resolved = DecomposedType(t) | |
| if resolved.is_optional: | |
| return resolved.get_optional_type().type | |
| return resolved.type |
🤖 Prompt for AI Agents
In packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py around
lines 37 to 48, resolve_type currently returns DecomposedType(t).type but does
not strip None from Optional/PEP 604 unions; change it to call
DecomposedType(t).get_optional_type() and if that returns a non-None value
return it, otherwise fall back to resolved.type so Optional[...]/... | None is
replaced with the non-None member while other types remain unchanged.
There was a problem hiding this comment.
This should be appropriately addressed
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the test cases for resolve_type to understand expected behavior
echo "=== Test cases for resolve_type ==="
rg -nA5 "def test.*resolve_type" packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
echo -e "\n=== All resolve_type test assertions ==="
rg -nB2 -A2 "resolve_type" packages/nvidia_nat_autogen/tests/test_tool_wrapper.py
echo -e "\n=== DecomposedType implementation ==="
# Find and examine DecomposedType class
fd -t f "type_utils.py" --exec rg -nA30 "class DecomposedType"
echo -e "\n=== get_optional_type method ==="
fd -t f "type_utils.py" --exec rg -nA10 "def get_optional_type"
Length of output: 4986
🏁 Script executed:
#!/bin/bash
# Check for is_optional property in DecomposedType
echo "=== is_optional property in DecomposedType ==="
fd -t f "type_utils.py" --exec rg -nB2 -A10 "def is_optional|@property.*is_optional"
echo -e "\n=== Full get_optional_type implementation ==="
fd -t f "type_utils.py" --exec rg -nA20 "def get_optional_type"
echo -e "\n=== Usage of get_optional_type in codebase ==="
rg -n "get_optional_type\(\)" --type=py
echo -e "\n=== Current usage of resolve_type in tool_wrapper.py ==="
rg -nB3 -A3 "resolve_type\(" packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
Length of output: 2235
@willkill07, you're absolutely right. The resolve_type function currently doesn't match its documented behavior.
The fix is straightforward using DecomposedType's existing is_optional property and get_optional_type() method:
def resolve_type(t: Any) -> Any:
"""Return the non-None member of a Union/PEP 604 union;
otherwise return the type unchanged.
Args:
t (Any): The type to resolve.
Returns:
Any: The resolved type.
"""
resolved = DecomposedType(t)
if resolved.is_optional:
return resolved.get_optional_type().type
return resolved.typeThis pattern is already used elsewhere in the codebase (e.g., src/nat/observability/mixin/type_introspection_mixin.py:495 and src/nat/cli/commands/start.py:99).
Note: The tests in test_tool_wrapper.py will also need updating, as they currently assert that resolve_type(str | None) returns str | None unchanged, when it should now return str.
| if input_schema is not None: | ||
| func_to_wrap.__signature__ = inspect.Signature(parameters=params) | ||
| + func_to_wrap.__annotations__ = annotations |
There was a problem hiding this comment.
Fix critical syntax error.
Line 148 has an invalid + prefix that causes a syntax error. Additionally, lines 146-148 redundantly check if input_schema is not None: when already inside a block that checks the same condition (lines 133-145).
Apply this diff to fix the syntax error and remove redundant nesting:
if input_schema is not None:
annotations = {}
params = []
model_fields = getattr(input_schema, "model_fields", {})
for param_name, model_field in model_fields.items():
resolved_type = resolve_type(model_field.annotation)
default = model_field.default if model_field.is_required() is False else inspect._empty
params.append(
inspect.Parameter(param_name,
inspect.Parameter.POSITIONAL_OR_KEYWORD,
annotation=resolved_type,
default=default))
annotations[param_name] = resolved_type
- if input_schema is not None:
func_to_wrap.__signature__ = inspect.Signature(parameters=params)
-+ func_to_wrap.__annotations__ = annotations
+ func_to_wrap.__annotations__ = annotations
return func_to_wrap📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if input_schema is not None: | |
| func_to_wrap.__signature__ = inspect.Signature(parameters=params) | |
| + func_to_wrap.__annotations__ = annotations | |
| if input_schema is not None: | |
| annotations = {} | |
| params = [] | |
| model_fields = getattr(input_schema, "model_fields", {}) | |
| for param_name, model_field in model_fields.items(): | |
| resolved_type = resolve_type(model_field.annotation) | |
| default = model_field.default if model_field.is_required() is False else inspect._empty | |
| params.append( | |
| inspect.Parameter(param_name, | |
| inspect.Parameter.POSITIONAL_OR_KEYWORD, | |
| annotation=resolved_type, | |
| default=default)) | |
| annotations[param_name] = resolved_type | |
| func_to_wrap.__signature__ = inspect.Signature(parameters=params) | |
| func_to_wrap.__annotations__ = annotations | |
| return func_to_wrap |
🧰 Tools
🪛 Ruff (0.13.3)
148-148: Invalid assignment target
(invalid-syntax)
🤖 Prompt for AI Agents
In packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py around
lines 133 to 148, remove the redundant inner "if input_schema is not None:"
block and the invalid '+' prefix on line 148; instead, directly set
func_to_wrap.__signature__ = inspect.Signature(parameters=params) and
func_to_wrap.__annotations__ = annotations within the existing outer branch
where input_schema is already known non-None, ensuring no duplicated condition
and no stray '+' character that causes a syntax error.
Signed-off-by: Onkar Kulkarni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (1)
35-46:resolve_typedoesn't stripNonefrom optional unions as documented.The docstring claims to "Return the non-None member of a Union/PEP 604 union" but the implementation just returns
resolved.typeunchanged. Forstr | None, this returns the entire union instead ofstr.Apply this diff to implement the documented behavior using
DecomposedType's built-in method:def resolve_type(t: Any) -> Any: """Return the non-None member of a Union/PEP 604 union; otherwise return the type unchanged. Args: t (Any): The type to resolve. Returns: Any: The resolved type. """ resolved = DecomposedType(t) - return resolved.type + if resolved.is_optional: + return resolved.get_optional_type().type + return resolved.type
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.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).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Importable Python code inside packages must live under packages//src/
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.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:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.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_autogen/src/nat/plugins/autogen/tool_wrapper.py
🧠 Learnings (1)
📚 Learning: 2025-10-09T22:21:15.918Z
Learnt from: onkkul
PR: NVIDIA/NeMo-Agent-Toolkit#859
File: packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py:115-118
Timestamp: 2025-10-09T22:21:15.918Z
Learning: In `packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py`, when extracting parameters from input_schema at line 136, the code must handle both dataclasses (standard and Pydantic) and Pydantic BaseModels differently. Pydantic dataclasses have `__pydantic_fields__` (not `model_fields`), so use `dataclasses.fields()` for dataclasses and `model_fields` for BaseModels to properly extract function parameters.
Applied to files:
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
🧬 Code graph analysis (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (4)
src/nat/builder/builder.py (1)
Builder(68-290)src/nat/builder/framework_enum.py (1)
LLMFrameworkEnum(19-26)src/nat/utils/type_utils.py (1)
DecomposedType(60-488)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (2)
description(603-609)input_schema(612-616)
🪛 Ruff (0.13.3)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py
111-111: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/tool_wrapper.py (1)
156-166: LGTM: Streaming logic and FunctionTool creation are correct.The logic correctly selects between streaming and non-streaming wrappers based on
fn.has_streaming_outputandfn.has_single_output, and theFunctionToolis created with appropriate parameters.
| # If input_schema is a dataclass, convert it to a Pydantic model | ||
| if input_schema is not None and is_dataclass(input_schema): | ||
| input_schema = pydantic_dataclass(input_schema) | ||
|
|
||
| def decorator(func_to_wrap: Callable[..., Any]) -> Callable[..., Any]: | ||
| """ | ||
| Decorator to set metadata on the function. | ||
| """ | ||
| # Set the function's metadata | ||
| if name is not None: | ||
| func_to_wrap.__name__ = name | ||
| if description is not None: | ||
| func_to_wrap.__doc__ = description | ||
|
|
||
| # Set signature only if input_schema is provided | ||
| params: list[inspect.Parameter] = [] | ||
| annotations: dict[str, Any] = {} | ||
|
|
||
| if input_schema is not None: | ||
| annotations = {} | ||
| params = [] | ||
| model_fields = getattr(input_schema, "model_fields", {}) | ||
| for param_name, model_field in model_fields.items(): | ||
| resolved_type = resolve_type(model_field.annotation) | ||
| default = model_field.default if model_field.is_required() is False else inspect._empty | ||
| params.append( | ||
| inspect.Parameter(param_name, | ||
| inspect.Parameter.POSITIONAL_OR_KEYWORD, | ||
| annotation=resolved_type, | ||
| default=default)) | ||
| annotations[param_name] = resolved_type | ||
| if input_schema is not None: | ||
| func_to_wrap.__signature__ = inspect.Signature(parameters=params) | ||
| func_to_wrap.__annotations__ = annotations |
There was a problem hiding this comment.
Critical: Pydantic dataclasses don't have model_fields, breaking signature extraction.
After converting a dataclass to a Pydantic dataclass at line 115, the code at line 134 attempts to access model_fields, which doesn't exist on Pydantic dataclasses—they have __pydantic_fields__ instead. This returns an empty dict, so the loop at lines 135-143 never executes, leaving params and annotations empty. AutoGen then receives a tool with no parameters, breaking any NAT function that uses a dataclass schema.
Based on learnings.
Apply this diff to handle both Pydantic dataclasses and BaseModels:
+ from dataclasses import fields as dc_fields
+
if input_schema is not None:
annotations = {}
params = []
- model_fields = getattr(input_schema, "model_fields", {})
- for param_name, model_field in model_fields.items():
- resolved_type = resolve_type(model_field.annotation)
- default = model_field.default if model_field.is_required() is False else inspect._empty
+
+ # Handle both Pydantic dataclasses and BaseModels
+ if hasattr(input_schema, "__pydantic_fields__"):
+ # Pydantic dataclass
+ for field in dc_fields(input_schema):
+ resolved_type = resolve_type(field.type)
+ default = field.default if field.default is not inspect._empty else inspect._empty
+ params.append(
+ inspect.Parameter(field.name,
+ inspect.Parameter.POSITIONAL_OR_KEYWORD,
+ annotation=resolved_type,
+ default=default))
+ annotations[field.name] = resolved_type
+ elif hasattr(input_schema, "model_fields"):
+ # Pydantic BaseModel
+ for param_name, model_field in input_schema.model_fields.items():
+ resolved_type = resolve_type(model_field.annotation)
+ default = model_field.default if model_field.is_required() is False else inspect._empty
+ params.append(
+ inspect.Parameter(param_name,
+ inspect.Parameter.POSITIONAL_OR_KEYWORD,
+ annotation=resolved_type,
+ default=default))
+ annotations[param_name] = resolved_type
+ else:
+ logger.warning("input_schema has neither __pydantic_fields__ nor model_fields: %s",
+ type(input_schema))
- params.append(
- inspect.Parameter(param_name,
- inspect.Parameter.POSITIONAL_OR_KEYWORD,
- annotation=resolved_type,
- default=default))
- annotations[param_name] = resolved_type
if input_schema is not None:
func_to_wrap.__signature__ = inspect.Signature(parameters=params)
func_to_wrap.__annotations__ = annotations📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # If input_schema is a dataclass, convert it to a Pydantic model | |
| if input_schema is not None and is_dataclass(input_schema): | |
| input_schema = pydantic_dataclass(input_schema) | |
| def decorator(func_to_wrap: Callable[..., Any]) -> Callable[..., Any]: | |
| """ | |
| Decorator to set metadata on the function. | |
| """ | |
| # Set the function's metadata | |
| if name is not None: | |
| func_to_wrap.__name__ = name | |
| if description is not None: | |
| func_to_wrap.__doc__ = description | |
| # Set signature only if input_schema is provided | |
| params: list[inspect.Parameter] = [] | |
| annotations: dict[str, Any] = {} | |
| if input_schema is not None: | |
| annotations = {} | |
| params = [] | |
| model_fields = getattr(input_schema, "model_fields", {}) | |
| for param_name, model_field in model_fields.items(): | |
| resolved_type = resolve_type(model_field.annotation) | |
| default = model_field.default if model_field.is_required() is False else inspect._empty | |
| params.append( | |
| inspect.Parameter(param_name, | |
| inspect.Parameter.POSITIONAL_OR_KEYWORD, | |
| annotation=resolved_type, | |
| default=default)) | |
| annotations[param_name] = resolved_type | |
| if input_schema is not None: | |
| func_to_wrap.__signature__ = inspect.Signature(parameters=params) | |
| func_to_wrap.__annotations__ = annotations | |
| # If input_schema is a dataclass, convert it to a Pydantic model | |
| if input_schema is not None and is_dataclass(input_schema): | |
| input_schema = pydantic_dataclass(input_schema) | |
| def decorator(func_to_wrap: Callable[..., Any]) -> Callable[..., Any]: | |
| """ | |
| Decorator to set metadata on the function. | |
| """ | |
| # Set the function's metadata | |
| if name is not None: | |
| func_to_wrap.__name__ = name | |
| if description is not None: | |
| func_to_wrap.__doc__ = description | |
| # Set signature only if input_schema is provided | |
| from dataclasses import fields as dc_fields | |
| params: list[inspect.Parameter] = [] | |
| annotations: dict[str, Any] = {} | |
| if input_schema is not None: | |
| annotations = {} | |
| params = [] | |
| # Handle both Pydantic dataclasses and BaseModels | |
| if hasattr(input_schema, "__pydantic_fields__"): | |
| # Pydantic dataclass | |
| for field in dc_fields(input_schema): | |
| resolved_type = resolve_type(field.type) | |
| default = field.default if field.default is not inspect._empty else inspect._empty | |
| params.append( | |
| inspect.Parameter( | |
| field.name, | |
| inspect.Parameter.POSITIONAL_OR_KEYWORD, | |
| annotation=resolved_type, | |
| default=default, | |
| ) | |
| ) | |
| annotations[field.name] = resolved_type | |
| elif hasattr(input_schema, "model_fields"): | |
| # Pydantic BaseModel | |
| for param_name, model_field in input_schema.model_fields.items(): | |
| resolved_type = resolve_type(model_field.annotation) | |
| default = ( | |
| model_field.default | |
| if model_field.is_required() is False | |
| else inspect._empty | |
| ) | |
| params.append( | |
| inspect.Parameter( | |
| param_name, | |
| inspect.Parameter.POSITIONAL_OR_KEYWORD, | |
| annotation=resolved_type, | |
| default=default, | |
| ) | |
| ) | |
| annotations[param_name] = resolved_type | |
| else: | |
| logger.warning( | |
| "input_schema has neither __pydantic_fields__ nor model_fields: %s", | |
| type(input_schema), | |
| ) | |
| if input_schema is not None: | |
| func_to_wrap.__signature__ = inspect.Signature(parameters=params) | |
| func_to_wrap.__annotations__ = annotations |
| #### Set up the MCP server | ||
| This example demonstrates how NAT can interact with MCP servers on behalf of AutoGen agents. | ||
|
|
||
| First run the MCP server with this command: | ||
|
|
||
| ```bash | ||
| nat mcp serve --config_file examples/getting_started/simple_calculator/configs/config.yml --tool_names current_datetime |
There was a problem hiding this comment.
I've been thinking about this more.
I (personally) do not see any advantage for using MCP internals of frameworks when NAT can already abstract this away by being able to treat MCP functions like ordinary tools.
By preferring what exists in NAT, it provides less of an API surface for integration while providing all of the benefits.
The only change here would be in the configuration file. See here:
| def resolve_type(t: Any) -> Any: | ||
| """Return the non-None member of a Union/PEP 604 union; | ||
| otherwise return the type unchanged. | ||
| Args: | ||
| t (Any): The type to resolve. | ||
| Returns: | ||
| Any: The resolved type. | ||
| """ | ||
| resolved = DecomposedType(t) | ||
| return resolved.type |
There was a problem hiding this comment.
This should be appropriately addressed
| def test_imports_exist(self): | ||
| """Test that required imports exist and work.""" | ||
| try: | ||
| from nat.plugins.autogen import llm # pylint: disable=import-outside-toplevel | ||
| from nat.plugins.autogen import tool_wrapper # pylint: disable=import-outside-toplevel | ||
| assert llm is not None | ||
| assert tool_wrapper is not None | ||
| except ImportError as e: | ||
| pytest.fail(f"Failed to import required modules: {e}") | ||
|
|
||
| def test_register_module_importable(self): | ||
| """Test that the register module can be imported.""" | ||
| try: | ||
| from nat.plugins.autogen import register # pylint: disable=import-outside-toplevel | ||
| assert register is not None | ||
| except ImportError as e: | ||
| pytest.fail(f"Failed to import register module: {e}") |
There was a problem hiding this comment.
I don't think these are necessary. The nat cli would not run if register didn't exist. And the llm and tool_wrapper wouldn't be accessible via the type introspection when parsing a valid configuration file.
| if __name__ == "__main__": | ||
| pytest.main([__file__, "-v"]) |
There was a problem hiding this comment.
| if __name__ == "__main__": | |
| pytest.main([__file__, "-v"]) |
| if input_schema is not None: | ||
| # This block should not execute | ||
| pytest.fail("Should not process None input_schema") | ||
|
|
There was a problem hiding this comment.
| if input_schema is not None: | |
| # This block should not execute | |
| pytest.fail("Should not process None input_schema") |
| def test_union_type_detection(self): | ||
| """Test union type detection.""" | ||
| union_type = str | int | None | ||
| origin = types.UnionType if hasattr(types, 'UnionType') else type(union_type) | ||
|
|
||
| # Test that we can detect union types | ||
| assert origin is not None |
There was a problem hiding this comment.
This is a broken test that will always pass.
| if __name__ == "__main__": | ||
| pytest.main([__file__, "-v"]) |
There was a problem hiding this comment.
| if __name__ == "__main__": | |
| pytest.main([__file__, "-v"]) |
| -------------------------------------------------- | ||
| Workflow Result: | ||
| ['New York weather: Sunny, around 25°C (77°F).\nCurrent local time in New York: 5:34 PM EDT (UTC−4) on October 7, 2025.\n\nAPPROVE'] | ||
| -------------------------------------------------- | ||
|
|
There was a problem hiding this comment.
Duplicated
| -------------------------------------------------- | |
| Workflow Result: | |
| ['New York weather: Sunny, around 25°C (77°F).\nCurrent local time in New York: 5:34 PM EDT (UTC−4) on October 7, 2025.\n\nAPPROVE'] | |
| -------------------------------------------------- |
| front_end: | ||
| _type: console | ||
| cors: | ||
| allow_origins: ["*"] |
There was a problem hiding this comment.
console front-end probably doesn't need to be concerned with cors
| front_end: | |
| _type: console | |
| cors: | |
| allow_origins: ["*"] |
There was a problem hiding this comment.
Just letting you know that we (will) have additional things listed in the pyproject file. If you want to see ahead of time, look at what's on the release/1.3 branch (it will all be forward merged into develop)
|
/ok to test 73079e4 |
|
CI isn't passing because the environment cannot be built. Can you please run CI locally and attempt to fix? |
4 participants
[Synopsys] OpenSource Contribution
Description
Microsoft AutoGen Integration
Closes: #419, #122
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores