Skip to content

Conversation

yyzxw
Copy link
Contributor

@yyzxw yyzxw commented Sep 18, 2025

Purpose

fix #24888

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@yyzxw yyzxw force-pushed the fix/speculative-config-parse-error branch from 2b5ca2a to a98c9aa Compare September 18, 2025 06:44
Copy link
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 addresses an error in parsing SpeculativeConfig by refactoring how the --speculative-config command-line argument is handled. The changes correctly fix the immediate parsing issue. However, I've identified a critical flaw in the new type check within _is_v1_supported_oracle. The current fix is incomplete as it only validates the configuration when it's a dictionary, but overlooks the case where it's a SpeculativeConfig object. This could lead to incorrect behavior and subsequent runtime failures. I have provided a detailed comment with a suggested code change to make the validation more robust.

@yyzxw yyzxw changed the title [Bugfix] Parse SpeculativeConfig error [Bugfix] Parse SpeculativeConfig Error Sep 18, 2025
@yyzxw yyzxw force-pushed the fix/speculative-config-parse-error branch 2 times, most recently from fa070f8 to ac91b85 Compare September 18, 2025 08:13
Copy link
Member

@DarkLight1337 DarkLight1337 left a comment

Choose a reason for hiding this comment

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

@yyzxw
Copy link
Contributor Author

yyzxw commented Sep 23, 2025

@hmellor PTAL, thanks!

Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

It is already parsed as a dict in

# We construct SpeculativeConfig using fields from other configs in
# create_engine_config. So we set the type to a JSON string here to
# delay the Pydantic validation that comes with SpeculativeConfig.
vllm_kwargs["speculative_config"]["type"] = optional_type(json.loads)
vllm_group.add_argument("--speculative-config",
**vllm_kwargs["speculative_config"])

Only the second change is necessary

Signed-off-by: zxw <1020938856@qq.com>
@yyzxw yyzxw force-pushed the fix/speculative-config-parse-error branch from ac91b85 to a7983f0 Compare September 23, 2025 14:10
Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor hmellor force-pushed the fix/speculative-config-parse-error branch from c456609 to 9b510f5 Compare September 25, 2025 08:58
@hmellor
Copy link
Member

hmellor commented Sep 25, 2025

(sorry I messed up DCO on my commit so I force-pushed to replace just my commit with a signed-off one)

@hmellor hmellor enabled auto-merge (squash) September 25, 2025 08:59
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 25, 2025
@hmellor hmellor merged commit eaeca3c into vllm-project:main Sep 25, 2025
53 checks passed
Zhuul pushed a commit to Zhuul/vllm that referenced this pull request Sep 26, 2025
Signed-off-by: zxw <1020938856@qq.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: zxw <1020938856@qq.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: yewentao256 <zhyanwentao@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: --speculative-config '{"method": "ngram", "num_speculative_tokens": 10, "ngram_prompt_lookup_max": 10}' does not work
3 participants