You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[https://nvbugs/7042835][test] Unwaive unittest/disaggregated fixed by #21597 - #21810
#21021 renamed KVCacheManagerV2.low_precision_dtype, but the BF16 context-first profile check still read the old name, so unittest/disaggregated failed in the CPU stage (https://nvbugs/7042835) and the whole directory was waived. #21597 fixed that reader. This PR removes the directory waive.
Test Coverage
unittest/disaggregated in the CPU stage (l0_cpu.yml), the only stage that runs the directory as a whole.
The PR removes the blanket unittest/disaggregated waiver for NVBUG 7042835 from tests/integration/test_lists/waives.txt. The unittest/disaggregated suite remains listed in the CPU test configuration. The change removes the skip; it does not change test code or establish that the tests pass.
QA Engineer Review
The PR modifies one test-list file and removes one directory-wide waiver. The CPU test list still includes unittest/disaggregated, so those tests can run without the blanket skip. The supplied evidence does not include completed test results. Coverage verdict: needs follow-up.
Per-File QA Perspective
tests/integration/test_lists/waives.txt: Removes the waiver for unittest/disaggregated. Verify that the CPU-stage suite runs as intended after removal.
…NVIDIA#21597NVIDIA#21021 renamed KVCacheManagerV2.low_precision_dtype, but the BF16
context-first profile check still read the old name, so
unittest/disaggregated failed in the CPU stage and the whole directory
was waived. NVIDIA#21597 fixed that reader. Remove the directory waive.
Signed-off-by: tianruih <tianruih@nvidia.com>
This change re-enables the disaggregated unit tests in the CPU stage. The reported fix and the clean run indicate it is safe to merge.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly states that the PR removes the unittest/disaggregated waive.
Description check
The description explains the issue, fix, and test coverage. The PR checklist section is missing.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
Reviewing files that changed from the base of the PR and between 2f528b2 and e7564db.
Files selected for processing: 1
tests/integration/test_lists/waives.txt
Files with no reviewable changes: 1
tests/integration/test_lists/waives.txt
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the one-line unwaive of unittest/disaggregated (nvbugs/7042835): 0 Blocker, 0 Possibly blocking, 1 Required, 0 Nice to have. The fix (#21597) is on the base, and all 42 sub-test failures behind the waive (pre-merge 68002) were the renamed low_precision_dtype reader. This PR's pre-merge run (L0_MergeRequest_PR 68667) ran test_unittests_v2[unittest/disaggregated] unwaived on CPU-Generic-x86-1 and CPU-Generic-arm-1: 1795 passed, 2 strict xfail, 0 failed. Waives match exact node ids, so this line only ever skipped the l0_cpu directory entry, and no GPU stage is affected.
[Required] The same root cause also left three DGX_H100 waives on main, and this unwaive should take them with it. After this PR's base, #21730 added full:DGX_H100/unittest/disaggregated/test_kv_transfer_mp.py::test_context_first_bf16_owned_transfer, full:DGX_H100/unittest/disaggregated/test_transfer_lifecycle_gpu.py::test_context_first_bf16_lifecycle_gpu and full:DGX_H100/disaggregated/test_disaggregated.py::test_disaggregated_context_first_bf16_ownership under nvbugs/7051902 (waives.txt:93, 95 and 96 on main). In post-merge 3055 (2a8d8e4, before #21597), test_context_first_bf16_owned_transfer[tp1/tp2] failed at and manager.low_precision_dtype is None with AttributeError: 'KVCacheManagerV2' object has no attribute 'low_precision_dtype', the same failure as 7042835. In post-merge 3057 (d782229, which includes #21597 and predates the waive), all three passed on DGX_H100-4_GPUs-PyTorch-Others-1/2. As it stands, the 4-GPU context-first BF16 transfer path stays skipped on DGX_H100 even though it is fixed, and 7051902 has no engineer assigned. Please rebase and drop those three lines here, confirmed with /bot run --stage-list "DGX_H100-4_GPUs-PyTorch-Others-1,DGX_H100-4_GPUs-PyTorch-Others-2", and close 7051902 as a duplicate of 7042835. If you'd rather do it in a follow-up, link 7051902 from this PR.
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.
Brings the branch up to date with main. main removed the waive of
unittest/disaggregated (NVIDIA#21810) after NVIDIA#21597 fixed those tests, so the
pre-merge CPU stage now runs the tests in tests/unittest/disaggregated,
including the ones this branch adds. No conflicts.
Signed-off-by: berryz <berryz@nvidia.com>
…ordinator socket test
main removed the unittest/disaggregated waiver (NVIDIA#21810), so that entry runs test_coordinator_uds.py again. The separate entry would run the file's five cases a second time.
Signed-off-by: berryz <berryz@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sunnyqgg
left a comment
Description
#21021 renamed
KVCacheManagerV2.low_precision_dtype, but the BF16 context-first profile check still read the old name, sounittest/disaggregatedfailed in the CPU stage (https://nvbugs/7042835) and the whole directory was waived. #21597 fixed that reader. This PR removes the directory waive.Test Coverage
unittest/disaggregatedin the CPU stage (l0_cpu.yml), the only stage that runs the directory as a whole.Dev Engineer Review
The PR removes the blanket
unittest/disaggregatedwaiver for NVBUG 7042835 fromtests/integration/test_lists/waives.txt. Theunittest/disaggregatedsuite remains listed in the CPU test configuration. The change removes the skip; it does not change test code or establish that the tests pass.QA Engineer Review
The PR modifies one test-list file and removes one directory-wide waiver. The CPU test list still includes
unittest/disaggregated, so those tests can run without the blanket skip. The supplied evidence does not include completed test results. Coverage verdict: needs follow-up.Per-File QA Perspective
tests/integration/test_lists/waives.txt: Removes the waiver forunittest/disaggregated. Verify that the CPU-stage suite runs as intended after removal.