Skip to content

[DFlash2] Keep the greedy selector walk in range for all-NaN score rows - #41737

Merged
kpham-sgl merged 8 commits into
sgl-project:mainfrom
modal-projects:rohan/up/dflash2-all-nan-selector-row
Oct 5, 2026
Merged

kpham-sgl merged 8 commits into
sgl-project:mainfrom
modal-projects:rohan/up/dflash2-all-nan-selector-row

Conversation

@rodamani

@rodamani rodamani commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

The greedy branch of _selector_walk_kernel (kernels/ops/speculative/dflash.py) takes best = tl.max(scores) and then the first offset where scores == best. If a whole score row is NaN, best is NaN, no element compares equal, and index falls through to top_k: one past the row, so the walk resolves to a neighbouring row's candidates.

Modifications

  • Greedy branch of _selector_walk_kernel now makes the same pick as torch.argmax (and _selector_walk_torch): the first NaN if the row has one, else the first maximum. index < top_k for every row, including all-NaN rows, and mixed-NaN rows match the reference.
  • test/registered/kernels/ops/speculative/test_lilicorr_cuda.py: test_greedy_path_matches_the_reference_on_nan_scores (all-NaN and mixed-NaN rows), comparing tokens and q rows from lilicorr_sample_path against _selector_walk_torch.

DSpark

DFlash-only: DSparkWorkerV2 does not use candidate_selector / selector_walk_triton, so DSpark is unaffected.

Accuracy Tests

CUDA test (test_lilicorr_cuda.py, registered on 1-gpu-large); results from CI. The kernel was also checked against _selector_walk_torch under TRITON_INTERPRET=1 for all-NaN, mixed-NaN, finite, tied and -inf rows (all match).

Speed Tests and Profiling

No hot-path change beyond the fix itself; not separately benchmarked.

Checklist

Review and Merge Process

  1. Ping Merge Oncalls to start the process. See the PR Merge Process.
  2. Get approvals from CODEOWNERS and other reviewers.
  3. Trigger CI tests with comments or contact authorized users to do so.
    • Common commands include /tag-and-rerun-ci, /tag-run-ci-label, /rerun-failed-ci
  4. After green CI and required approvals, ask Merge Oncalls or people with Write permission to merge the PR.

CI States

Latest PR Test (Base): ❌ Run #37372296257
Latest PR Test (Extra): ❌ Run #37372295607
Latest PR Test (AMD ROCm 10): ❌ Run #37372296288

…n all-NaN score row stays in range (sgl-project#105)

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@rodamani
rodamani marked this pull request as ready for review September 29, 2026 21:15
@rodamani

Copy link
Copy Markdown
Contributor Author

/rerun-test -c test_basic_sanity_dflash.py

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Results for /rerun-test -c test_basic_sanity_dflash.py:

🚀 1-gpu-5090 (1 test): ✅ View workflow run

cd test/ && python3 registered/core/test_basic_sanity_dflash.py

⛔ test/registered/unit/kernels/test_dflash_selector_walk_nan_row.py: File not found: test/registered/unit/kernels/test_dflash_selector_walk_nan_row.py

@rodamani

Copy link
Copy Markdown
Contributor Author

/tag-and-rerun-ci

@github-actions github-actions Bot added the run-ci CI: run the baseline test suite on this PR label Sep 30, 2026
rodamani and others added 4 commits September 30, 2026 01:41
…riton interpreter

Setting TRITON_INTERPRET inside the test module fails in CI's CPU suite once
triton is already imported (tl.sum raises 'Cannot call @triton.jit'd outside
of the scope of a kernel'). Move it next to the other speculative kernel tests
and register it for base-b-kernel-unit on 1-gpu-large.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@kpham-sgl kpham-sgl 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.

Fix is real; comments inline. Nit: the description's test path is wrong, and the CPU pass/fail numbers can't be right (test is CUDA-only).

Comment thread python/sglang/kernels/ops/speculative/dflash.py Outdated
Comment thread test/registered/kernels/ops/speculative/test_dflash_selector_walk_nan_row.py Outdated
Pick the first NaN if the row has one, else the first maximum, so mixed-NaN
rows agree with _selector_walk_torch. Move NaN coverage into
test_lilicorr_cuda.py as reference comparisons.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@rodamani

rodamani commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@kpham-sgl comments addressed here!

@kpham-sgl

Copy link
Copy Markdown
Collaborator

/rerun-test test/registered/kernels/ops/speculative/test_lilicorr_cuda.py test/registered/kernels/ops/speculative/test_dflash_domino.py test/registered/e2e/models/test_glm53_flash_b200.py

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

⚠️ Rebase Required Before Re-run

A major update has landed on main. Your PR is diverged relative to required base commit fea3c088a141.

Re-run was not dispatched. What to do:

  • Rebase your branch onto the latest main and push again
  • Follow issue #21065 for context
  • CI-fix PRs may request the bypass-maintenance label to skip this check

@rodamani

rodamani commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

/rerun-test test/registered/kernels/ops/speculative/test_lilicorr_cuda.py test/registered/kernels/ops/speculative/test_dflash_domino.py test/registered/e2e/models/test_glm53_flash_b200.py

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

🚀 1-gpu-h100 (2 tests): ✅ View workflow run

cd test/ && python3 registered/kernels/ops/speculative/test_lilicorr_cuda.py
cd test/ && python3 registered/kernels/ops/speculative/test_dflash_domino.py

🚀 4-gpu-b200 (1 test): ✅ View workflow run

cd test/ && python3 registered/e2e/models/test_glm53_flash_b200.py

@kpham-sgl
kpham-sgl merged commit c765f88 into sgl-project:main Oct 5, 2026
106 of 123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants