[https://nvbugs/5979673][fix] Unwaive test_agent_multi_backends.py::test_run_with_different_env - #14939
Conversation
The trtllm dev environment used to install the meta `nixl` package, which provides a top-level `nixl/__init__.py` shim that redirects to `nixl_cu13` / `nixl_cu12`. After NVIDIA#14436 switched the dependency to `nixl-cu13` directly, the meta shim is no longer present, so `from nixl import nixl_agent, ...` in `tensorrt_llm/_torch/disaggregation/nixl/_agent_py.py` raises `ModuleNotFoundError: No module named 'nixl'` at import time, which in turn breaks `test_agent_multi_backends::test_run_with_different_env[1]` during collection of the spawned subprocess pytest. Resolve the symbols from `nixl_cu13` (or `nixl_cu12`) first and fall back to the meta-package only when neither CUDA-specific package is installed. Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
This case (TRTLLM_USE_PY_NIXL_KVCACHE=1) was waived under nvbugs/5979673 because the meta `nixl` package pulled the latest `nixl-cu12` wheel, whose `_bindings.so` referenced symbols absent from the source-built 0.9.0 `libnixl.so`. With `nixl-cu13==0.9.0` directly pinned in requirements-dev.txt, the bindings and the libnixl in the container are ABI-consistent again, so the test is expected to pass. Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
|
/bot skip --comment "CI had already passed in /LLM/main/L0_MergeRequest_PR pipeline #40891 before handling the waive file conflicts." |
📝 WalkthroughWalkthroughThis PR updates the NIXL module dependency import logic to support CUDA-specific package variants with automatic fallback, and removes a corresponding test waiver that is no longer needed following the import fix. ChangesNIXL Dependency Import and Test Waiver
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/disaggregation/nixl/_agent_py.py`:
- Around line 17-21: The loop that probes NIXL candidates uses
importlib.import_module(_candidate) and currently catches ImportError broadly,
which may hide failures inside a module; change the except clause to catch
ModuleNotFoundError as e, inspect e.name (or e.args[0]) and only continue when
the missing module equals the candidate string (_candidate); for any other
ModuleNotFoundError or any other ImportError/exception raised by
importlib.import_module, re-raise it so real import-time errors inside the
candidate are not swallowed; ensure this logic references _candidate and
_nixl_mod in the same block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b508712-758f-48fc-9d99-846e9df16b48
📒 Files selected for processing (2)
tensorrt_llm/_torch/disaggregation/nixl/_agent_py.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
PR_Github #51980 [ skip ] triggered by Bot. Commit: |
|
PR_Github #51980 [ skip ] completed with state |
…est_run_with_different_env (NVIDIA#14939) Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
…est_run_with_different_env (NVIDIA#14939) Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com> Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
Summary by CodeRabbit
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.