toy_model_plugin's kToyInfo is positional over a ModelInfo whose leading fields are all bool, so a mid-struct insert misassigns SILENTLY
Nobody has claimed this yet.
Assessment
- Difficulty
- 1/5
- Estimated time
- Under an hour
- Newbie friendliness
- 92/100
- Issue type
- Refactor
- Clarity
- Clearly specified
- Activity status
- Active
- Tech stack
- cpp
- Domain
- testing-qa
Research direction
Start at tests/vllm/plugins/toy_model_plugin.cpp:67 and compare kToyInfo with ModelInfo in include/vllm/model_executor/models/model_registry.h:64. Convert kToyInfo to designated initialization, then run the relevant toy model plugin tests and confirm the existing is_text_generation_model assertions still pass without behavior changes.
Written by the indexing model from the issue text.
Description
Row: ENG-MM-INPUT-PIPELINE
tests/vllm/plugins/toy_model_plugin.cpp:67 initialises vllm::ModelInfo kToyInfo
positionally:
constexpr vllm::ModelInfo kToyInfo{
/*is_text_generation_model=*/true,
};
ModelInfo (include/vllm/model_executor/models/model_registry.h:64) opens with five
consecutive bool fields: is_text_generation_model, is_pooling_model, is_hybrid,
has_inner_state, supports_multimodal.
Insert any new bool ahead of is_text_generation_model and that true silently becomes
the new field's value while is_text_generation_model falls back to its default false.
It compiles clean. No diagnostic fires.
This is the same defect class that #2398 hit on the sibling struct, and the reason to file
it separately is that the sibling was survivable and this one is not. There, ModelFactory
gained encode_mm mid-struct and kToyFactory's positional initializer put a bool into a
function-pointer slot -- a type error, caught loudly by four CI lanes:
tests/vllm/plugins/toy_model_plugin.cpp:65:1: error: cannot convert 'bool' to
'vllm::ModelEncodeMmFn' ... in initialization
ModelInfo's leading run of same-typed fields removes exactly that protection. The failure
would instead be a plugin that quietly reports the wrong capability, and the tests asserting
is_text_generation_model would read the misassigned value as the truth.
#2398 converted kToyFactory to designated initialization and enumerated all 37
ModelFactory initialisers in the tree (the other 36 were already designated). kToyInfo
was noted in that work and deliberately left, because changing it there would have grown a
pull request that was already at review. It is the last positional initialiser of a
registration struct in the tree.
Fix: convert kToyInfo to designated initialisation. One line, no behaviour change.
Worth pairing with a sweep for any other positional initialiser over a struct whose leading
fields share a type, since that combination is what makes the misassignment silent rather
than a compile error.
- Dominant language
- C++
- Stars
- 423
- Forks
- 53
- Avg merge
- 19h 7m
- Merged PRs (30d)
- 337
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from mudler/vllm.cpp
-
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
-
Difficulty 1/5 Under an hour Newbie friendliness 88/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
-
sync(UPSTREAM-SYNC-HEADPIN): step6-c1a credits the version read to a job whose own read returned 1 Open
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
Similar issues
-
Difficulty 1/5 Under an hour Newbie friendliness 90/100
AXERA-TECH/ax-llm#77 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
-
bug-unconfirmed
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
NVIDIA/cuda-samples#453 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
infiniflow/infinity#3502 ·