Replace nest-asyncio with nest-asyncio2
#1190
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…st_asyncio patch wasn't already applied Signed-off-by: David Gardner <dagardner@nvidia.com>
…ed prior to the nest_asyncio2 patch Signed-off-by: David Gardner <dagardner@nvidia.com>
…into david-replace-nest-asyncio Signed-off-by: David Gardner <dagardner@nvidia.com>
|
Warning Rate limit exceeded@dagardner-nv has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplace nest_asyncio with nest_asyncio2 across tests and CLI, make the test fixture autouse and add a pre-patch assertion, modernize type hints with TYPE_CHECKING and forward references in the rag evaluator and tests, and bump several dependencies in pyproject.toml. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Pytest as Pytest Session
participant Plugin as require_nest_asyncio (autouse fixture)
participant Asyncio as asyncio
participant Nest as nest_asyncio2
Pytest->>Plugin: session start (autouse fixture runs)
Plugin->>Asyncio: import asyncio
Plugin->>Asyncio: assert not patched
alt assertion passes
Plugin->>Nest: lazy import nest_asyncio2
Plugin->>Nest: nest_asyncio2.patch(error_on_mispatched=True)
Nest->>Asyncio: patch asyncio (run/loop behavior)
Plugin-->>Pytest: fixture setup complete
else assertion fails
Plugin-->>Pytest: raise RuntimeError (advise lazy imports)
end
sequenceDiagram
autonumber
participant CLI as Program start
participant Entrypoint as entrypoint
participant Nest as nest_asyncio2
CLI->>Entrypoint: import/setup
Entrypoint->>Nest: import nest_asyncio2
Entrypoint->>Nest: nest_asyncio2.apply() / patch()
Entrypoint-->>CLI: continue startup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/nat/eval/rag_evaluator/evaluate.py (1)
18-36: RAGEvaluator typing / lazy ragas imports are correct; consider tightening remaining parameter typesThe switch to
typing.TYPE_CHECKINGplus string forward references forLangchainLLMWrapper,Metric,EvaluationDataset, andEvaluationResultcleanly removes top‑level ragas imports while preserving accurate types. ImportingEvaluationDataset/SingleTurnSampleinsideeval_input_to_ragasis a good fit for the “patch via nest_asyncio2 first, then load ragas” pattern.As a small follow‑up, you could make the constructor fully typed by annotating the remaining parameters:
- def __init__(self, - evaluator_llm: "LangchainLLMWrapper", - metrics: Sequence["Metric"], - max_concurrency=8, - input_obj_field: str | None = None): + def __init__( + self, + evaluator_llm: "LangchainLLMWrapper", + metrics: Sequence["Metric"], + max_concurrency: int = 8, + input_obj_field: str | None = None, + ):This keeps the public API more self‑documenting without changing behavior.
Also applies to: 42-47, 71-75, 106-111
📜 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 (5)
packages/nvidia_nat_test/src/nat/test/plugin.py(2 hunks)pyproject.toml(3 hunks)src/nat/cli/entrypoint.py(2 hunks)src/nat/eval/rag_evaluator/evaluate.py(4 hunks)tests/nat/eval/rag_evaluator/test_rag_evaluate.py(3 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:
src/nat/cli/entrypoint.pysrc/nat/eval/rag_evaluator/evaluate.pypyproject.tomltests/nat/eval/rag_evaluator/test_rag_evaluate.pypackages/nvidia_nat_test/src/nat/test/plugin.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/cli/entrypoint.pysrc/nat/eval/rag_evaluator/evaluate.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/eval/rag_evaluator/test_rag_evaluate.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/plugin.py
🧠 Learnings (2)
📓 Common learnings
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.
📚 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:
pyproject.tomlpackages/nvidia_nat_test/src/nat/test/plugin.py
🧬 Code graph analysis (1)
src/nat/eval/rag_evaluator/evaluate.py (2)
tests/nat/eval/test_evaluate.py (1)
eval_input(68-79)src/nat/eval/evaluator/evaluator_model.py (2)
EvalInput(46-47)EvalOutput(56-58)
⏰ 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 (3)
src/nat/cli/entrypoint.py (1)
31-31: Switch tonest_asyncio2in CLI entrypoint looks correctImporting
nest_asyncio2and callingnest_asyncio2.apply()early in the module preserves the previous behavior while routing nested event‑loop support through the new package, which matches the PR’s goal. No further changes needed here.Also applies to: 53-53
pyproject.toml (1)
32-32: Dependency updates align with the nest_asyncio2 migration; verify async endpoints with new versionsReplacing
nest-asynciowithnest-asyncio2~=1.6, relaxinguvicorn[standard]to~=0.38, and updatingdask/distributedto~=2025.11follow the project’s versioning and sorting guidelines and are consistent with the PR’s objectives. Please make sure the CLI andasync_endpointsflows are exercised in CI (or locally) with these versions to catch any compatibility surprises.Also applies to: 51-51, 143-144
tests/nat/eval/rag_evaluator/test_rag_evaluate.py (1)
16-17: Forward‑ref typing and lazy ragas imports in tests look goodUsing a
typing.TYPE_CHECKINGblock plus string return types forragas_judge_llm/ragas_metrics, and moving the ragas imports (includingEvaluationDataset/SingleTurnSample) inside fixtures/tests, cleanly avoids import‑time ragas side effects while keeping type checkers happy. This fits well with the nest_asyncio2‑first strategy.Also applies to: 30-33, 40-53, 78-80
…avid-replace-nest-asyncio Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
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 (1)
pyproject.toml(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ 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:
pyproject.toml
🧠 Learnings (1)
📚 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:
pyproject.toml
🔇 Additional comments (2)
pyproject.toml (2)
51-51: Uvicorn dependency update follows guidelines.The use of
~=0.38with 2-digit precision aligns with project versioning philosophy and removes the previous strict upper-bound constraint, enabling compatibility with newer asyncio features.
145-146: Dask/distributed dependency bumps are well-scoped and properly formatted.The updates to
~=2025.11follow the 2-digit compatible release format, remove the previous exact pins that constrained asyncio compatibility, and are appropriately scoped to theasync_endpointsoptional dependency group.
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
Maybe add a comment somewhere in the code about needing to lazily import ragas related code.
Signed-off-by: David Gardner <dagardner@nvidia.com>
…yncio is applied Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pyproject.toml (1)
32-32: Use compatible release version specifier per project guidelines.The exact pin
==1.6.3conflicts with the project's versioning philosophy stated in lines 16–19, which recommends~=with 2-digit precision to keep constraints open while maintaining stability.Apply this diff to align with project guidelines:
- "nest-asyncio2==1.6.3", + "nest-asyncio2~=1.6",packages/nvidia_nat_test/src/nat/test/plugin.py (1)
488-503: Add return type annotation and fix docstring typos.Per coding guidelines, Python methods should use type hints for all parameters and return values. Additionally, the docstring contains typos and awkward phrasing that should be corrected.
Based on coding guidelines
Apply this diff to fix the issues:
@pytest.fixture(name="require_nest_asyncio", scope="session", autouse=True) -def require_nest_asyncio_fixture(): +def require_nest_asyncio_fixture() -> None: """ - Some tests require either nest_asyncio (or nest_asyncio2) to be installed to allow nested event loops, calling - nest_asyncio2.apply() more than once is a no-op so. However we need to ensure that the nest_asyncio2 patch is - applied prior to the older nest_asyncio patch is applued. + Some tests require either ``nest_asyncio`` or ``nest_asyncio2`` to allow nested event loops. Calling + ``nest_asyncio2.apply()`` more than once is a no-op, but we need to ensure that the ``nest_asyncio2`` patch + is applied before any older ``nest_asyncio`` patch. - Requiring us to ensure that any library which will apply the patch on import is lazily imported. + This requires us to lazily import any library that applies the patch on import. """ - assert not hasattr(asyncio, "_nest_patched"), \ - ("nest_asyncio2 fixture called but asyncio is already patched, most likely this is due to the nest_asyncio " - "being applied first, which is not compatible with Python 3.12+. Please ensure that any libraries which " - "apply nest_asyncio on import are lazily imported.") + assert not hasattr(asyncio, "_nest_patched"), ( + "require_nest_asyncio fixture called but asyncio is already patched, most likely due to nest_asyncio " + "being applied first, which is not compatible with Python 3.12+. Please ensure that any libraries which " + "apply nest_asyncio on import are lazily imported." + ) import nest_asyncio2 nest_asyncio2.apply()
📜 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 (4)
packages/nvidia_nat_test/src/nat/test/plugin.py(2 hunks)pyproject.toml(3 hunks)src/nat/eval/rag_evaluator/evaluate.py(4 hunks)tests/nat/eval/rag_evaluator/test_rag_evaluate.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/nat/eval/rag_evaluator/test_rag_evaluate.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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/plugin.pypyproject.tomlsrc/nat/eval/rag_evaluator/evaluate.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/plugin.py
src/nat/**/*
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/eval/rag_evaluator/evaluate.py
🧠 Learnings (2)
📚 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_test/src/nat/test/plugin.pypyproject.toml
📚 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:
pyproject.toml
🧬 Code graph analysis (1)
src/nat/eval/rag_evaluator/evaluate.py (2)
tests/nat/eval/test_evaluate.py (1)
eval_input(68-79)src/nat/eval/evaluator/evaluator_model.py (2)
EvalInput(46-47)EvalOutput(56-58)
⏰ 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 (7)
pyproject.toml (2)
51-51: LGTM!The uvicorn version update to
~=0.38correctly follows project guidelines and removes the previous upper-bound constraint, supporting the PR's goal of eliminating compatibility issues withnest-asyncio.
145-146: I need to investigate further to understand the context of these dependencies.No critical breaking changes detected; rely on integration testing given indirect dependency usage.
The version specifiers
~=2025.11correctly follow project guidelines with 2-digit precision. Recent Dask.distributed 2025.11.0 changes primarily affect CLI deprecation patterns and remove previously-deprecated CLI options, not core APIs that would impact general library consumers. No direct imports of dask or distributed packages were found in non-test code, suggesting these are indirect/transitive dependencies. While the two-year version jump (from 2023.6) poses no documented breaking changes to public APIs, actual compatibility should be verified through integration testing and dependency resolution to confirm no unforeseen issues arise with this project's specific use cases.packages/nvidia_nat_test/src/nat/test/plugin.py (1)
16-16: LGTM!The asyncio import is necessary to support the precondition assertion in the
require_nest_asynciofixture.src/nat/eval/rag_evaluator/evaluate.py (4)
18-18: LGTM!The typing import is necessary to support the TYPE_CHECKING guard that enables lazy imports of ragas types.
45-46: LGTM!Forward-referenced type hints correctly defer ragas imports to type-checking time while maintaining type safety.
73-77: LGTM!The lazy imports of
EvaluationDatasetandSingleTurnSamplewithin the method correctly defer ragas imports to runtime, ensuringnest_asyncio2can be applied first.
108-108: LGTM!Forward-referenced return type maintains type safety while avoiding import-time side effects from ragas.
Signed-off-by: David Gardner <dagardner@nvidia.com>
…into david-replace-nest-asyncio Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…, clean-up docstring Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nvidia_nat_test/src/nat/test/plugin.py (1)
488-503: Address missing return type annotation and improve docstring clarity.This fixture has a similar concern to the past review comment. Please add the return type annotation and improve the docstring:
Missing return type: Add
-> Noneto the fixture signature per coding guidelines.Docstring issues:
- Grammar: "is applied prior to the older nest_asyncio patch is applied" has redundant wording
- Use backticks for
nest_asyncio2for consistency- Last sentence could be clearer
Optional - Long error message: The static analysis tool flags the long error message (TRY003), though the detailed message is helpful for debugging. Consider shortening if preferred.
Apply this diff:
@pytest.fixture(name="require_nest_asyncio", scope="session", autouse=True) -def require_nest_asyncio_fixture(): +def require_nest_asyncio_fixture() -> None: """ - Some tests require the nest_asyncio2 patch to be applied to allow nested event loops, calling - `nest_asyncio2.apply()` more than once is a no-op. However we need to ensure that the nest_asyncio2 patch is - applied prior to the older nest_asyncio patch is applied. Requiring us to ensure that any library which will apply - the patch on import is lazily imported. + Some tests require either ``nest_asyncio`` or ``nest_asyncio2`` to allow nested event loops. Calling + ``nest_asyncio2.apply()`` more than once is a no-op, but we must ensure the ``nest_asyncio2`` patch + is applied before any older ``nest_asyncio`` patch. This requires lazily importing any library that + applies the patch on import. """ import nest_asyncio2 try: nest_asyncio2.apply(error_on_mispatched=True) except RuntimeError as e: raise RuntimeError( - "nest_asyncio2 fixture called but asyncio is already patched, most likely this is due to the nest_asyncio " - "being applied first, which is not compatible with Python 3.12+. Please ensure that any libraries which " - "apply nest_asyncio on import are lazily imported.") from e + "nest_asyncio2 fixture called but asyncio is already patched. " + "Most likely nest_asyncio was applied first (incompatible with Python 3.12+). " + "Please ensure libraries that apply nest_asyncio on import are lazily imported." + ) from e
📜 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_test/src/nat/test/plugin.py(2 hunks)pyproject.toml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 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_test/src/nat/test/plugin.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/plugin.py
🧠 Learnings (2)
📓 Common learnings
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.
📚 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_test/src/nat/test/plugin.py
🪛 Ruff (0.14.5)
packages/nvidia_nat_test/src/nat/test/plugin.py
500-503: Avoid specifying long messages outside the exception class
(TRY003)
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nvidia_nat_test/src/nat/test/plugin.py (1)
500-503: Consider extracting the long error message to a constant.Static analysis flags the long error message in the RuntimeError. While the detailed message is helpful, you could improve code organization by extracting it to a module-level constant.
For example:
_NEST_ASYNCIO_PATCH_ERROR = ( "nest_asyncio2 fixture called but asyncio is already patched, most likely this is due to the nest_asyncio " "being applied first, which is not compatible with Python 3.12+. Please ensure that any libraries which " "apply nest_asyncio on import are lazily imported." )Then use:
except RuntimeError as e: - raise RuntimeError( - "nest_asyncio2 fixture called but asyncio is already patched, most likely this is due to the nest_asyncio " - "being applied first, which is not compatible with Python 3.12+. Please ensure that any libraries which " - "apply nest_asyncio on import are lazily imported.") from e + raise RuntimeError(_NEST_ASYNCIO_PATCH_ERROR) from e
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nvidia_nat_test/src/nat/test/plugin.py(1 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.
Referenced Documentation Contents
pyproject.toml:
Dependencies: replace nest-asyncio with nest-asyncio2 (1.7.1); uvicorn[standard] version adjusted to ~0.38; adjust async-related packages: remove ...
Files:
packages/nvidia_nat_test/src/nat/test/plugin.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/plugin.py
🧠 Learnings (2)
📓 Common learnings
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.
📚 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_test/src/nat/test/plugin.py
🪛 Ruff (0.14.5)
packages/nvidia_nat_test/src/nat/test/plugin.py
500-503: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
packages/nvidia_nat_test/src/nat/test/plugin.py (1)
488-503: LGTM! The nest_asyncio2 migration and autouse fixture are well implemented.The changes align perfectly with the PR objectives:
autouse=Trueensures the patch is applied early and consistently across all testserror_on_mispatched=Trueprovides early detection if asyncio was already patched by the wrong library- The detailed error message guides developers to lazy-import problematic libraries
- The docstring clearly explains the patch ordering requirements
…thout a return type Signed-off-by: David Gardner <dagardner@nvidia.com>
|
/merge |
4 participants
Description
ragasis lazily imported since it will apply the nest-asyncio patch on import, and we need to ensure that nest-asyncio2 is applied instead.Closes #770
By Submitting this PR I confirm:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.