Skip to content

[https://nvbugs/6287721][chore] Unwaive a test for update weights - #16102

Open
shikicloud wants to merge 1 commit into
NVIDIA:mainfrom
shikicloud:waive-nvbugs
Open

[https://nvbugs/6287721][chore] Unwaive a test for update weights#16102
shikicloud wants to merge 1 commit into
NVIDIA:mainfrom
shikicloud:waive-nvbugs

Conversation

@shikicloud

@shikicloud shikicloud commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Removed the part0 and specific FP8 Qwen3 8B waivers.
  • Retained the multi-GPU part1 and part4 waivers.
  • Replaced the 4-GPU Ray part0 marker with explicit FP8 weight-update tests for Qwen3 8B and Qwen3 30B-A3B.

Dev Engineer Review

  • The changes use valid test-list formats.
  • The explicit test entries match the intended Qwen3 FP8 weight-update coverage.
  • No code, API, performance, or error-handling changes are included.
  • No duplicate or unintended public test entries were identified.

QA Engineer Review

  • Modified tests/integration/test_lists/waives.txt:
    • Removed the part0 waiver.
    • Removed the specific FP8 Qwen3 8B waiver.
    • Retained the part1 and part4 waivers.
  • Modified tests/integration/test_lists/test-db/l0_dgx_b200.yml:
    • Removed the part0 marker-based entry.
    • Added explicit FP8 weight-update tests for Qwen3 8B and Qwen3 30B-A3B.
  • CI confirmed that the unwaived tests passed.
  • Verdict: sufficient.

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-compatible or api-breaking. For api-breaking, include BREAKING in 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.

@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run --stage-list "DGX_B200-4_GPUs-PyTorch-Ray-1"

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e6bd2046-79ad-4d14-9e66-0af0d2ea7a7d

📥 Commits

Reviewing files that changed from the base of the PR and between bfc4966 and 0f87f28.

📒 Files selected for processing (2)
  • tests/integration/test_lists/test-db/l0_dgx_b200.yml
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt

Walkthrough

The Ray 4-GPU pre-merge list now runs explicit FP8 weight-update tests for Qwen3 8B and Qwen3 30B-A3B. The obsolete part0 and Qwen3 8B waivers were removed.

Changes

Ray FP8 test selection

Layer / File(s) Summary
Configure explicit FP8 tests and waivers
tests/integration/test_lists/test-db/l0_dgx_b200.yml, tests/integration/test_lists/waives.txt
The test list replaces the part0 marker with explicit Qwen3 8B and Qwen3 30B-A3B FP8 weight-update tests. The waiver list removes the matching part0 and Qwen3 8B entries while retaining part1 and part4 waivers.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: qijune, brnguyen2, pcastonguay, bowenfu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description retains the template but leaves the Description and Test Coverage sections empty and provides no implementation or testing details. Add a short explanation of the issue and solution, and list the relevant test coverage and results.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the NVBugs ticket, change type, and primary change: removing a waiver for an update-weights test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@shikicloud shikicloud changed the title [https://nvbugs/6287721][chore] Unwaive update weights test for specific models [https://nvbugs/6287721][chore] Waive update weights test for specific models Jul 8, 2026
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58151 [ run ] triggered by Bot. Commit: 2e0fedd Link to invocation

Comment thread tests/integration/test_lists/waives.txt Outdated
unittest/_torch/ray_orchestrator/multi_gpu/test_llm_update_weights_multi_gpu.py::test_llm_partial_update_weights_nvfp4[auto-Qwen3/Qwen3-8B] SKIP (https://nvbugs/6372690)
unittest/_torch/ray_orchestrator/multi_gpu/test_llm_update_weights_multi_gpu.py::test_llm_partial_update_weights_nvfp4[fp8-Qwen3/Qwen3-30B-A3B] SKIP (https://nvbugs/6372690)
unittest/_torch/ray_orchestrator/multi_gpu/test_llm_update_weights_multi_gpu.py::test_llm_partial_update_weights_nvfp4[fp8-Qwen3/Qwen3-8B] SKIP (https://nvbugs/6372690)
unittest/_torch/ray_orchestrator/multi_gpu/test_llm_update_weights_multi_gpu.py::test_llm_update_weights_fp8[Qwen3/Qwen3-8B-Qwen3/Qwen3-8B-FP8] SKIP (https://nvbugs/6287721)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If skipping both tests is as expected from a functionality perspective, then you may use pytest.skip in the code. If not, and both should be TODOs, then keep it.

@Superjomn Superjomn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shikicloud shikicloud changed the title [https://nvbugs/6287721][chore] Waive update weights test for specific models [https://nvbugs/6287721][chore] Unwaive a test for update weights Jul 8, 2026
@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run --stage-list "DGX_B200-4_GPUs-PyTorch-Ray-1"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58176 [ run ] triggered by Bot. Commit: a31e9e0 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58151 [ run ] completed with state ABORTED. Commit: 2e0fedd

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58176 [ run ] completed with state FAILURE. Commit: a31e9e0
/LLM/main/L0_MergeRequest_PR pipeline #46826 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run --stage-list "DGX_B200-4_GPUs-PyTorch-Ray-1"

1 similar comment
@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run --stage-list "DGX_B200-4_GPUs-PyTorch-Ray-1"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58252 [ run ] triggered by Bot. Commit: ad85689 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58252 [ run ] completed with state FAILURE. Commit: ad85689
/LLM/main/L0_MergeRequest_PR pipeline #46893 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run --stage-list "DGX_B200-4_GPUs-PyTorch-Ray-1"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58267 [ run ] triggered by Bot. Commit: 3f55b11 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58267 [ run ] completed with state SUCCESS. Commit: 3f55b11
/LLM/main/L0_MergeRequest_PR pipeline #46906 (Partly Tested) completed with status: 'SUCCESS'

CI Report

Link to invocation

@Superjomn

Superjomn commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

@Superjomn

Copy link
Copy Markdown
Collaborator

/bot skip --comment "the unwaived tests passed"

@Superjomn
Superjomn enabled auto-merge (squash) July 13, 2026 07:41
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58929 [ skip ] triggered by Bot. Commit: 3f55b11 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #58929 [ skip ] completed with state SUCCESS. Commit: 3f55b11
Skipping testing for commit 3f55b11

Link to invocation

@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run --stage-list "DGX_B200-4_GPUs-PyTorch-Ray-1" -- unittest/_torch/ray_orchestrator/multi_gpu/test_llm_update_weights_multi_gpu.py::test_llm_update_weights_fp8[Qwen3/Qwen3-8B-Qwen3/Qwen3-8B-FP8] --run-ray

auto-merge was automatically disabled August 5, 2026 08:40

Head branch was pushed to by a user without write access

@shikicloud
shikicloud requested review from a team as code owners August 5, 2026 08:40

@VALLIS-NERIA VALLIS-NERIA left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not change KV cache manager code. No objection from the KV cache manager ownership scope.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run

@shikicloud
shikicloud removed request for a team August 11, 2026 08:16
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65243 [ run ] triggered by Bot. Commit: 0f87f28 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65243 [ run ] completed with state FAILURE. Commit: 0f87f28
/LLM/main/L0_MergeRequest_PR pipeline #53021 completed with status: 'UNSTABLE'

CI Report

⚠️ Multi-GPU Label Required:
Multi-GPU tests require the ci: full pre-merge approved label on this PR. Ask a member of NVIDIA/trt-llm-ci-approvers to add the label, then re-trigger CI with the same bot command (no rebase needed).

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65261 [ run ] triggered by Bot. Commit: 0f87f28 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65261 [ run ] completed with state FAILURE. Commit: 0f87f28
/LLM/main/L0_MergeRequest_PR pipeline #53038 completed with status: 'UNSTABLE'

CI Report

⚠️ Multi-GPU Label Required:
Multi-GPU tests require the ci: full pre-merge approved label on this PR. Ask a member of NVIDIA/trt-llm-ci-approvers to add the label, then re-trigger CI with the same bot command (no rebase needed).

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

@shikicloud

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65273 [ run ] triggered by Bot. Commit: 0f87f28 Link to invocation

@brnguyen2 brnguyen2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the comments below are optional touch-ups, not blockers.

Unwaive itself looks right: part0 currently contains exactly test_llm_update_weights_fp8[...8B] and [...30B-A3B], so the two explicit entries reproduce the old marker's coverage, and the part1/part4 waivers correctly stay.

Two things worth adding to the description: what actually fixed https://nvbugs/6287721 (or a link to the CI run that shows the two tests passing), and why the marker entry was replaced with explicit IDs rather than just dropping the waiver line.

tests:
- unittest/llmapi/test_llm_multi_gpu_pytorch.py -m "gpu4"
- unittest/_torch/ray_orchestrator/multi_gpu/test_llm_update_weights_multi_gpu.py -m "part0"
- unittest/_torch/ray_orchestrator/multi_gpu/test_llm_update_weights_multi_gpu.py::test_llm_update_weights_fp8[Qwen3/Qwen3-8B-Qwen3/Qwen3-8B-FP8]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinning the two parametrizations explicitly means any test added later with @pytest.mark.part0 will not be scheduled here — the marker entry picked those up automatically. If the goal is only to unwaive, keeping -m "part0" and just deleting the waives.txt lines achieves the same thing without that gap. If you do want the explicit form (e.g. to re-waive a single param later), a short comment saying so would help the next person.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.