-
Notifications
You must be signed in to change notification settings - Fork 1k
[Feature]: Remove some useless hf_overrides in yaml
#1898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
princepride
merged 2 commits into
vllm-project:main
from
princepride:remove-useless-hf_overrides
Mar 18, 2026
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing
hf_overrides.architecturesfrom the Qwen3-TTS Code2Wav stage disables the config path that stripsrope_parametersfor this non-LLM decoder:Qwen3TTSConfig.get_text_config()only performs that strip whenarchitecturescontainsCode2Wav(vllm_omni/model_executor/models/qwen3_tts/configuration_qwen3_tts.py), butOmniEngineArgs.create_model_config()setshf_config.architecturesonly afterOmniModelConfiginitialization (vllm_omni/engine/arg_utils.py), whileOmniModelConfig.__post_init__already computeshf_text_config(vllm_omni/config/model.py). This means Code2Wav can run with mRoPE still enabled, which regresses stage-1 runtime behavior/latency for all qwen3_tts configs touched here.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statement in this review is inaccurate. All changes can be kept, and there is no need to revert any of them. Below is a detailed analysis:
The Reviewer's Argument
The reviewer states that removing
hf_overrides.architectureswill causeQwen3TTSConfig.get_text_config()to fail to striprope_parameters, thereby causing Code2Wav to mistakenly enable mRoPE. They cited three points in the execution timeline:get_text_config()only stripsrope_parameterswhenarchitecturesincludes "Code2Wav".create_model_config()setshf_config.architecturesafter the initialization ofOmniModelConfig.OmniModelConfig.__post_init__has already computedhf_text_configduring initialization.Why This Argument is Invalid
Key Point 1:
uses_mropeis a lazy property, not cached during__post_init__This is a standard
@property(not a@cached_property), meaning it is re-evaluated every time it is accessed.Key Point 2: The
uses_mrope()function callsconfig.get_text_config()every timeKey Point 3:
hf_config.architecturesis correctly set before the model runner accessesuses_mropeIn
create_model_config()withinarg_utils.py, at line 209:And the
OmniModelConfig.architecturesproperty returns[self.model_arch]:Since
model_arch: Qwen3TTSCode2Wavis preserved in the YAML,omni_config.architecturesevaluates to["Qwen3TTSCode2Wav"]. This is assigned tohf_config.architecturesbeforecreate_model_config()returns.Complete Timeline Analysis
__post_init__**:hf_config.architecturesholds its original value (fromconfig.json), which does not include "Code2Wav". Whenhf_text_configis computed,rope_parametersis not yet stripped.create_model_config()returns (Line 209):hf_config.architectures = ["Qwen3TTSCode2Wav"]is set.model_config.uses_mropeis accessed → callsuses_mrope(hf_config)→ callshf_config.get_text_config(). By this point,self.architecturesalready contains "Code2Wav" →rope_parametersis correctly stripped →uses_mropereturnsFalse.The Impact of
rope_parametersDuring__post_init__The only place where
hf_text_config.rope_parametersis used during__post_init__is in_get_and_verify_max_len(to calculatemax_model_len). However, the Code2Wav YAML explicitly specifiesmax_model_len: 32768, and themax_position_embeddingsin the talker config is also 32768, so this validation will not fail.The Case of FishSpeech
FishSpeechS2ProConfig.get_text_config()does not have a similar stripping logic forrope_parameters:Therefore, the removal of
hf_overridesfor FishSpeech has absolutely no impact.Conclusion
All changes can be kept, and there is no need to revert any of them. While the timing issue pointed out by the reviewer does indeed exist during the
__post_init__phase (architectureshas not been set at that point), it will not cause Code2Wav to mistakenly use mRoPE at runtime. This is becauseuses_mropeis lazily evaluated. When it is actually used by the model runner,hf_config.architectureshas already been correctly set to["Qwen3TTSCode2Wav"].There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use Claude-4.6-Opus-high review the review