-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[Bugfix] fix IntermediateTensors equal method #23027
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
[Bugfix] fix IntermediateTensors equal method #23027
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 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 🚀 |
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 aims to fix a bug in the __eq__
method of the IntermediateTensors
class. The original implementation was incorrect. The proposed change is an improvement but is still not fully correct for comparing dictionaries of PyTorch tensors. My review provides a corrected implementation for comparing the tensors
attribute and points out that another attribute, kv_connector_output
, is also missing from the comparison.
vllm/sequence.py
Outdated
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.
The dictionary comparison self.tensors == other.tensors
is incorrect for dictionaries of PyTorch tensors. tensor1 == tensor2
performs an element-wise comparison and returns a boolean tensor, not a scalar boolean. This will cause the dictionary comparison to almost always return False
if tensors have more than one element. You should iterate through the dictionary and use torch.equal()
to compare tensors.
Additionally, for a complete equality check, the kv_connector_output
attribute should also be compared. I've left that out of the suggestion for brevity, but it should be added for correctness.
def __eq__(self, other: object):
if not isinstance(other, self.__class__):
return False
if self.tensors.keys() != other.tensors.keys():
return False
return all(
torch.equal(self.tensors[k], other.tensors[k])
for k in self.tensors)
610fe9c
to
1b1c62a
Compare
vllm/sequence.py
Outdated
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.
Add a unit test?
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.
Done.
41bbfc6
to
c591a0b
Compare
7c494e3
to
3cdcc7f
Compare
Head branch was pushed to by a user without write access
3cdcc7f
to
6dc1dab
Compare
Signed-off-by: Andy Xie <[email protected]>
6dc1dab
to
9d2e155
Compare
Signed-off-by: Andy Xie <[email protected]>
Signed-off-by: Andy Xie <[email protected]>
Signed-off-by: Andy Xie <[email protected]>
Signed-off-by: Andy Xie <[email protected]> Signed-off-by: Duncan Moss <[email protected]>
Signed-off-by: Andy Xie <[email protected]>
Signed-off-by: Andy Xie <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
Signed-off-by: Andy Xie <[email protected]>
Signed-off-by: Andy Xie <[email protected]>
Signed-off-by: Andy Xie <[email protected]>
Purpose
Compare struct fields in
IntermediateTensors
equal method. I.e., comparetensors
.Test Plan
NA
Test Result
NA
(Optional) Documentation Update
NA
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.