[https://nvbugs/6337235][test] Remove stale MX/GMS model loader skips - #15987
Conversation
|
/bot run --disable-fail-fast |
|
PR_Github #57799 [ run ] triggered by Bot. Commit: |
|
PR_Github #57799 [ run ] completed with state
|
5fa59ff to
77440c2
Compare
|
/bot run --disable-fail-fast --stage-list "A10-PyTorch-1-cbts" |
|
PR_Github #57810 [ run ] triggered by Bot. Commit: |
|
PR_Github #57810 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "A10-PyTorch-1" |
|
PR_Github #57854 [ run ] triggered by Bot. Commit: |
|
PR_Github #57854 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #57909 [ run ] triggered by Bot. Commit: |
|
PR_Github #57909 [ run ] completed with state
|
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
77440c2 to
88f9cf4
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #58044 [ run ] triggered by Bot. Commit: |
|
PR_Github #58044 [ run ] completed with state |
📝 WalkthroughWalkthroughRemoves a skip marker from GMS model loader tests, enabling them to run. Refactors MX checkpoint loader tests to introduce a shared SourceIdentity fixture and helper for stubbing metadata fetch, and removes NVBUG skip decorators from timeout-related tests. ChangesTest suite re-enablement and MX loader test refactor
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py (1)
489-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the new helper.
_allow_mx_source_identitylacks parameter and return type annotations, per repo guideline requiring functions to always be annotated.📝 Suggested annotation fix
-def _allow_mx_source_identity(loader): +def _allow_mx_source_identity(loader: MXCheckpointLoader) -> dict[str, SourceIdentity]: """Let MX P2P path tests bypass the metadata fetch seam with a matching identity.""" loader._fetch_source_identity = MagicMock(return_value=_SOURCE_IDENTITY) return {"source_identity": _SOURCE_IDENTITY}As per coding guidelines, "Always annotate functions with return types (use
Noneif no return); annotate class members and variables when necessary".🤖 Prompt for 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. In `@tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py` around lines 489 - 494, Add type annotations to the new helper _allow_mx_source_identity so it follows the repository rule that all functions must be annotated. Specify the loader parameter type for the checkpoint loader object being patched, and add an explicit return type for the dictionary it returns; keep the existing behavior of assigning MagicMock to loader._fetch_source_identity and returning the source_identity mapping.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py`:
- Around line 489-494: Add type annotations to the new helper
_allow_mx_source_identity so it follows the repository rule that all functions
must be annotated. Specify the loader parameter type for the checkpoint loader
object being patched, and add an explicit return type for the dictionary it
returns; keep the existing behavior of assigning MagicMock to
loader._fetch_source_identity and returning the source_identity mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c32ec541-11f6-4295-adff-dc0dea723184
📒 Files selected for processing (2)
tests/unittest/_torch/executor/test_model_loader_gms.pytests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py
💤 Files with no reviewable changes (1)
- tests/unittest/_torch/executor/test_model_loader_gms.py
…NVIDIA#15987) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…NVIDIA#15987) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Description
Remove stale pytest skips that were added by #15368 for the MX/GMS model-loader coverage gap tracked by https://nvbugs/6337235.
#15471 merged before #15368 and fixed the stale GMS/MX
ModelLoaderfixtures, so the blanket skip no longer needs to suppress these tests. Unskipping the MX checkpoint-loader tests also exposed stale P2P-path fixtures: currentMXCheckpointLoader.load_weights()now requires a matchingSourceIdentitybefore it proceeds with MX transfer. This PR updates those branch-focused tests to provide a fixed matching identity and patch the MX metadata fetch seam, leaving the dedicated SourceIdentity gate coverage intests/unittest/_torch/weight_sharing/test_mx_source_identity_gate.pyas the source of truth for compatibility behavior.Test Coverage
PYTHONPYCACHEPREFIX=/private/tmp/trtllm_pycache python3 -m py_compile tests/unittest/_torch/executor/test_model_loader_gms.py tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.pygit diff --check -- tests/unittest/_torch/executor/test_model_loader_gms.py tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.pywaive list checkandvalidate-test-listslocally because this machine uses Python 3.9 for those hooks and fails on Python 3.10 union syntax inscripts/check_test_list.py./opt/miniconda3/bin/pytestandPYTHONPATH=$PWD, but this local environment is missingtransformers, imported bytests/unittest/conftest.py./LLM/main/L0_MergeRequest_PR #46496failed only inA10-PyTorch-1-cbtson the newly unskipped MX tests.References: #15368, #15471
Summary by CodeRabbit