-
-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[BugFix][LoRA] use adapter_id instead of id field of lora_request #27728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Biswa Panda <[email protected]>
There was a problem hiding this 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 correctly fixes a bug in vllm/v1/core/block_pool.py. The code was attempting to access request.lora_request.id, but the LoRARequest object does not have an id attribute. The change replaces this with request.lora_request.adapter_id, which is the correct attribute. This prevents a runtime AttributeError when KV cache events are enabled for LoRA requests. The fix is accurate and addresses the issue effectively. I find no further issues with this change.
markmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, the fix is correct 👍
It looks like the bug has existed since the code was merged in #16750 so it appears we have no test coverage for KV event publishing with LoRA. It would be great to get that gap plugged
|
Agreed, this could have been caught with some tests. |
Probably |
Signed-off-by: Biswa Panda <[email protected]>
|
Thanks @markmc , I've added unit test and results. PTAL |
lgtm, thanks! |
|
Thank you @markmc - when will this PR be merged? what are next steps? |
Next step is for a committer to review and approve the PR, perhaps @jeejeelee |
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]>
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]> Signed-off-by: git-jxj <[email protected]>
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]>
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]>
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]>
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]>
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]>
…lm-project#27728) Signed-off-by: Biswa Panda <[email protected]>
Purpose
This PR fixes a bug in the KV cache block storage system where the code was incorrectly trying to access
request.lora_request.idinstead of the correctrequest.lora_request.adapter_idproperty.Root Cause:
The LoRARequest class in vllm/lora/request.py does not have an
idfield. Instead, it has:lora_int_id: The actual ID fieldadapter_id: A property that returns lora_int_idSample Error:
Test Plan
Added unit test case
test_kv_cache_events_with_loraintest_prefix_caching.pyTest Result
After
Before
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.