[Bugfix][Core] fix KeyError in _update_waiting_for_remote_kv#25563
[Bugfix][Core] fix KeyError in _update_waiting_for_remote_kv#25563kebe7jun wants to merge 1 commit intovllm-project:mainfrom
Conversation
Signed-off-by: Kebe <mail@kebe7jun.com>
There was a problem hiding this comment.
Code Review
This pull request addresses a KeyError that can occur in _update_from_kv_xfer_finished due to a race condition with request abortion. The fix is to check if a request ID exists in self.requests before attempting to access it. The change is correct and effectively prevents the crash. I've suggested a minor refinement to use dict.get() for a slightly cleaner and more efficient implementation.
| if req_id not in self.requests: | ||
| continue | ||
| logger.debug("Finished sending KV transfer for request %s", req_id) | ||
| self._free_blocks(self.requests[req_id]) |
There was a problem hiding this comment.
The added check correctly prevents the KeyError if a request has been aborted and removed from self.requests. For a slight improvement in efficiency and conciseness, you could use dict.get() to perform the check and retrieval in a single operation. This avoids the double dictionary lookup (in followed by []).
| if req_id not in self.requests: | |
| continue | |
| logger.debug("Finished sending KV transfer for request %s", req_id) | |
| self._free_blocks(self.requests[req_id]) | |
| request = self.requests.get(req_id) | |
| if request: | |
| logger.debug("Finished sending KV transfer for request %s", req_id) | |
| self._free_blocks(request) |
| logger.debug("Finished recving KV transfer for request %s", req_id) | ||
| self.finished_recving_kv_req_ids.add(req_id) | ||
| for req_id in (kv_connector_output.finished_sending or ()): | ||
| if req_id not in self.requests: |
There was a problem hiding this comment.
maybe we could also add the existence check for for req_id in (kv_connector_output.finished_recving or ()):
|
Should be fixed by #25844 |
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
Purpose
Fix #25562
Test Plan
N/A
Test Result
N/A
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.