Add test LLM provider to support testing #764
Conversation
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
WalkthroughAdds a deterministic test LLM provider "nat_test_llm" with multi-framework client adapters and registration, new documentation/tutorial pages, extensive async tests covering YAML and builder flows, a small builder metadata iteration change, and a minor comment typo fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User / CLI
participant Loader as Workflow Loader
participant Registry as LLM Provider Registry
participant TestLLM as nat_test_llm
participant WF as Workflow
U->>Loader: nat run --config_file config.yml --input "..."
Loader->>Registry: Resolve provider for `_type=nat_test_llm`
Registry-->>Loader: Provider + client builder
Loader->>TestLLM: Initialize (response_seq, delay_ms)
U->>WF: Execute chat_completion
WF->>TestLLM: Request completion
TestLLM-->>WF: Next response (cycled) after optional delay
WF-->>U: Output (e.g., "alpha")
sequenceDiagram
autonumber
participant Dev as Developer Code
participant Builder as WorkflowBuilder
participant Registry as LLM Provider Registry
participant TestClient as Framework-specific Test LLM
Dev->>Builder: configure LLM (`_type=nat_test_llm`, `response_seq`)
Builder->>Registry: Resolve framework client (e.g., LangChain)
Registry-->>Builder: Framework-specific test client
Dev->>TestClient: invoke()/ainvoke()/stream
TestClient-->>Dev: Deterministic next response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed Checks (1 warning)
✅ Passed Checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zhongxuanwang-nv
changed the title
zhongxuanwang-nv
added
improvement
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
docs/source/tutorials/testing-with-nat-test-llm.md (2)
24-29: Prerequisites should include installing the core toolkit, not just the test plugin.Readers may hit “command not found: nat” if nvidia-nat isn’t installed.
Apply:
-## Prerequisites - -- Install the testing plugin package: +## Prerequisites + +- Install the core toolkit and testing plugin: ```bash -uv pip install nvidia-nat-test +uv pip install nvidia-nat nvidia-nat-test--- `58-66`: **Add runnable entrypoint to the async example.** Make the snippet copy-paste runnable. ```diff from nat.runtime.loader import load_workflow async def main(): async with load_workflow("config.yml") as workflow: async with workflow.run("What is 1 + 2?") as runner: result = await runner.result() print(result) + +if __name__ == "__main__": + import asyncio + asyncio.run(main())docs/source/workflows/llms/index.md (1)
131-134: Conform unordered list style to project (MD004).Switch dashes to asterisks.
-- Installation: `uv pip install nvidia-nat-test` -- Purpose: Deterministic cycling responses for quick validation -- Not for production +* Installation: `uv pip install nvidia-nat-test` +* Purpose: Deterministic cycling responses for quick validation +* Not for productionpackages/nvidia_nat_test/tests/test_test_llm.py (3)
61-63: Consider importing AIMessage at the module level.The
AIMessageimport is repeated across multiple test functions. Consider moving it to the module-level imports for better consistency and reduced redundancy.Move the import to the top of the file:
+from langchain_core.messages import AIMessage from nat.builder.framework_enum import LLMFrameworkEnum from nat.builder.workflow_builder import WorkflowBuilder from nat.runtime.loader import load_workflow from nat.test.llm import TestLLMConfigThen remove the local imports from lines 61, 97, 129, 173, 215, 263, 302, and 340.
92-94: Improve file naming for clarity.The conditional expression for generating the filename could be clearer using a dedicated variable.
- config_file = tmp_path / ("chat_completion_cycle_workflow_first.yml" - if workflow_first else "chat_completion_cycle_llms_first.yml") + filename = "chat_completion_cycle_workflow_first.yml" if workflow_first else "chat_completion_cycle_llms_first.yml" + config_file = tmp_path / filename
240-247: Consider using yaml.dump for safer YAML generation.While the manual quoting logic works, using a proper YAML library would be more robust for handling special characters and edge cases.
Consider using PyYAML:
import yaml seq_yaml = yaml.dump(seq, default_flow_style=True).strip()This would handle all special characters and quoting automatically.
packages/nvidia_nat_test/src/nat/test/llm.py (2)
69-71: Remove unnecessary del statement.The
del builderstatement is unnecessary in this context and could be handled with a more Pythonic approach.@register_llm_provider(config_type=TestLLMConfig) -async def test_llm_provider(config: TestLLMConfig, builder: Builder): - del builder # suppress linting error +async def test_llm_provider(config: TestLLMConfig, builder: Builder): # pylint: disable=unused-argument yield LLMProviderInfo(config=config, description="Test LLM provider")This same pattern applies to lines 79, 109, 149, 167, and 189.
125-127: Add type hints for better code clarity.The LlamaIndex test implementation would benefit from type hints on method parameters and return types.
- def chat(self, messages: list[Any] | None = None, **_kwargs: Any) -> LIChatResponse: + def chat(self, messages: list[Any] | None = None, **_kwargs: Any) -> "LIChatResponse": chooser.sync_sleep() return LIChatResponse(chooser.next_response())Apply similar changes to other methods in this class and other test LLM classes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
docs/source/tutorials/testing-with-nat-test-llm.md(1 hunks)docs/source/workflows/llms/index.md(1 hunks)packages/nvidia_nat_test/src/nat/test/llm.py(1 hunks)packages/nvidia_nat_test/src/nat/test/register.py(1 hunks)packages/nvidia_nat_test/tests/test_test_llm.py(1 hunks)src/nat/builder/component_utils.py(1 hunks)src/nat/data_models/common.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*
⚙️ 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:
docs/source/tutorials/testing-with-nat-test-llm.mdsrc/nat/builder/component_utils.pysrc/nat/data_models/common.pypackages/nvidia_nat_test/src/nat/test/register.pypackages/nvidia_nat_test/tests/test_test_llm.pydocs/source/workflows/llms/index.mdpackages/nvidia_nat_test/src/nat/test/llm.py
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/tutorials/testing-with-nat-test-llm.mddocs/source/workflows/llms/index.md
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/builder/component_utils.pysrc/nat/data_models/common.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/register.pypackages/nvidia_nat_test/tests/test_test_llm.pypackages/nvidia_nat_test/src/nat/test/llm.py
🪛 LanguageTool
docs/source/tutorials/testing-with-nat-test-llm.md
[grammar] ~71-~71: There might be a mistake here.
Context: ...r](../extend/adding-an-llm-provider.md). - For more about configuring LLMs, see: [L...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...see: LLMs.
(QB_NEW_EN)
docs/source/workflows/llms/index.md
[grammar] ~132-~132: There might be a mistake here.
Context: ...c cycling responses for quick validation - Not for production Minimal YAML example...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/source/workflows/llms/index.md
131-131: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
132-132: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
133-133: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
149-149: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
150-150: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 GitHub Actions: Build pull request
docs/source/workflows/llms/index.md
[error] 150-150: Vale.Spelling: Did you really mean 'nat_test_llm'?
[error] 129-129: Path check failed: invalid/broken path 'development/testing' referenced in docs/source/workflows/llms/index.md at line 129:21.
🔇 Additional comments (10)
src/nat/data_models/common.py (1)
163-169: Minor typo fix in comment — looks good.No functional change; safe to merge.
src/nat/builder/component_utils.py (1)
177-192: Iterating class-level model_fields is correct for Pydantic v2; verify no regressions on optional/defaulted fields.Using type(instance_config).model_fields avoids any instance shadowing and is consistent with how Pydantic exposes field metadata. Please run/extend tests to cover:
- Fields with default None and ComponentRef nested inside unions/containers.
- Discriminated unions where annotations live on the class.
packages/nvidia_nat_test/src/nat/test/register.py (1)
24-24: LGTM — ensures provider side-effect registration.Importing llm here allows discovery without explicit imports.
packages/nvidia_nat_test/tests/test_test_llm.py (3)
1-14: LGTM!The Apache license header and copyright year are correctly implemented as required.
32-40: LGTM! Well-structured parametrized test.The test effectively covers edge cases (empty sequence) and normal cases for single response retrieval.
329-330: LGTM! Good defensive check for optional dependency.The check for
semantic_kernelavailability before running tests is appropriate.packages/nvidia_nat_test/src/nat/test/llm.py (4)
1-14: LGTM!The Apache license header and copyright year are correctly implemented.
35-43: LGTM! Well-structured configuration class.The
TestLLMConfigclass is properly defined with appropriate field validation and clear documentation. The__test__ = Falseattribute correctly prevents pytest from collecting this class.
74-103: LGTM! Well-implemented LangChain test client.The implementation correctly provides both sync and async methods with appropriate delay simulation.
186-210: LGTM! Complete Agno client implementation.The Agno test client properly implements all required methods with both sync and async variants.
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
Signed-off-by: Daniel Wang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/source/workflows/llms/index.md (1)
127-134: Good fix: replaced “development/testing” with “development and testing.”This resolves the CI path-check and aligns with earlier feedback.
🧹 Nitpick comments (3)
docs/source/workflows/llms/index.md (3)
22-22: Grammar tweak for headline sentence.Prepend “The” for smoother flow; keeps the official first-use name intact.
-NVIDIA NeMo Agent toolkit supports the following LLM providers: +The NVIDIA NeMo Agent toolkit supports the following LLM providers:
131-134: List punctuation consistency.End list items with periods to satisfy grammar/style lint consistency.
-* Installation: `uv pip install nvidia-nat-test` -* Purpose: Deterministic cycling responses for quick validation -* Not for production +* Installation: `uv pip install nvidia-nat-test`. +* Purpose: Deterministic cycling responses for quick validation. +* Not for production.
149-153: Avoid disabling Vale; format the provider name instead.Remove the Vale off/on markers and wrap the provider name in the link text. This adheres to the vocab rules without suppressing linting.
-* Learn how to add your own LLM provider: [Adding an LLM Provider](../../extend/adding-an-llm-provider.md) -<!-- vale off --> -* See a short tutorial using YAML and `nat_test_llm`: [Testing with nat_test_llm](../../tutorials/testing-with-nat-test-llm.md) -<!-- vale on --> +* Learn how to add your own LLM provider: [Adding an LLM Provider](../../extend/adding-an-llm-provider.md) +* See a short tutorial using YAML and `nat_test_llm`: [Testing with `nat_test_llm`](../../tutorials/testing-with-nat-test-llm.md)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/source/tutorials/index.md(1 hunks)docs/source/workflows/llms/index.md(2 hunks)packages/nvidia_nat_test/src/nat/test/llm.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/tutorials/index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nvidia_nat_test/src/nat/test/llm.py
🧰 Additional context used
📓 Path-based instructions (5)
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Use the official naming: first use “NVIDIA NeMo Agent toolkit”; subsequent uses “NeMo Agent toolkit”; never use deprecated names in documentation
Documentation sources must be Markdown under docs/source; keep docs in sync and fix Sphinx errors/broken links
Documentation must be clear, comprehensive, free of TODO/FIXME/placeholders/offensive/outdated terms; fix spelling; adhere to Vale vocab allow/reject lists
Files:
docs/source/workflows/llms/index.md
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
docs/source/workflows/llms/index.md
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
docs/source/workflows/llms/index.md
**/*
⚙️ 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:
docs/source/workflows/llms/index.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/llms/index.md
🪛 LanguageTool
docs/source/workflows/llms/index.md
[grammar] ~22-~22: There might be a mistake here.
Context: ...it supports the following LLM providers: | Provider | Type | Description | |-----...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ...c cycling responses for quick validation * Not for production Minimal YAML example...
(QB_NEW_EN)
Signed-off-by: Daniel Wang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nvidia_nat_test/src/nat/test/llm.py (1)
51-60: Empty-sequence handling is fixed properly.
Setting_cyclertoNonefor empty input and short-circuiting innext_response()resolves the earlier StopIteration issue.
🧹 Nitpick comments (9)
packages/nvidia_nat_test/src/nat/test/llm.py (9)
35-43: Preferdefault_factoryfor mutable defaults.
Avoid potential shared-state surprises and keep ruff happy.- response_seq: list[str] = Field( - default=[], - description=("returns the next element in order (wraps)"), - ) + response_seq: list[str] = Field( + default_factory=list, + description="Returns the next element in order (wraps).", + )
45-60: Concurrency note: cycler isn’t thread-/task-safe if a single instance is shared.
If callers share one client instance across concurrent calls,next()may interleave. Either document single-consumer usage or guardnext_response()with a lock (threading/async) per your concurrency model.
68-72: Add a docstring for the public provider registration.
Meets the repo’s public-API docstring guideline.async def test_llm_provider(config: TestLLMConfig, builder: Builder): + """Register the `nat_test_llm` provider for the NAT registry.""" del builder # suppress linting error yield LLMProviderInfo(config=config, description="Test LLM provider")
74-101: Silence ruff/pylint unused-argument by prefixing with underscores.
No behavior change; reduces global disables.- def invoke(self, messages: Any, **_kwargs: Any) -> AIMessage: + def invoke(self, _messages: Any, **_kwargs: Any) -> AIMessage: ... - async def ainvoke(self, messages: Any, **_kwargs: Any) -> AIMessage: + async def ainvoke(self, _messages: Any, **_kwargs: Any) -> AIMessage: ... - def stream(self, messages: Any, **_kwargs: Any) -> Iterator[AIMessage]: + def stream(self, _messages: Any, **_kwargs: Any) -> Iterator[AIMessage]: ... - async def astream(self, messages: Any, **_kwargs: Any) -> AsyncGenerator[AIMessage, None]: + async def astream(self, _messages: Any, **_kwargs: Any) -> AsyncGenerator[AIMessage, None]:
120-140: Apply the same unused-argument cleanup for LlamaIndex adapter.- def chat(self, messages: list[Any] | None = None, **_kwargs: Any) -> LIChatResponse: + def chat(self, _messages: list[Any] | None = None, **_kwargs: Any) -> LIChatResponse: ... - async def achat(self, messages: list[Any] | None = None, **_kwargs: Any) -> LIChatResponse: + async def achat(self, _messages: list[Any] | None = None, **_kwargs: Any) -> LIChatResponse: ... - def stream_chat(self, messages: list[Any] | None = None, **_kwargs: Any) -> Iterator[LIChatResponse]: + def stream_chat(self, _messages: list[Any] | None = None, **_kwargs: Any) -> Iterator[LIChatResponse]: ... - async def astream_chat(self, - messages: list[Any] | None = None, + async def astream_chat(self, + _messages: list[Any] | None = None, **_kwargs: Any) -> AsyncGenerator[LIChatResponse, None]:
149-155: CrewAI: underscore unused parameters.- def call(self, messages: list[dict[str, str]] | None = None, **kwargs: Any) -> str: + def call(self, _messages: list[dict[str, str]] | None = None, **_kwargs: Any) -> str:
167-179: SemanticKernel: underscore unused parameters.- async def get_chat_message_contents(self, chat_history: Any, **_kwargs: Any) -> list[ChatMessageContent]: + async def get_chat_message_contents(self, _chat_history: Any, **_kwargs: Any) -> list[ChatMessageContent]: ... - async def get_streaming_chat_message_contents(self, chat_history: Any, + async def get_streaming_chat_message_contents(self, _chat_history: Any, **_kwargs: Any) -> AsyncGenerator[ChatMessageContent, None]:
189-207: Agno: underscore unused parameters.- def invoke(self, messages: Any | None = None, **_kwargs: Any) -> str: + def invoke(self, _messages: Any | None = None, **_kwargs: Any) -> str: ... - async def ainvoke(self, messages: Any | None = None, **_kwargs: Any) -> str: + async def ainvoke(self, _messages: Any | None = None, **_kwargs: Any) -> str: ... - def invoke_stream(self, messages: Any | None = None, **_kwargs: Any) -> Iterator[str]: + def invoke_stream(self, _messages: Any | None = None, **_kwargs: Any) -> Iterator[str]: ... - async def ainvoke_stream(self, messages: Any | None = None, **_kwargs: Any) -> AsyncGenerator[str, None]: + async def ainvoke_stream(self, _messages: Any | None = None, **_kwargs: Any) -> AsyncGenerator[str, None]:
76-76: Docstring punctuation and casing nit.
End one-line docstrings with a period; capitalize “Agno”.- """LLM client for LangChain""" + """LLM client for LangChain.""" ... - """LLM client for LlamaIndex""" + """LLM client for LlamaIndex.""" ... - """LLM client for CrewAI""" + """LLM client for CrewAI.""" ... - """LLM client for SemanticKernel""" + """LLM client for SemanticKernel.""" ... - """LLM client for agno""" + """LLM client for Agno."""Also applies to: 105-105, 145-145, 160-160, 185-185
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/nvidia_nat_test/src/nat/test/llm.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_test/src/nat/test/llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
Files:
packages/nvidia_nat_test/src/nat/test/llm.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
packages/nvidia_nat_test/src/nat/test/llm.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
packages/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_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_test/src/nat/test/llm.py
🧬 Code graph analysis (1)
packages/nvidia_nat_test/src/nat/test/llm.py (1)
src/nat/cli/type_registry.py (2)
register_llm_client(523-533)register_llm_provider(481-489)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (1)
packages/nvidia_nat_test/src/nat/test/llm.py (1)
1-15: SPDX header and licensing block look correct.
Years and headers are up to date.
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
This is pretty cool. Just some minor feedback but otherwise LGTM.
Signed-off-by: Daniel Wang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/nvidia_nat_test/tests/test_test_llm.py (4)
16-16: Allow pytest-style asserts (silence Ruff S101 in tests).Ruff flags S101 on every assert; pytest relies on asserts. Add a file-level ignore.
# pylint: disable=import-outside-toplevel,redefined-outer-name +# ruff: noqa: S101 # allow asserts in tests; pytest uses them idiomatically
29-36: Name fixtures with fixture_ prefix per guidelines.Renames keep behavior the same (autouse still applies) and appease style rules.
-@pytest.fixture(autouse=True, scope="module") -def _register_test_llm(): +@pytest.fixture(autouse=True, scope="module", name="fixture_register_test_llm") +def fixture_register_test_llm(): @@ -@pytest.fixture(scope="module") -def test_llm_config_cls(): +@pytest.fixture(scope="module", name="fixture_test_llm_config_cls") +def fixture_test_llm_config_cls(): @@ -async def test_builder_framework_cycle(wrapper: str, seq: list[str], test_llm_config_cls): +async def test_builder_framework_cycle(wrapper: str, seq: list[str], fixture_test_llm_config_cls): @@ - cfg = test_llm_config_cls(response_seq=list(seq), delay_ms=0) + cfg = fixture_test_llm_config_cls(response_seq=list(seq), delay_ms=0)Also applies to: 38-43, 330-341
41-43: Prefer direct attribute access over getattr for constant attribute names (Ruff B009).- mod = importlib.import_module("nat.test.llm") - return getattr(mod, "TestLLMConfig") + mod = importlib.import_module("nat.test.llm") + return mod.TestLLMConfig
356-365: Avoid nested getattr; use explicit attribute access with try/except.This removes Ruff B009 and fails loudly on unexpected shapes.
- elif wrapper == LLMFrameworkEnum.LLAMA_INDEX.value: - for _ in range(len(seq)): - r = await client.achat([]) - # Prefer message.content if available; fallback to .text - content = getattr(getattr(r, "message", None), "content", None) - if content is None: - content = getattr(r, "text", None) - assert isinstance(content, str), f"Unexpected LlamaIndex response: {r}" - outs.append(content) + elif wrapper == LLMFrameworkEnum.LLAMA_INDEX.value: + for _ in range(len(seq)): + r = await client.achat([]) + # Prefer r.message.content, fallback to r.text + try: + content = r.message.content # type: ignore[attr-defined] + except AttributeError: + try: + content = r.text # type: ignore[attr-defined] + except AttributeError: + content = None + assert isinstance(content, str), f"Unexpected LlamaIndex response: {r}" + outs.append(content)packages/nvidia_nat_test/src/nat/test/llm.py (3)
70-70: Prefix unusedbuilderargs with underscore to satisfy Ruff (ARG001).-async def test_llm_provider(config: TestLLMConfig, builder: Builder): +async def test_llm_provider(config: TestLLMConfig, _builder: Builder): @@ -async def test_llm_langchain(config: TestLLMConfig, builder: Builder): +async def test_llm_langchain(config: TestLLMConfig, _builder: Builder): @@ -async def test_llm_llama_index(config: TestLLMConfig, builder: Builder): +async def test_llm_llama_index(config: TestLLMConfig, _builder: Builder): @@ -async def test_llm_crewai(config: TestLLMConfig, builder: Builder): +async def test_llm_crewai(config: TestLLMConfig, _builder: Builder): @@ -async def test_llm_semantic_kernel(config: TestLLMConfig, builder: Builder): +async def test_llm_semantic_kernel(config: TestLLMConfig, _builder: Builder): @@ -async def test_llm_agno(config: TestLLMConfig, builder: Builder): +async def test_llm_agno(config: TestLLMConfig, _builder: Builder):Also applies to: 76-76, 103-103, 138-138, 153-153, 182-182
83-99: Prefix unused method parameters with underscore (Ruff ARG002).Keeps signatures stable and clears lints.
class LangChainTestLLM: - def invoke(self, messages: Any, **_kwargs: Any) -> str: + def invoke(self, _messages: Any, **_kwargs: Any) -> str: @@ - async def ainvoke(self, messages: Any, **_kwargs: Any) -> str: + async def ainvoke(self, _messages: Any, **_kwargs: Any) -> str: @@ - def stream(self, messages: Any, **_kwargs: Any) -> Iterator[str]: + def stream(self, _messages: Any, **_kwargs: Any) -> Iterator[str]: @@ - async def astream(self, messages: Any, **_kwargs: Any) -> AsyncGenerator[str]: + async def astream(self, _messages: Any, **_kwargs: Any) -> AsyncGenerator[str]: @@ - def chat(self, messages: list[Any] | None = None, **_kwargs: Any) -> ChatResponse: + def chat(self, _messages: list[Any] | None = None, **_kwargs: Any) -> ChatResponse: @@ - async def achat(self, messages: list[Any] | None = None, **_kwargs: Any) -> ChatResponse: + async def achat(self, _messages: list[Any] | None = None, **_kwargs: Any) -> ChatResponse: @@ - def stream_chat(self, messages: list[Any] | None = None, **_kwargs: Any) -> Iterator[ChatResponse]: + def stream_chat(self, _messages: list[Any] | None = None, **_kwargs: Any) -> Iterator[ChatResponse]: @@ - async def astream_chat(self, - messages: list[Any] | None = None, + async def astream_chat(self, + _messages: list[Any] | None = None, **_kwargs: Any) -> AsyncGenerator[ChatResponse, None]: @@ - def call(self, messages: list[dict[str, str]] | None = None, **kwargs: Any) -> str: + def call(self, _messages: list[dict[str, str]] | None = None, **_kwargs: Any) -> str: @@ - async def get_chat_message_contents(self, chat_history: Any, **_kwargs: Any) -> list[ChatMessageContent]: + async def get_chat_message_contents(self, _chat_history: Any, **_kwargs: Any) -> list[ChatMessageContent]: @@ - async def get_streaming_chat_message_contents(self, chat_history: Any, + async def get_streaming_chat_message_contents(self, _chat_history: Any, **_kwargs: Any) -> AsyncGenerator[ChatMessageContent, None]: @@ - def invoke(self, messages: Any | None = None, **_kwargs: Any) -> str: + def invoke(self, _messages: Any | None = None, **_kwargs: Any) -> str: @@ - async def ainvoke(self, messages: Any | None = None, **_kwargs: Any) -> str: + async def ainvoke(self, _messages: Any | None = None, **_kwargs: Any) -> str: @@ - def invoke_stream(self, messages: Any | None = None, **_kwargs: Any) -> Iterator[str]: + def invoke_stream(self, _messages: Any | None = None, **_kwargs: Any) -> Iterator[str]: @@ - async def ainvoke_stream(self, messages: Any | None = None, **_kwargs: Any) -> AsyncGenerator[str, None]: + async def ainvoke_stream(self, _messages: Any | None = None, **_kwargs: Any) -> AsyncGenerator[str, None]:Also applies to: 116-133, 145-149, 167-177, 189-205
109-111: Minor: trim long ImportError messages (TRY003).Keep the message concise; installation hints can go in docs.
- except ImportError as exc: - raise ImportError("llama_index is required for using the test_llm with llama_index. " - "Please install the `nvidia-nat-llama-index` package. ") from exc + except ImportError as exc: + raise ImportError("Install `nvidia-nat-llama-index` to use the llama_index test client.") from exc @@ - except ImportError as exc: - raise ImportError("Semantic Kernel is required for using the test_llm with semantic_kernel. " - "Please install the `nvidia-nat-semantic-kernel` package. ") from exc + except ImportError as exc: + raise ImportError("Install `nvidia-nat-semantic-kernel` to use the Semantic Kernel test client.") from excAlso applies to: 160-161
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/workflows/llms/index.md(2 hunks)packages/nvidia_nat_test/src/nat/test/llm.py(1 hunks)packages/nvidia_nat_test/tests/test_test_llm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/source/workflows/llms/index.md
🧰 Additional context used
📓 Path-based instructions (9)
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
packages/*/src/**/*.py: All importable Python code in packages must live under packages//src/
All public APIs in packaged code require Python 3.11+ type hints; prefer typing/collections.abc; use typing.Annotated when useful
Provide Google-style docstrings for public APIs in packages; first line concise with a period; use backticks for code entities
Files:
packages/nvidia_nat_test/src/nat/test/llm.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
Files:
packages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_test/tests/test_test_llm.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
packages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_test/tests/test_test_llm.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
packages/nvidia_nat_test/src/nat/test/llm.pypackages/nvidia_nat_test/tests/test_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_test/src/nat/test/llm.pypackages/nvidia_nat_test/tests/test_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_test/src/nat/test/llm.pypackages/nvidia_nat_test/tests/test_test_llm.py
packages/*/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Packages containing Python code must have tests under packages//tests
Files:
packages/nvidia_nat_test/tests/test_test_llm.py
**/tests/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Test files must be named test_*.py and placed under a tests/ folder
Files:
packages/nvidia_nat_test/tests/test_test_llm.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
packages/nvidia_nat_test/tests/test_test_llm.py
🧠 Learnings (1)
📚 Learning: 2025-08-28T23:22:41.742Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-28T23:22:41.742Z
Learning: Applies to packages/*/pyproject.toml : Package pyproject.toml must depend on nvidia-nat or a package starting with nvidia-nat- using ~=<two-digit> versions (e.g., ~=1.0)
Applied to files:
packages/nvidia_nat_test/src/nat/test/llm.py
🧬 Code graph analysis (1)
packages/nvidia_nat_test/tests/test_test_llm.py (3)
src/nat/builder/workflow_builder.py (1)
WorkflowBuilder(130-940)src/nat/runtime/loader.py (1)
load_workflow(97-117)src/nat/runtime/session.py (1)
workflow(84-85)
🪛 Ruff (0.12.2)
packages/nvidia_nat_test/src/nat/test/llm.py
70-70: Unused function argument: builder
(ARG001)
76-76: Unused function argument: builder
(ARG001)
83-83: Unused method argument: messages
(ARG002)
87-87: Unused method argument: messages
(ARG002)
91-91: Unused method argument: messages
(ARG002)
95-95: Unused method argument: messages
(ARG002)
103-103: Unused function argument: builder
(ARG001)
109-110: Avoid specifying long messages outside the exception class
(TRY003)
116-116: Unused method argument: messages
(ARG002)
120-120: Unused method argument: messages
(ARG002)
124-124: Unused method argument: messages
(ARG002)
129-129: Unused method argument: messages
(ARG002)
138-138: Unused function argument: builder
(ARG001)
145-145: Unused method argument: messages
(ARG002)
145-145: Unused method argument: kwargs
(ARG002)
153-153: Unused function argument: builder
(ARG001)
160-161: Avoid specifying long messages outside the exception class
(TRY003)
167-167: Unused method argument: chat_history
(ARG002)
172-172: Unused method argument: chat_history
(ARG002)
182-182: Unused function argument: builder
(ARG001)
189-189: Unused method argument: messages
(ARG002)
193-193: Unused method argument: messages
(ARG002)
197-197: Unused method argument: messages
(ARG002)
201-201: Unused method argument: messages
(ARG002)
packages/nvidia_nat_test/tests/test_test_llm.py
42-42: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
77-77: Use of assert detected
(S101)
78-78: Use of assert detected
(S101)
119-119: Use of assert detected
(S101)
142-142: Use of assert detected
(S101)
143-143: Use of assert detected
(S101)
188-188: Use of assert detected
(S101)
189-189: Use of assert detected
(S101)
194-194: Use of assert detected
(S101)
195-195: Use of assert detected
(S101)
230-230: Use of assert detected
(S101)
233-233: Use of assert detected
(S101)
276-276: Use of assert detected
(S101)
280-280: Use of assert detected
(S101)
313-313: Use of assert detected
(S101)
316-316: Use of assert detected
(S101)
353-353: Use of assert detected
(S101)
363-363: Use of assert detected
(S101)
372-372: Use of assert detected
(S101)
380-380: Use of assert detected
(S101)
381-381: Use of assert detected
(S101)
392-392: Use of assert detected
(S101)
398-398: Use of assert detected
(S101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (2)
packages/nvidia_nat_test/src/nat/test/llm.py (2)
1-15: nvidia-nat dependency correctly declared
packages/nvidia_nat_test/pyproject.toml already includes "nvidia-nat~=1.3", satisfying the two-digit~=rule; no change needed. Optionally add extras guidance for wrapper dependencies.
116-133: Use explicitcontent=keyword when constructingChatMessageand prefix unusedmessageswith_.
Update calls fromChatMessage(chooser.next_response())to
ChatMessage(content=chooser.next_response())and rename the unused
messagesparameter to_messages.
Please verify in your environment thatChatMessagesupports thecontentkeyword.
2 participants
Description
Closes AIQ-439
response_seqand artificial latency viadelay_ms.LangChain,LlamaIndex,CrewAI,SemanticKernel,Agno)nat_test_llmbehavior (cycling, latency, and stream yielding a single chunk).model_fieldsand a small typo.By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor
Style