Skip to content

[Test] Cover tensors excluded from WeightChecker reset - #31621

Open
yuchenwang3 wants to merge 1 commit into
sgl-project:mainfrom
yuchenwang3:fix-weight-checker-reset-skip-weight-check
Open

yuchenwang3 wants to merge 1 commit into
sgl-project:mainfrom
yuchenwang3:fix-weight-checker-reset-skip-weight-check

Conversation

@yuchenwang3

@yuchenwang3 yuchenwang3 commented Jul 17, 2026 •

Copy link
Copy Markdown

Motivation

#40777 already made reset, comparison, and checksumming share _is_skip_weight_check, covering the fix originally proposed here. The existing reset tests do not cover a parameter marked _skip_weight_check.

Modifications

Keep a regression in TestResetTensors that marks a parameter as excluded, calls _reset_tensors, and checks that its value is preserved. The redundant production guard has been removed.

Testing

Rebased onto main 10edb05e72afb79ecfdf7dbe7603f90ba05ad3ee on October 3.

  • All applicable changed-file pre-commit hooks pass.
  • The existing regression body passes against the actual reset implementation using a manual CPU fixture. This bypasses the CUDA fixture and the constructor; it is a focused reset check.
  • The registered CUDA suite has not been rerun on this revision. The earlier 63-test GPU result belongs to cad0e99dc, before this rebase.

Accuracy Tests

Validation checks preservation of the excluded parameter during reset.

Speed Tests and Profiling

No performance benchmark; this revision adds test coverage only.

Checklist

  • Changed-file pre-commit hooks pass.
  • Regression added to the existing registered test suite.
  • Documentation applicability reviewed; this revision adds no API or configuration.
  • Validation scope stated above.
  • Followed the existing test style.

AI assistance: OpenAI Codex rebased the branch, removed the redundant guard, and ran local validation.


CI States

Latest PR Test (Base): ❌ Run #37160496178
Latest PR Test (Extra): ❌ Run #37160495962
Latest PR Test (AMD ROCm 10): ❌ Run #37160496146

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@yuchenwang3

Copy link
Copy Markdown
Author

@fzyzcjy pinging you since weight_checker.py is mostly your code. This fixes _reset_tensors resetting tensors that _compare/_compute_checksum already exclude via _skip_weight_check (e.g. fp8 KV-cache scales) — the self-test corrupts state the weight update never restores. One-file change, open for ~3 weeks without review. Could you take a look?

@yuchenwang3
yuchenwang3 force-pushed the fix-weight-checker-reset-skip-weight-check branch from 3aa1dfb to 5094145 Compare August 25, 2026 07:41
@yuchenwang3

Copy link
Copy Markdown
Author

Rebased this onto current main and added a regression case in the existing TestResetTensors suite. The test covers a tensor marked _skip_weight_check and verifies reset leaves it untouched. @fzyzcjy, could you take a look and add run-ci if this direction looks right?

@yuchenwang3

yuchenwang3 commented Aug 28, 2026 •

Copy link
Copy Markdown
Author

@b8zhong @ch-wan, could one of you review/triage this? You both recently reviewed or merged #27692 in the same WeightChecker path.

I have now rebased the PR onto current main; the current head is 3cd3ae989d83a097b2ded652d9538341dfa36154. The targeted reset symmetry harness passes, current-head lint is green, and the red checks are PR-gate failures with Missing required label 'run-ci'; the actual CPU/GPU suites were skipped.

The change makes _reset_tensors honor the same _skip_weight_check marker already honored by _compare and _compute_checksum, so unchecked derived tensors are not randomized and then left unverified. If the direction looks right, could you review it and run /tag-and-rerun-ci extra?

@yuchenwang3
yuchenwang3 force-pushed the fix-weight-checker-reset-skip-weight-check branch 2 times, most recently from 3cd3ae9 to e5c9e91 Compare September 3, 2026 22:06
@yuchenwang3

Copy link
Copy Markdown
Author

@fzyzcjy, following up on this small WeightChecker fix: reset should honor the same _skip_weight_check marker as comparison and checksumming. The regression test is included, but there is still no review. Could you review it or point me to the current owner, and authorize CI if appropriate? Thanks.

@b8zhong

b8zhong commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

/tag-and-rerun-ci

@b8zhong b8zhong added the run-ci CI: run the baseline test suite on this PR label Sep 14, 2026
@yuchenwang3
yuchenwang3 force-pushed the fix-weight-checker-reset-skip-weight-check branch 2 times, most recently from 84172fa to cad0e99 Compare September 17, 2026 16:59
@yuchenwang3

Copy link
Copy Markdown
Author

CI follow-up for current head cad0e99dc560b33289406a114809b4a02ff8158e:

These findings cover those two failures, not every red check. The branch is unchanged. Could you review the WeightChecker change and advise whether to refresh CI once the pending MLX fixture fix lands?

Assisted by OpenAI Codex for log inspection, the local base/head comparison, and this write-up.

@yuchenwang3

yuchenwang3 commented Oct 3, 2026 •

Copy link
Copy Markdown
Author

@b8zhong Rebased onto 10edb05e72 at a3f146f34c. #40777 already implemented the reset exclusion through _is_skip_weight_check, so I removed the redundant guard and kept the missing regression. This PR now changes only the test. Changed-file pre-commit checks and the regression body with a manual CPU fixture pass; the CUDA suite has not been rerun on this head. Could you review the remaining test coverage?

The current Intel image-build job failed before tests with no space left on device. The extra workflows also stop at the optional run-ci-extra label gate.

The AMD EAGLE3 job fails during RoPE graph capture with expected scalar type Half but found Float. The scheduled main job at ab9d8f029c12 has the same failure in TestBasicSanityEagle3.setUpClass. The HIP fallback retains the FP32 cache introduced by f6fcda8827, while rotary_embedding dispatches on the FP16 query and reads the cache as that type. This PR only adds nine lines to the WeightChecker test; it does not change either path. These logs do not establish a current-head pass for the WeightChecker CUDA suite.

Signed-off-by: Yuchen Wang <93700456+yuchenwang3@users.noreply.github.com>
@yuchenwang3
yuchenwang3 force-pushed the fix-weight-checker-reset-skip-weight-check branch from cad0e99 to a3f146f Compare October 3, 2026 23:03
@yuchenwang3 yuchenwang3 changed the title fix(weight-checker): skip _skip_weight_check tensors in _reset_tensors [Test] Cover tensors excluded from WeightChecker reset Oct 3, 2026

Copy link
Copy Markdown
Author

@yueming-yuan Since #40777 covers the reset logic, this now only adds the missing regression test. Could you take a look?

This branch has not been deployed

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

Labels

run-ci CI: run the baseline test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants