Skip to content

Conversation

@qthequartermasterman
Copy link
Contributor

@qthequartermasterman qthequartermasterman commented Sep 25, 2025

Purpose

It was previously unknown if #24278 was compatible with LoRA adapters or not. This PR adds tests explicitly for that combination. Since #25663 swapped out Zephyr for OPT125-m for testing prompt embeds, this PR also adds LoRA support for opt125-m.

Test Plan

Updated tests cases. I've also tested it locally with a meta-llama/Llama-3.1-8B-Instruct LoRA and everything seems to work as expected there.

Test Result

New tests are working locally. Pending CI.


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: Andrew Sansom <andrew@protopia.ai>
@mergify mergify bot added the documentation Improvements or additions to documentation label Sep 25, 2025
@qthequartermasterman
Copy link
Contributor Author

qthequartermasterman commented Sep 25, 2025

@DarkLight1337. I'm not sure who else would need to look at this. I also wonder that using this model could speed up some of the other entrypoints LoRA tests that are currently using zephyr-7b, just like you sped up these tests in #25663.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds LoRA support for OPT models and includes corresponding tests. The changes to enable LoRA in the OPT model implementation are mostly correct, following patterns from other models in the repository. However, I found a critical issue in the initialization of the LogitsProcessor which would lead to incorrect behavior when using LoRA adapters with extra vocabulary tokens. My review provides a code suggestion to fix this.

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: Andrew Sansom <andrew@protopia.ai>
…list

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Copy link
Member

@DarkLight1337 DarkLight1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if the tests pass, cc @jeejeelee if you want to double check the model

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) September 26, 2025 04:23
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 26, 2025
@DarkLight1337
Copy link
Member

I'm not sure who else would need to look at this. I also wonder that using this model could speed up some of the other entrypoints LoRA tests that are currently using zephyr-7b, just like you sped up these tests in #25663.

cc @jeejeelee

auto-merge was automatically disabled September 26, 2025 15:29

Head branch was pushed to by a user without write access

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: Andrew Sansom <andrew@protopia.ai>
@qthequartermasterman
Copy link
Contributor Author

@DarkLight1337 This looks like it's ready for re-review. Thanks!

Thanks @jeejeelee for your help.

@jeejeelee jeejeelee merged commit 78a47f8 into vllm-project:main Sep 30, 2025
50 checks passed
pdasigi pushed a commit to pdasigi/vllm that referenced this pull request Oct 2, 2025
… Models (vllm-project#25717)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
… Models (#25717)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: yewentao256 <zhyanwentao@126.com>
tomeras91 pushed a commit to tomeras91/vllm that referenced this pull request Oct 6, 2025
… Models (vllm-project#25717)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: Tomer Asida <57313761+tomeras91@users.noreply.github.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
… Models (vllm-project#25717)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
… Models (vllm-project#25717)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
… Models (vllm-project#25717)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
… Models (vllm-project#25717)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants