Skip to content

[Spec] Harden multi-layer EAGLE grammar verification - #30155

Open
shanemort1982 wants to merge 1 commit into
sgl-project:mainfrom
shanemort1982:fix-multilayer-eagle-grammar-vocab-mask
Open

shanemort1982 wants to merge 1 commit into
sgl-project:mainfrom
shanemort1982:fix-multilayer-eagle-grammar-vocab-mask

Conversation

@shanemort1982

@shanemort1982 shanemort1982 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Motivation

The original PR identified a real bug: multi-layer EAGLE could sample target tokens without applying the grammar vocabulary mask. Since this PR was opened, #31380 consolidated single- and multi-layer verification into eagle_worker_common.run_eagle_verify, so current main now contains the missing mask flow.

That shared implementation still reshapes verify_input.draft_token with .view(...). A non-contiguous tensor can make .view() raise before the grammar mask is built. Current main also has no end-to-end constrained-decoding coverage that launches the multi-layer worker, so the shared-path wiring could regress unnoticed.

Modifications

  • Use .reshape(...) in the shared run_eagle_verify grammar snapshot so both contiguous and non-contiguous draft-token tensors are handled safely.
  • Add test_multi_layer_eagle_constrained_decoding.py, exercising JSON and regex constraints with --enable-multi-layer-eagle.
  • Cover both overlap and non-overlap scheduling.

The duplicate worker-local mask implementation from the original revision is intentionally gone; current main's shared verify path is the source of truth.

Accuracy Tests

The GPU integration test mirrors the existing single-layer EAGLE constrained-decoding suite and fails if target verification admits a grammar-illegal token. It is registered for base-b on 1-gpu-large and requires a maintainer CI trigger.

Original production evidence that motivated the regression coverage (MiMo-V2.5, 8xB300, tp4, multi-layer EAGLE 3-1-4):

Scenario Before grammar mask With grammar mask
Forced tool choice 0/6 8/8
Automatic tool choice under load failed 8/8

Local verification

  • python -m py_compile on both changed files
  • python -m black --check on both changed files
  • git diff --check

The GPU integration suite cannot run on the Windows authoring host and is left to upstream CI.

Checklist

  • Rebased onto current main
  • Formatting and syntax checks
  • GPU regression test registered
  • Maintainer-triggered GPU CI
  • No documentation change required (correctness hardening of an existing path)

CI States

Latest PR Test (Base): ❌ Run #29835815371
Latest PR Test (Extra): ❌ Run #29835815568

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request adds support for grammar-constrained decoding in the multi-layer EAGLE verification path by generating and applying a vocabulary mask during sampling, preventing the target sampler from accepting grammar-illegal tokens. It also introduces integration tests to guard against regressions. The review feedback recommends using .reshape() instead of .view() when preparing draft tokens on the CPU to prevent potential runtime errors with non-contiguous tensors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread python/sglang/srt/speculative/multi_layer_eagle_worker_v2.py Outdated
@shanemort1982

Copy link
Copy Markdown
Contributor Author

Codex review against current upstream main (2026-07-21): the correctness issue remains real; current main still does not apply the xgrammar vocabulary mask in this multi-layer EAGLE path, so this PR has not been superseded. The overlap/non-overlap integration coverage is valuable.

Two things need resolving before merge: (1) rebase/reapply the mask at the current verify/sampling seam because the worker has been substantially refactored upstream, and (2) replace .view(...) with .reshape(...) for the potentially non-contiguous expanded tensor, as the existing bot review noted. Then run the relevant GPU CI. Good fix, but the current conflicted head is not ready to merge unchanged.

@shanemort1982
shanemort1982 force-pushed the fix-multilayer-eagle-grammar-vocab-mask branch from c6eb56e to dee7b7b Compare July 21, 2026 13:43
@shanemort1982
shanemort1982 force-pushed the fix-multilayer-eagle-grammar-vocab-mask branch from dee7b7b to 6d95fe4 Compare July 21, 2026 13:44
@shanemort1982 shanemort1982 changed the title Fix xgrammar vocab-mask on the multi-layer EAGLE verify path [Spec] Harden multi-layer EAGLE grammar verification Jul 21, 2026
@shanemort1982

shanemort1982 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Codex follow-up ? rebased and updated in 6d95fe417.

Current main absorbed the original missing-mask implementation through #31380's shared run_eagle_verify, so the obsolete worker-local port was removed. The remaining real finding is fixed in that shared path: .view(...) is now .reshape(...), preventing a non-contiguous draft-token tensor from aborting grammar-mask generation.

The PR also adds end-to-end multi-layer EAGLE constrained-decoding coverage for JSON + regex in overlap and non-overlap modes. The title/body now describe the current patch and the stale review thread is resolved.

Verified: GitHub lint/pre-commit is green and the PR is mergeable. py_compile, Black, and git diff --check pass locally. The registered base-b GPU test still needs a maintainer CI trigger.

@shanemort1982

Copy link
Copy Markdown
Contributor Author

CI follow-up: head 6d95fe417 is mergeable, satisfies the mandatory base, and lint/pre-commit is green. The red platform/aggregate jobs are solely the missing run-ci gate. @Qiaolin-Yu or @hnyls2002, could one of you apply run-ci so the registered base-b multi-layer EAGLE constrained-decoding test can execute?

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.

1 participant