Skip to content

[SPEC] feat: init adaptive spec params from config#27493

Open
alphabetc1 wants to merge 3 commits into
sgl-project:mainfrom
alphabetc1:feat/adaptive-spec-param
Open

[SPEC] feat: init adaptive spec params from config#27493
alphabetc1 wants to merge 3 commits into
sgl-project:mainfrom
alphabetc1:feat/adaptive-spec-param

Conversation

@alphabetc1
Copy link
Copy Markdown
Collaborator

@alphabetc1 alphabetc1 commented Jun 7, 2026

Summary

  • initialize adaptive speculative params from config before algorithm-specific handling
  • keep unsupported adaptive configs on the existing warning-and-disable fallback path
  • move speculative auto-default selection into the speculative hook

Tests

  • /home/wangshuwen.wsw/code/py310-venv/bin/python3.10 -m py_compile python/sglang/srt/arg_groups/speculative_hook.py python/sglang/srt/speculative/adaptive_spec_params.py test/registered/unit/server_args/test_server_args.py test/registered/unit/spec/test_adaptive_spec_params.py test/registered/spec/eagle/test_adaptive_speculative.py

CI States

Latest PR Test (Base): ❌ Run #27092161683
Latest PR Test (Extra): ❌ Run #27092161562

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

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 refactors the initialization and validation of speculative decoding parameters, particularly for adaptive speculative decoding. It moves adaptive parameter setup earlier in the process, introduces helper functions _init_adaptive_speculative_params and _auto_choose_speculative_params within speculative_hook.py, and removes redundant code from server_args.py and adaptive_spec_params.py. Feedback on the changes highlights a missing import of SimpleNamespace in the newly added unit test, which will cause a NameError when executed.

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 on lines +526 to +527
def test_adaptive_defaults_to_config_step_when_spec_params_omitted(self):
with tempfile.NamedTemporaryFile("w", suffix=".json") as f:
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.

medium

The test uses SimpleNamespace to mock the model configuration, but SimpleNamespace is not imported in this file. This will cause a NameError when the test is executed. Please import SimpleNamespace from types.

    def test_adaptive_defaults_to_config_step_when_spec_params_omitted(self):
        from types import SimpleNamespace
        with tempfile.NamedTemporaryFile("w", suffix=".json") as f:

@alphabetc1
Copy link
Copy Markdown
Collaborator Author

/rerun-test test_adaptive_speculative.py test_adaptive_spec_params.py

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

Results for /rerun-test test_adaptive_speculative.py test_adaptive_spec_params.py:

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

cd test/ && python3 registered/spec/eagle/test_adaptive_speculative.py

🚀 ubuntu-latest (1 test): ✅ View workflow run

cd test/ && python3 registered/unit/spec/test_adaptive_spec_params.py

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79a6e3600d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

args.speculative_adaptive = True
args.speculative_adaptive_config = f.name
args.device = "cuda"
args.get_model_config = lambda: SimpleNamespace(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Import SimpleNamespace before using it

This new test uses SimpleNamespace but the module never imports it, so when this test runs with the normal test dependencies installed it raises NameError while constructing args.get_model_config and never exercises handle_speculative_decoding. Add from types import SimpleNamespace (as done in other tests) so the adaptive-default coverage can pass.

Useful? React with 👍 / 👎.

@alphabetc1
Copy link
Copy Markdown
Collaborator Author

/tag-and-rerun-ci

@github-actions github-actions Bot added the run-ci label Jun 7, 2026
@alphabetc1 alphabetc1 force-pushed the feat/adaptive-spec-param branch 2 times, most recently from f326e2f to 79a6e36 Compare June 7, 2026 10:08
@alphabetc1
Copy link
Copy Markdown
Collaborator Author

/rerun-test test_adaptive_speculative.py test_adaptive_spec_params.py

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

Results for /rerun-test test_adaptive_speculative.py test_adaptive_spec_params.py:

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

cd test/ && python3 registered/spec/eagle/test_adaptive_speculative.py

🚀 ubuntu-latest (1 test): ✅ View workflow run

cd test/ && python3 registered/unit/spec/test_adaptive_spec_params.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant