refactor: move training hyperparameters from model configs to trainer kwargs by orestis-z · Pull Request #707 · vllm-project/speculators

orestis-z · 2026-07-02T12:11:52Z

Summary

Follow-up to #687 as discussed in review comments:

Move training-only hyperparameters out of model configs into get_trainer_kwargs → forward(). These fields don't affect model architecture or inference behavior and shouldn't be persisted in config.json.

DFlash/DSpark:

max_anchors — anchor sampling count, training-only
sliding_window_non_causal — training attention mask flag (vLLM uses its own attention path)

PEagle:

num_depths, down_sample_ratio, down_sample_ratio_min — COD sampling params, training-only
Inference-time speculation count remains in SpeculatorsConfig.proposal_methods[0].speculative_tokens

Not breaking: SpeculatorModelConfig uses extra="allow", so old checkpoints with these fields load fine.

Test plan

pytest tests/integration/models/test_model_forward.py -k dflash — DFlash param tests
pytest tests/integration/models/test_model_forward.py -k peagle — PEagle param tests
pytest tests/integration/models/test_model_forward.py — all model forward tests (no regressions)
Existing checkpoints load without error (removed fields silently accepted by extra="allow")

🤖 Generated with Claude Code

coderabbitai · 2026-07-02T12:13:24Z

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: 2539896d-f9ea-4b4a-9168-3fa83cb1ac52

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

[!WARNING]

Walkthrough skipped

File diffs could not be summarized.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch dflash-max-anchors-to-trainer-kwargs

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.}

mergify · 2026-07-02T14:18:37Z

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

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

fynnsu

Thanks for fixing this!

max_anchors is a training hyperparameter, not a model architecture parameter, so it should not be persisted in the model config. This follows the same pattern applied to peagle in PR #687. - Remove max_anchors field from DFlashSpeculatorConfig - Pass max_anchors through get_trainer_kwargs → forward for both DFlash and DSpark - Update tests to pass max_anchors as a forward kwarg Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

num_depths, down_sample_ratio, and down_sample_ratio_min are training hyperparameters (COD sampling behavior), not model architecture parameters. Move them from PEagleSpeculatorConfig to get_trainer_kwargs → forward(), following the same pattern as max_anchors for DFlash. The inference-time speculation count remains in SpeculatorsConfig.proposal_methods[0].speculative_tokens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

sliding_window_non_causal only affects the training attention mask; vLLM uses its own attention path at inference. Move it from DFlashSpeculatorConfig to get_trainer_kwargs → forward() for both DFlash and DSpark. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

…wargs" This reverts commit 058e920. Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

mergify · 2026-07-03T12:34:00Z

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

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

github-actions · 2026-07-03T12:40:07Z

~~Link Check Results (DOCS)~~

All links are now valid - this issue has been resolved.

Marked as resolved: 147abf9

github-actions · 2026-07-03T12:41:24Z

~~Link Check Results (REPO)~~

All links are now valid - this issue has been resolved.

Marked as resolved: 147abf9

Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

Resolve conflicts from vllm-project#707 (max_anchors moved to trainer kwargs): - core.py: combine global_step (domino) + max_anchors (main) in forward() - conftest.py: keep projector_type param, drop max_anchors param - test_model_forward.py: add max_anchors forward_kwargs to both specs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

shift_targets=True creates an off-by-one mismatch between training (position p predicts token p+1) and vLLM inference (position p drafts token p). This degrades acceptance length by -46% vs DFlash baseline. Fix: - Set shift_targets=False to align training with inference - Always include anchor positions in Domino loss mask (decoupled from shift_targets) - Use unshifted suffix_start unconditionally - Derive num_anchors from tensor shape (config.max_anchors removed in vllm-project#707) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

… kwargs (vllm-project#707) Follow-up to vllm-project#687 as discussed in [review comments](vllm-project#687 (comment)): Move training-only hyperparameters out of model configs into `get_trainer_kwargs` → `forward()`. These fields don't affect model architecture or inference behavior and shouldn't be persisted in `config.json`. **DFlash/DSpark:** - `max_anchors` — anchor sampling count, training-only - `sliding_window_non_causal` — training attention mask flag (vLLM uses its own attention path) **PEagle:** - `num_depths`, `down_sample_ratio`, `down_sample_ratio_min` — COD sampling params, training-only - Inference-time speculation count remains in `SpeculatorsConfig.proposal_methods[0].speculative_tokens` Not breaking: `SpeculatorModelConfig` uses `extra="allow"`, so old checkpoints with these fields load fine. - [ ] `pytest tests/integration/models/test_model_forward.py -k dflash` — DFlash param tests - [ ] `pytest tests/integration/models/test_model_forward.py -k peagle` — PEagle param tests - [ ] `pytest tests/integration/models/test_model_forward.py` — all model forward tests (no regressions) - [ ] Existing checkpoints load without error (removed fields silently accepted by `extra="allow"`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

shift_targets=True creates an off-by-one mismatch between training (position p predicts token p+1) and vLLM inference (position p drafts token p). This degrades acceptance length by -46% vs DFlash baseline. Fix: - Set shift_targets=False to align training with inference - Always include anchor positions in Domino loss mask (decoupled from shift_targets) - Use unshifted suffix_start unconditionally - Derive num_anchors from tensor shape (config.max_anchors removed in vllm-project#707) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

… kwargs (vllm-project#707) Follow-up to vllm-project#687 as discussed in [review comments](vllm-project#687 (comment)): Move training-only hyperparameters out of model configs into `get_trainer_kwargs` → `forward()`. These fields don't affect model architecture or inference behavior and shouldn't be persisted in `config.json`. **DFlash/DSpark:** - `max_anchors` — anchor sampling count, training-only - `sliding_window_non_causal` — training attention mask flag (vLLM uses its own attention path) **PEagle:** - `num_depths`, `down_sample_ratio`, `down_sample_ratio_min` — COD sampling params, training-only - Inference-time speculation count remains in `SpeculatorsConfig.proposal_methods[0].speculative_tokens` Not breaking: `SpeculatorModelConfig` uses `extra="allow"`, so old checkpoints with these fields load fine. - [ ] `pytest tests/integration/models/test_model_forward.py -k dflash` — DFlash param tests - [ ] `pytest tests/integration/models/test_model_forward.py -k peagle` — PEagle param tests - [ ] `pytest tests/integration/models/test_model_forward.py` — all model forward tests (no regressions) - [ ] Existing checkpoints load without error (removed fields silently accepted by `extra="allow"`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

shift_targets=True creates an off-by-one mismatch between training (position p predicts token p+1) and vLLM inference (position p drafts token p). This degrades acceptance length by -46% vs DFlash baseline. Fix: - Set shift_targets=False to align training with inference - Always include anchor positions in Domino loss mask (decoupled from shift_targets) - Use unshifted suffix_start unconditionally - Derive num_anchors from tensor shape (config.max_anchors removed in vllm-project#707) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

… kwargs (vllm-project#707) Follow-up to vllm-project#687 as discussed in [review comments](vllm-project#687 (comment)): Move training-only hyperparameters out of model configs into `get_trainer_kwargs` → `forward()`. These fields don't affect model architecture or inference behavior and shouldn't be persisted in `config.json`. **DFlash/DSpark:** - `max_anchors` — anchor sampling count, training-only - `sliding_window_non_causal` — training attention mask flag (vLLM uses its own attention path) **PEagle:** - `num_depths`, `down_sample_ratio`, `down_sample_ratio_min` — COD sampling params, training-only - Inference-time speculation count remains in `SpeculatorsConfig.proposal_methods[0].speculative_tokens` Not breaking: `SpeculatorModelConfig` uses `extra="allow"`, so old checkpoints with these fields load fine. - [ ] `pytest tests/integration/models/test_model_forward.py -k dflash` — DFlash param tests - [ ] `pytest tests/integration/models/test_model_forward.py -k peagle` — PEagle param tests - [ ] `pytest tests/integration/models/test_model_forward.py` — all model forward tests (no regressions) - [ ] Existing checkpoints load without error (removed fields silently accepted by `extra="allow"`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

shift_targets=True creates an off-by-one mismatch between training (position p predicts token p+1) and vLLM inference (position p drafts token p). This degrades acceptance length by -46% vs DFlash baseline. Fix: - Set shift_targets=False to align training with inference - Always include anchor positions in Domino loss mask (decoupled from shift_targets) - Use unshifted suffix_start unconditionally - Derive num_anchors from tensor shape (config.max_anchors removed in vllm-project#707) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

- Add max_anchors=8 to all _backbone_forward() and model() calls in unit tests (PR vllm-project#707 removed max_anchors from config, so _backbone_forward defaults to 3072, causing CUDA OOM) - Fix lambda_base decay: decay_steps=0 means no decay (stay at lambda_base_start), not immediate drop to 0.0 Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

… kwargs (vllm-project#707) Follow-up to vllm-project#687 as discussed in [review comments](vllm-project#687 (comment)): Move training-only hyperparameters out of model configs into `get_trainer_kwargs` → `forward()`. These fields don't affect model architecture or inference behavior and shouldn't be persisted in `config.json`. **DFlash/DSpark:** - `max_anchors` — anchor sampling count, training-only - `sliding_window_non_causal` — training attention mask flag (vLLM uses its own attention path) **PEagle:** - `num_depths`, `down_sample_ratio`, `down_sample_ratio_min` — COD sampling params, training-only - Inference-time speculation count remains in `SpeculatorsConfig.proposal_methods[0].speculative_tokens` Not breaking: `SpeculatorModelConfig` uses `extra="allow"`, so old checkpoints with these fields load fine. - [ ] `pytest tests/integration/models/test_model_forward.py -k dflash` — DFlash param tests - [ ] `pytest tests/integration/models/test_model_forward.py -k peagle` — PEagle param tests - [ ] `pytest tests/integration/models/test_model_forward.py` — all model forward tests (no regressions) - [ ] Existing checkpoints load without error (removed fields silently accepted by `extra="allow"`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

shift_targets=True creates an off-by-one mismatch between training (position p predicts token p+1) and vLLM inference (position p drafts token p). This degrades acceptance length by -46% vs DFlash baseline. Fix: - Set shift_targets=False to align training with inference - Always include anchor positions in Domino loss mask (decoupled from shift_targets) - Use unshifted suffix_start unconditionally - Derive num_anchors from tensor shape (config.max_anchors removed in vllm-project#707) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

- Add max_anchors=8 to all _backbone_forward() and model() calls in unit tests (PR vllm-project#707 removed max_anchors from config, so _backbone_forward defaults to 3072, causing CUDA OOM) - Fix lambda_base decay: decay_steps=0 means no decay (stay at lambda_base_start), not immediate drop to 0.0 Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

… kwargs (vllm-project#707) Follow-up to vllm-project#687 as discussed in [review comments](vllm-project#687 (comment)): Move training-only hyperparameters out of model configs into `get_trainer_kwargs` → `forward()`. These fields don't affect model architecture or inference behavior and shouldn't be persisted in `config.json`. **DFlash/DSpark:** - `max_anchors` — anchor sampling count, training-only - `sliding_window_non_causal` — training attention mask flag (vLLM uses its own attention path) **PEagle:** - `num_depths`, `down_sample_ratio`, `down_sample_ratio_min` — COD sampling params, training-only - Inference-time speculation count remains in `SpeculatorsConfig.proposal_methods[0].speculative_tokens` Not breaking: `SpeculatorModelConfig` uses `extra="allow"`, so old checkpoints with these fields load fine. - [ ] `pytest tests/integration/models/test_model_forward.py -k dflash` — DFlash param tests - [ ] `pytest tests/integration/models/test_model_forward.py -k peagle` — PEagle param tests - [ ] `pytest tests/integration/models/test_model_forward.py` — all model forward tests (no regressions) - [ ] Existing checkpoints load without error (removed fields silently accepted by `extra="allow"`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

shift_targets=True creates an off-by-one mismatch between training (position p predicts token p+1) and vLLM inference (position p drafts token p). This degrades acceptance length by -46% vs DFlash baseline. Fix: - Set shift_targets=False to align training with inference - Always include anchor positions in Domino loss mask (decoupled from shift_targets) - Use unshifted suffix_start unconditionally - Derive num_anchors from tensor shape (config.max_anchors removed in vllm-project#707) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

- Add max_anchors=8 to all _backbone_forward() and model() calls in unit tests (PR vllm-project#707 removed max_anchors from config, so _backbone_forward defaults to 3072, causing CUDA OOM) - Fix lambda_base decay: decay_steps=0 means no decay (stay at lambda_base_start), not immediate drop to 0.0 Signed-off-by: Eros483 <arnabmandal2912@gmail.com>

orestis-z self-assigned this Jul 2, 2026

orestis-z marked this pull request as draft July 2, 2026 12:24

orestis-z changed the title ~~refactor(dflash): move max_anchors from model config to trainer kwargs~~ refactor: move training hyperparameters from model config to trainer kwargs Jul 2, 2026

orestis-z changed the title ~~refactor: move training hyperparameters from model config to trainer kwargs~~ refactor: move training hyperparameters from model configs to trainer kwargs Jul 2, 2026

orestis-z marked this pull request as ready for review July 2, 2026 12:38

orestis-z requested review from fynnsu and shanjiaz July 2, 2026 12:38

mergify Bot added the needs-rebase label Jul 2, 2026

orestis-z force-pushed the dflash-max-anchors-to-trainer-kwargs branch from 4974e9b to 8bd9f72 Compare July 2, 2026 14:27

mergify Bot removed the needs-rebase label Jul 2, 2026

fynnsu approved these changes Jul 2, 2026

View reviewed changes

Comment thread src/speculators/models/dflash/config.py

orestis-z force-pushed the dflash-max-anchors-to-trainer-kwargs branch 2 times, most recently from c0675ac to 058e920 Compare July 3, 2026 11:36

orestis-z and others added 4 commits July 3, 2026 11:39

Revert "refactor(dflash): move sliding_window_non_causal to trainer k…

b384dc8

…wargs" This reverts commit 058e920. Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

orestis-z force-pushed the dflash-max-anchors-to-trainer-kwargs branch from bfcbcae to b384dc8 Compare July 3, 2026 11:39

orestis-z enabled auto-merge (squash) July 3, 2026 11:39

orestis-z added the ready This PR is ready for review label Jul 3, 2026

Merge branch 'main' into dflash-max-anchors-to-trainer-kwargs

b99a6b4

mergify Bot added the quality-failed label Jul 3, 2026

fix(test): pass max_anchors as forward kwarg in vllm parity test

0627801

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

mergify Bot removed the quality-failed label Jul 3, 2026

Merge branch 'main' into dflash-max-anchors-to-trainer-kwargs

48afc60

orestis-z added 2 commits July 3, 2026 13:11

chore: retrigger CI

3439407

Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

chore: retrigger CI

147abf9

Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>

orestis-z merged commit 15297c2 into main Jul 3, 2026
9 checks passed

orestis-z deleted the dflash-max-anchors-to-trainer-kwargs branch July 3, 2026 13:36

orestis-z mentioned this pull request Jul 4, 2026

fix: decouple shift_targets from Domino loss mask Eros483/speculators#1

Merged

heiheiha798 mentioned this pull request Jul 18, 2026

[review] PR #811 bounded async hidden-state fan-out — multi-agent review report (request-changes) heiheiha798/speculators#1

Open

Navigation Menu

Uh oh!

refactor: move training hyperparameters from model configs to trainer kwargs - #707

refactor: move training hyperparameters from model configs to trainer kwargs#707
orestis-z merged 9 commits into
mainfrom
dflash-max-anchors-to-trainer-kwargs

orestis-z commented Jul 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

Review skipped

Walkthrough skipped

Uh oh!

mergify Bot commented Jul 2, 2026

Uh oh!

fynnsu left a comment

Uh oh!

Uh oh!

mergify Bot commented Jul 3, 2026

Uh oh!

github-actions Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

orestis-z commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough skipped

Uh oh!

mergify Bot commented Jul 2, 2026

Uh oh!

fynnsu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify Bot commented Jul 3, 2026

Uh oh!

github-actions Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

orestis-z commented Jul 2, 2026 •

edited

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

github-actions Bot commented Jul 3, 2026 •

edited

Loading

github-actions Bot commented Jul 3, 2026 •

edited

Loading