[Core]ModelConfig use architecture rather than archiectures#31633
[Core]ModelConfig use architecture rather than archiectures#31633charlotte12l wants to merge 13 commits intovllm-project:mainfrom
Conversation
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of model architectures by transitioning from a list of architectures (architectures: list[str]) to a single architecture string (architecture: str) across the codebase. This change impacts configuration files (base_model_arch_groundtruth.json, draft_model_arch_groundtruth.json), test assertions, and core logic within vllm/config/model.py, vllm/config/model_arch.py, vllm/lora/layers/fused_moe.py, vllm/model_executor/models/registry.py, vllm/model_executor/models/transformers/moe.py, and vllm/transformers_utils/model_arch_config_convertor.py. The ModelArchitectureConfig class is updated to reflect this, and methods that previously accepted or returned a list of architectures now handle a single string. A review comment highlighted a potential AttributeError in vllm/model_executor/models/transformers/moe.py where self.config.architecture was used instead of the correct self.config.architectures[0] for transformers.PretrainedConfig objects, which was subsequently addressed by the author.
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
|
Just fyi @heheda12345 @hmellor I found some risky codes when writing this PR: EAGLE models uses vllm/vllm/config/speculative.py Lines 398 to 406 in 0d4044e |
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
Is this a bug on main branch? |
Yes, it's on main. Let me try to fix it first in a separate PR |
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
fixed in #31753 |
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
Signed-off-by: 22quinn <33176974+22quinn@users.noreply.github.com>
|
Test failures look legit |
ba1b544 to
1c300d8
Compare
@hmellor Just found some models have multiple architectures :(
Would appreciate any suggestions on how to proceed! cc @heheda12345 |
|
This pull request has merge conflicts that must be resolved before it can be |
Purpose
Currently, hf_config/PretrainedConfig has s field architectures which is of type None| List[str]. However, when it's a List, it always of length 1 according to @hmellor in #28454 (comment)
Therefore, this PR follow the discussion #28454 (comment) and consolidate to always use
architecturein ModelConfig. We will raise error if configarchitectureis different from inspected model clsarchitectureTest Plan
python -m pytest tests/config/test_model_arch_config.py
Test Result
passed
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.cc @heheda12345 @hmellor @DarkLight1337