Skip to content

Reduce graph breaks and recompiles#547

Merged
fynnsu merged 5 commits into
mainfrom
maybe_fix_slice
Jun 18, 2026
Merged

Reduce graph breaks and recompiles#547
fynnsu merged 5 commits into
mainfrom
maybe_fix_slice

Conversation

@fynnsu

@fynnsu fynnsu commented May 26, 2026

Copy link
Copy Markdown
Collaborator

PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.

Purpose

Late in epoch graph recompiles can cause training to crash after a substantial number of batches have been processed. This pr works to decrease these occurrences and also reduce the number of graph breaks in the compiled code.

Related Issue

I think this should resolve #544.

Tests

Will add some tests to this pr.

I have filled in:

  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan/results, such as providing test command and pasting the results.
  • (Optional) The necessary documentation update.
  • I (a human) have written or reviewed the code in this pr to the best of my ability.

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b8ab62d1-8d20-43bf-9bfa-941b835c54e8

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

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR modifies the data collation pipeline to pad the lengths tensor to a minimum size of 4 elements, replacing direct tensor construction with a padded version using torch.nn.functional.pad.

Changes

Data Collation Tensor Padding

Layer / File(s) Summary
Lengths tensor minimum size padding
src/speculators/train/data.py
After computing new_lengths, the collated_data["lengths"] tensor is now explicitly created as torch.long and padded to ensure at least 4 elements, instead of using the unpadded tensor directly.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

bug, training

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The PR title 'Reduce graph breaks and recompiles' is vague and does not clearly reflect the actual change of padding the lengths tensor to minimum size 4. Consider revising the title to be more specific about the change, such as 'Pad lengths tensor to minimum size 4 to avoid specialization' or similar, to better reflect the actual code modification.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code change addresses issue #544 by padding lengths tensors to prevent inductor pattern-matching failures, aligning with the root cause mitigation objective.
Out of Scope Changes check ✅ Passed The change is narrowly scoped to the collation function and directly addresses the lengths tensor padding requirement from issue #544.
Description check ✅ Passed The pull request description clearly relates to the changeset. It explains the purpose (reducing late-epoch graph recompiles and graph breaks), links to a related issue, and describes the technical approach.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch maybe_fix_slice

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.

@coderabbitai coderabbitai Bot added bug Something isn't working training labels May 26, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/speculators/train/data.py (1)

469-472: ⚡ Quick win

Convert length tensor scalars to Python ints before rebuilding lengths.

In src/speculators/train/data.py (create_collate_fn), iterating for length in lengths: yields 0-d tensor scalars; mixing them with cum_length (Python int) and collecting them into new_lengths makes torch.tensor(new_lengths, ...) less idiomatic/stable. Convert with length.item() and keep cum_length/the condition as pure ints.

Proposed diff
         lengths = collated_data["lengths"]
         new_lengths = []
         cum_length = 0
         for length in lengths:
-            if length + cum_length >= max_len:
+            length_i = int(length.item())
+            if length_i + cum_length >= max_len:
                 new_lengths.append(max_len - cum_length)
                 break
-            new_lengths.append(length)
-            cum_length += length
+            new_lengths.append(length_i)
+            cum_length += length_i
         lengths = torch.tensor(new_lengths, dtype=torch.long)
         # Pad to at least size 4 to avoid torch.compile recompiling on sizes 0/1
         pad_to = max(4, lengths.shape[0])
         collated_data["lengths"] = F.pad(lengths, (0, pad_to - lengths.shape[0]))
🤖 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 `@src/speculators/train/data.py` around lines 469 - 472, In create_collate_fn,
when iterating the tensor variable lengths you should convert each 0-dim tensor
to a Python int (use length.item()) before comparing/adding to cum_length and
appending to new_lengths; ensure cum_length stays an int and the condition uses
ints so new_lengths is a list of ints, then rebuild lengths with
torch.tensor(new_lengths, dtype=torch.long) and apply the existing padding logic
(pad_to, F.pad) as before.
🤖 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 `@src/speculators/train/data.py`:
- Around line 469-472: In create_collate_fn, when iterating the tensor variable
lengths you should convert each 0-dim tensor to a Python int (use length.item())
before comparing/adding to cum_length and appending to new_lengths; ensure
cum_length stays an int and the condition uses ints so new_lengths is a list of
ints, then rebuild lengths with torch.tensor(new_lengths, dtype=torch.long) and
apply the existing padding logic (pad_to, F.pad) as before.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d4014969-132a-4a0f-b139-3ddfff652ab0

📥 Commits

Reviewing files that changed from the base of the PR and between 1bc3788 and 9ac1d14.

📒 Files selected for processing (1)
  • src/speculators/train/data.py

@mergify

mergify Bot commented May 26, 2026

Copy link
Copy Markdown

The quality checks have failed. Please run make style and make quality under
the root directory to address the lint failures. You will need to install the
dev optional install to get the required linting packages:
https://github.com/vllm-project/speculators/blob/main/CONTRIBUTING.md

@fynnsu fynnsu force-pushed the maybe_fix_slice branch from 4221bdc to 49e7a8b Compare May 26, 2026 20:36
@fynnsu fynnsu changed the title Apply minimal padding to lengths tensor Reduce graph breaks and recompiles May 26, 2026
@mergify mergify Bot removed the quality-failed label May 26, 2026
@fynnsu fynnsu force-pushed the maybe_fix_slice branch from 0d4a104 to f2e77e9 Compare May 28, 2026 15:09
@fynnsu

fynnsu commented May 28, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased

@mergify

mergify Bot commented May 30, 2026

Copy link
Copy Markdown

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @fynnsu.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label May 30, 2026
Sawyer117 added a commit to Sawyer117/speculators that referenced this pull request Jun 9, 2026
…flex_attention (vllm-project#531)

Walk back the premature 'verified end-to-end' claim: training never completes a
step on NPU. Document the real blocker found by checking the repo issues:

- DFlash (and EAGLE3/PEAGLE) force simple_flex_attention; PyTorch flex_attention
  rejects npu devices (issue vllm-project#531, open). Only MTP uses eager attention.
- @torch.compile on the dflash forward fails triton-ascend codegen with
  block_size=16 (NPU flavor of vllm-project#544 / PR vllm-project#547); TORCHDYNAMO_DISABLE=1 gets past
  it but then hits vllm-project#531.
- add §9 with the algorithm/attention support table and options (train MTP, patch
  DFlash to eager/dense-mask attention, or track upstream).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sawyer117 added a commit to Sawyer117/speculators that referenced this pull request Jun 15, 2026
…flex_attention (vllm-project#531)

Walk back the premature 'verified end-to-end' claim: training never completes a
step on NPU. Document the real blocker found by checking the repo issues:

- DFlash (and EAGLE3/PEAGLE) force simple_flex_attention; PyTorch flex_attention
  rejects npu devices (issue vllm-project#531, open). Only MTP uses eager attention.
- @torch.compile on the dflash forward fails triton-ascend codegen with
  block_size=16 (NPU flavor of vllm-project#544 / PR vllm-project#547); TORCHDYNAMO_DISABLE=1 gets past
  it but then hits vllm-project#531.
- add §9 with the algorithm/attention support table and options (train MTP, patch
  DFlash to eager/dense-mask attention, or track upstream).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fynnsu fynnsu force-pushed the maybe_fix_slice branch from f2e77e9 to 022c36e Compare June 17, 2026 19:48
fynnsu added 4 commits June 17, 2026 19:49
Signed-off-by: Fynn Schmitt-Ulms <fschmitt@redhat.com>
Mark lengths dynamic to avoid recompiling on it

Signed-off-by: Fynn Schmitt-Ulms <fschmitt@redhat.com>
Signed-off-by: Fynn Schmitt-Ulms <fschmitt@redhat.com>
Signed-off-by: Fynn Schmitt-Ulms <fschmitt@redhat.com>
@fynnsu fynnsu force-pushed the maybe_fix_slice branch from 022c36e to 75249e2 Compare June 17, 2026 19:52
Signed-off-by: Fynn Schmitt-Ulms <fschmitt@redhat.com>
@fynnsu fynnsu force-pushed the maybe_fix_slice branch from 75249e2 to fa47379 Compare June 17, 2026 19:52
@fynnsu fynnsu added the ready This PR is ready for review label Jun 17, 2026
@fynnsu

fynnsu commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator Author

E2E (vllm@nightly, transformers<5.0, this branch): https://github.com/neuralmagic/llm-compressor-testing/actions/runs/27716366270/job/81990214563

@mergify mergify Bot removed the needs-rebase label Jun 17, 2026
Comment thread src/speculators/train/data.py

@shanjiaz shanjiaz 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.

Nice!

@fynnsu fynnsu merged commit dbd631b into main Jun 18, 2026
17 of 18 checks passed
@fynnsu fynnsu deleted the maybe_fix_slice branch June 18, 2026 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready This PR is ready for review training

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: qwen3-8b dflash

2 participants