Fix hard-coded parameter name in gemma3n.py#27946
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with a hard-coded parameter name in gemma3n.py by dynamically determining the model prefix. This makes the KV cache sharing mechanism more robust across different Gemma-3n model variations. The overall logic is sound. I have one suggestion to further improve the robustness by failing fast in case of an unexpected prefix format, rather than relying on a default value which could lead to subtle issues.
|
cc @NickLucche |
NickLucche
left a comment
NickLucche
left a comment
There was a problem hiding this comment.
Looks correct, thanks for contributing @seungduk-yanolja !
Please check the DCO errors in CI
Signed-off-by: Seungduk Kim <seungduk.kim@yanolja.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Seungduk Kim <seungduk.kim@yanolja.com>
seungduk-yanolja
force-pushed
the
gemma3n
branch
from
ca2f6b2 to
8ec74c0
Compare
seungduk-yanolja
requested review from
ApostaC,
WoosukKwon,
alexm-redhat,
comaniac,
heheda12345,
njhill,
robertgshaw2-redhat and
ywang96
as code owners
Signed-off-by: Seungduk Kim <seungduk.kim@yanolja.com>
…lm-project#27728) Signed-off-by: Biswa Panda <biswa.panda@gmail.com>
seungduk-yanolja
force-pushed
the
gemma3n
branch
from
8ec74c0 to
9c3e49e
Compare
|
Can someone tell me how to merge this? |
I think @NickLucche simply forgot to turn on auto-merge. I just merged it. |
Signed-off-by: Seungduk Kim <seungduk.kim@yanolja.com> Signed-off-by: Biswa Panda <biswa.panda@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Biswa Panda <biswa.panda@gmail.com> Co-authored-by: Nicolò Lucchesi <nlucches@redhat.com>
Signed-off-by: Seungduk Kim <seungduk.kim@yanolja.com> Signed-off-by: Biswa Panda <biswa.panda@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Biswa Panda <biswa.panda@gmail.com> Co-authored-by: Nicolò Lucchesi <nlucches@redhat.com>
4 participants
Purpose
When Gemma3nCausalLM is extracted from gemma-3n models, the hardcoded parameter name causes an error.
Test Plan
Tested both extracted Gemma3nCausalLM and google/gemma-3n-e4b-it
Test Result
Both worked
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.