Skip to content

Fix test assertions for 2-gpu - #772

Merged
kevalmorabia97 merged 1 commit into
feature/compressfrom
kmorabia/2-gpu-test-fix
Jan 13, 2026
Merged

kevalmorabia97 merged 1 commit into
feature/compressfrom
kmorabia/2-gpu-test-fix

Conversation

@kevalmorabia97

@kevalmorabia97 kevalmorabia97 commented Jan 13, 2026 •

Copy link
Copy Markdown
Collaborator
  • Assertions should account for layers split across PP ranks

Summary by CodeRabbit

  • Tests
    • Improved GPU test handling by capping usage to at most 2 GPUs.
    • Enhanced validation logic for distributed training scenarios with dynamic assertion handling.
    • Refined pruning score assertion checks to properly handle multi-GPU distributed validation paths.

✏️ Tip: You can customize this high-level summary in your review settings.

@codecov

codecov Bot commented Jan 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.64%. Comparing base (83ac3b1) to head (1b9d5cd).
⚠️ Report is 1 commits behind head on feature/compress.

Additional details and impacted files
@@                 Coverage Diff                  @@
##           feature/compress     #772      +/-   ##
====================================================
+ Coverage             74.62%   74.64%   +0.01%     
====================================================
  Files                   192      192              
  Lines                 18989    18989              
====================================================
+ Hits                  14171    14174       +3     
+ Misses                 4818     4815       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kevalmorabia97
kevalmorabia97 force-pushed the kmorabia/2-gpu-test-fix branch from e7cdb57 to 0c2a97a Compare January 13, 2026 13:26
@coderabbitai

coderabbitai Bot commented Jan 13, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This change modifies a GPU-accelerated torch compression test to cap GPU usage to at most 2 and restructures pruning score assertions. The validation logic is refactored to handle distributed scenarios by splitting assertion paths based on the distributed size and rank of processes.

Changes

Cohort / File(s) Summary
Distributed Compression Test Updates
tests/gpu/torch/_compress/test_compress.py
Caps GPU usage to 2 in tests. Restructures pruning score assertion validation to be size and rank-aware, introducing conditional paths for layer-specific validation. Moves assertion call to execute unconditionally post-compression while maintaining size-scaled assertion counts.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix test assertions for 2-gpu' clearly and concisely describes the main change: updating test assertions to handle 2-GPU scenarios where layers are distributed across pipeline-parallel ranks.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
@kevalmorabia97
kevalmorabia97 force-pushed the kmorabia/2-gpu-test-fix branch from 0c2a97a to 1b9d5cd Compare January 13, 2026 16:31
@kevalmorabia97
kevalmorabia97 merged commit 0eecfc6 into feature/compress Jan 13, 2026
33 of 35 checks passed
@kevalmorabia97
kevalmorabia97 deleted the kmorabia/2-gpu-test-fix branch January 13, 2026 18:20
h-guo18 added a commit that referenced this pull request Sep 21, 2026
Behaviour change, and the reason for the rest: the convolution's
kernel_projection and the selector's successor_codebook are now
zero-initialized, so a freshly built DFlash2 draft really is its DFlash
backbone bit-for-bit. The class docstring already claimed this while
_init_head_weights drew both from normal_(0, initializer_range), so the
claim was false and the identity test only passed because it zeroed the
projection by hand first. Zeroing matches the reference implementation
(SpecForge #772) and the way modeling_lilicorr installs this same class.
Both stay trainable: the conv's delta is added to a non-zero base kernel,
and successor_codebook is one factor of a bilinear form whose other two
factors -- predecessor_codebook and hidden_projection -- take one step to
start moving. That delay is now pinned by tests rather than discovered.

_convolve no longer forms the dense coefficient tensor. (base + delta) * x
is expanded as base * x + delta * x, which is mathematically identical but
never materializes [.., taps, groups, group_size] -- taps * hidden floats
per position, held by autograd until backward because it is a
multiplicand. At the shipped recipe that is ~1 GB per sample versus ~60 MB
for the delta it was built from.

The selector metrics leave _compute_loss as detached tensors instead of
.item() -- two CPU-GPU syncs per training step, which CONTRIBUTING
prohibits -- and are carried out on the forward output the way LiLiCorr,
DSpark and Domino do. Without that, selector_coverage, the only signal
that distinguishes a selector choosing from one handed the gold token,
never reached the logs.

Also: assert the published-contract fields (top-level is_causal, nested
block_size) the export test never covered; state the train/serve offset
contract where the objective and CandidateSelector.greedy_path meet, and
test it, since a misaligned objective still produces a finite decreasing
loss; drop the stale DSpark "Markov head" rationale from the recipe; and
correct the is_causal comment, whose premise #2149 removed by making
dflash_config.causal unconditional.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
h-guo18 added a commit that referenced this pull request Sep 22, 2026
Adds DFlash2 (https://inco.ai/blog/dflash2/) as a draft variant of the
existing DFlash mode, selected with
dflash_architecture_config.projector_type="dflash2" alongside domino,
dspark and lilicorr.

DFlash2 keeps DFlash's one-pass parallel backbone and adds two components
that recover the acceptance a purely parallel draft loses: a grouped
dynamic depthwise convolution around every attention and MLP sublayer,
giving each block position a view of its predecessors inside the block
without the taps crossing the block boundary; and a low-rank candidate
selector scoring transitions between adjacent positions' top-k
candidates, so serving walks one coherent path instead of taking an
independent argmax per position.

Both start as exact no-ops -- the convolution's base_kernel is an identity
and kernel_projection is zeroed, the selector's successor_codebook is
zeroed -- so a freshly built DFlash2 draft is its DFlash backbone, and
enabling the variant is an extension rather than a perturbation. This
matches the reference implementation (SpecForge #772) and the way
modeling_lilicorr installs the same convolution class.

This also unblocks a recipe already shipped on main:
modeling_lilicorr._install_sublayer_convs imports DFlashGroupedConv from
modeling_dflash2, so lilicorr_conv.yaml raises at model build today.
LiLiCorr's own initialization is unchanged and is now covered by tests --
it assigns kernel_projection explicitly, so it holds whichever way
DFlashGroupedConv initializes itself -- and the two texts on main that
described DFlash2's older random init are corrected.

Module and parameter names match the SGLang/vLLM DFlash2DraftModel
loaders, verified against the released z-lab/Qwen3.8-27B-DFlash2
checkpoint: 81 tensors, 21 name patterns, zero difference in either
direction. The serving side, vllm-project/vllm#52816, has merged with no
change to the checkpoint contract.

modeling_dflash2.py is adapted from sgl-project/SpecForge#772 and carries
its MIT notice ahead of the NVIDIA dual-SPDX header, matching
modeling_dflash.py. No new dependencies.

Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants