Bug/strands unit tests#1196
Conversation
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughUpdates the strands-agents dependency constraint from ~=1.15 to ~=1.17 and increases max_tokens in multiple LLM integration tests in the nvidia_nat_strands package; no other functional changes are present. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (2)
packages/nvidia_nat_strands/pyproject.toml(1 hunks)packages/nvidia_nat_strands/tests/test_strands_integration.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ 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_strands/pyproject.tomlpackages/nvidia_nat_strands/tests/test_strands_integration.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_strands/pyproject.tomlpackages/nvidia_nat_strands/tests/test_strands_integration.py
🧠 Learnings (2)
📚 Learning: 2025-11-10T21:26:35.059Z
Learnt from: jiaxiangr
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 974
File: packages/nvidia_nat_all/pyproject.toml:39-39
Timestamp: 2025-11-10T21:26:35.059Z
Learning: In packages/nvidia_nat_all/pyproject.toml, workspace dependencies (nvidia-nat-* plugin packages) should NOT have version constraints because they are managed as workspace dependencies. Version constraints are only applied to the base nvidia-nat package and external dependencies, not to internal workspace packages.
Applied to files:
packages/nvidia_nat_strands/pyproject.toml
📚 Learning: 2025-11-14T20:33:53.944Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 1181
File: packages/nvidia_nat_test/tests/test_test_llm.py:419-484
Timestamp: 2025-11-14T20:33:53.944Z
Learning: The NVIDIA NeMo-Agent-Toolkit project uses pytest-asyncio in strict mode (the default), which requires pytest.mark.asyncio decorator on all async test functions. All async tests in packages/nvidia_nat_test/tests/test_test_llm.py consistently follow this pattern.
Applied to files:
packages/nvidia_nat_strands/tests/test_strands_integration.py
🧬 Code graph analysis (1)
packages/nvidia_nat_strands/tests/test_strands_integration.py (2)
src/nat/llm/openai_llm.py (1)
OpenAIModelConfig(32-53)src/nat/llm/nim_llm.py (1)
NIMModelConfig(33-55)
🔇 Additional comments (6)
packages/nvidia_nat_strands/tests/test_strands_integration.py (5)
22-22: LGTM!The
osimport is required for the new environment-aware fixtures.
144-144: LGTM!The max_tokens increase to 256 aligns with the PR objective to prevent intermittent test failures, especially for tests involving multiple tool calls.
169-169: LGTM!The max_tokens increase is appropriate for error handling scenarios where the agent needs to explain errors.
220-220: LGTM!The max_tokens increase to 256 is consistent with changes in other LLM provider tests and helps prevent intermittent failures.
368-368: LGTM!The max_tokens increase to 256 is consistent with changes across all LLM provider tests and addresses the intermittent test failure issue mentioned in the PR objectives.
packages/nvidia_nat_strands/pyproject.toml (1)
40-40: No action required — version 1.17 is available and secure.Version 1.17.0 of strands-agents exists on PyPI and no security vulnerabilities were found. The dependency specification is correct.
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
/ok to test e562ee5 |
|
/ok to test 0c2b889 |
|
/merge |
Bumping up strands-agents to v1.17.0 to properly support "system_prompt_content" parameter in strands.models.openai.OpenAIModel.format_request_messages(). Also bumping up token limit in E2E integration tests from 65/96 to 256 to avoid intermittent errors with simple_calculator integration test. Closes - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. * **Dependencies** * Updated strands-agents dependency from 1.15 to 1.17 * **Improvements** * Increased model token limits in integration tests (up to 256) to allow longer interactions * Improved test infrastructure with environment-aware fixtures for conditional integration test execution <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - David Gardner (https://github.com/dagardner-nv) - https://github.com/Salonijain27 URL: NVIDIA#1196
Description
Bumping up strands-agents to v1.17.0 to properly support "system_prompt_content" parameter in strands.models.openai.OpenAIModel.format_request_messages(). Also bumping up token limit in E2E integration tests from 65/96 to 256 to avoid intermittent errors with simple_calculator integration test.
Closes
By Submitting this PR I confirm:
Summary by CodeRabbit
Dependencies
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.