Repository navigation
[LoRA][Nemotron] Add LoRA support for Nemotron VL models (for the language model only) - #56231
Conversation
qthequartermasterman
left a comment
There was a problem hiding this comment.
This is timely. I was just looking this afternoon what would be needed for LoRA support for Nemotron 3 Omni. This PR looks good to me.
linitra24
left a comment
There was a problem hiding this comment.
Overall, looks good to me. I also noticed that this model is not listed in docs/models/supported_models.md. Should we add it there as well?
| # the LoRA delta is undefined for tokens with no mapping entry. | ||
| embed_tokens = self.get_language_model().model.embed_tokens | ||
| if isinstance(embed_tokens, BaseLayerWithLoRA): | ||
| embed_tokens = embed_tokens.base_layer |
There was a problem hiding this comment.
QQ: If embed_tokens is adapted during training, wouldn’t the internally generated text/special tokens that are not replaced by video embeddings also receive the LoRA delta?
There was a problem hiding this comment.
I had to dig deeper to give a proper answer (with some help from Claude for the vision/video code), and you're correct.
Video marker tokens - the per-frame separator text (e.g. Frame 1 sampled at 0.50 seconds:) and the <img> / </img> tags wrapping each frame do not get the embedding LoRA delta.
They're generated inside the encoder by _create_final_video_embeddings, so they never appear in the scheduled token batch that vLLM builds the per-token LoRA adapter index mapping from - and there can be more of them than max_num_batched_tokens, which is what that mapping is sized for.
Without this PR, that path raises an error, so video + LoRA doesn't work at all today.
This issue is unlikely to be a problem in practice:
- peft with target_modules="all-linear" does not include
nn.Embedding. you'd have to name embed_tokens explicitly. The public adapter I tested (https://huggingface.co/oridos/nemotron-tinystories-lora) has no embedding weights. I am not sure if LoRA weights for the embedding is common / popular. - With no embedding LoRA, this change is a no-op (the delta would be zero).
For basic enablement of LoRA for Nemotron VL models, I think this change is OK.
@linitra24 tell me if something here doesn't align with your understanding of the flow.
@jeejeelee is there any code change you think would help?
Raising an error if embedding LoRA is detected doesn't sound like a good idea - no model in vLLM rejects adapters with embedding weights today, so Nemotron VL would become uniquely strict.
Note:
Similar flow (and likely limitation) seems to exist for Qwen3-VL (see function _create_final_video_embeddings in file vllm/model_executor/models/qwen3_vl.py).
|
please respond to @linitra24's comment |
|
@linitra24 I added the Nemotron-VL models to supported_models |
|
Documentation preview: https://vllm--56231.org.readthedocs.build/en/56231/ |
|
/ci run |
|
❌ This PR is 282 commits behind upstream |
Signed-off-by: Daniel Serebrenik <daserebrenik@nvidia.com>
Signed-off-by: Daniel Serebrenik <daserebrenik@nvidia.com>
Signed-off-by: Daniel Serebrenik <daserebrenik@nvidia.com>
…orted_models Signed-off-by: Daniel Serebrenik <daserebrenik@nvidia.com>
Head branch was pushed to by a user without write access
6382e5b to
12be5bc
Compare
|
Rebased on main to allow CI run. @jeejeelee can you re-enable auto-merge ? |
|
/ci run |
|
❌ This PR is 3 commits behind upstream |
|
/ci run |
|
✅ Triggered Buildkite CI #89118 for commit |
|
@danisereb Do you plan on adding LoRA support to the vision/audio encoders as well at some point? |
If needed, I'd be happy to take this on. |
|
@qthequartermasterman @linitra24 |
|
@danisereb @linitra24 My colleague @Nan2018 actually has just opened a PR for vision/audio support: #58658. |
Purpose
At this time Nemotron VL models fail if used with LoRA adapters:
This PR enables LoRA on the language model of Nemotron VL models.
Both text and media embeddings flow through the language model after the multimodal merge, adapters apply to media tokens too.
What's not covered is LoRA on the encoders and connectors themselves (RADIO, mlp1, Parakeet), which vLLM gates behind the
supports_tower_connector_loraopt-in that this model does not declare.From
docs/models/supported_models.md:!!! note vLLM currently supports adding LoRA adapters to the language backbone for most multimodal models. Additionally, vLLM now experimentally supports adding LoRA to the tower and connector modules for some multimodal models. See [this page](../features/lora.md).Note: AI assistance was used in this PR.
Test Plan
Verify correctness using evals (gsm8k) with Nemotron VL model (with/without LoRA adapter).
Test Result
GSM8K results for Nemotron-3-Nano-Omni-30B-A3B:
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.