Skip to content

Conversation

DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Aug 26, 2025

Purpose

FIX https://github.com/vllm-project/vllm/pull/23623/files#r2301482573

Test Plan

Test Result


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.

@DarkLight1337 DarkLight1337 requested a review from Isotr0py August 26, 2025 16:14
@DarkLight1337 DarkLight1337 requested a review from ywang96 as a code owner August 26, 2025 16:14
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 26, 2025
@mergify mergify bot added the multi-modality Related to multi-modality (#4194) label Aug 26, 2025
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 correctly fixes a bug where the original shape of a bfloat16 tensor was being incorrectly determined during the hashing process, which could lead to hash collisions. While this fix is necessary, I've identified a pre-existing critical issue in the same code block that handles bfloat16 tensors. The method used to convert the tensor to its byte representation is incorrect and will result in a runtime error. I have provided a review comment with a suggested fix for this critical bug.

Comment on lines 53 to 62
tensor_obj = tensor_obj.contiguous()
tensor_obj = tensor_obj.view(
(tensor_obj.numel(), )).view(torch.uint8)

return cls.item_to_bytes(
"tensor", {
"original_dtype": str(tensor_dtype),
"original_shape": tuple(tensor_obj.shape),
"data": tensor_obj.numpy()
"original_shape": tuple(tensor_shape),
"data": tensor_obj.numpy(),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The method used to get the byte representation of a bfloat16 tensor is incorrect and will cause a TypeError at runtime. The torch.Tensor.view() method expects a shape as input, not a dtype. Additionally, a bfloat16 tensor (2 bytes per element) cannot be directly viewed as a uint8 tensor (1 byte per element) by reinterpreting memory this way.

A more robust approach is to first view the bfloat16 tensor as a type of the same size (e.g., uint16), then convert it to a NumPy array, and finally view that array as uint8 to get the raw bytes for hashing.

Suggested change
tensor_obj = tensor_obj.contiguous()
tensor_obj = tensor_obj.view(
(tensor_obj.numel(), )).view(torch.uint8)
return cls.item_to_bytes(
"tensor", {
"original_dtype": str(tensor_dtype),
"original_shape": tuple(tensor_obj.shape),
"data": tensor_obj.numpy()
"original_shape": tuple(tensor_shape),
"data": tensor_obj.numpy(),
})
tensor_obj = tensor_obj.contiguous()
# To correctly get the byte representation of a bfloat16 tensor,
# it should be viewed as a type of the same size (e.g., uint16),
# then converted to a numpy array, and finally viewed as uint8.
data_np = tensor_obj.view(torch.uint16).numpy().view(np.uint8)
return cls.item_to_bytes(
"tensor", {
"original_dtype": str(tensor_dtype),
"original_shape": tuple(tensor_shape),
"data": data_np,
})

Add test for bfloat16 hash collision
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) August 26, 2025 16:30
@DarkLight1337 DarkLight1337 merged commit 9715f7b into vllm-project:main Aug 26, 2025
36 of 37 checks passed
@DarkLight1337 DarkLight1337 deleted the fix-hash-shape branch August 26, 2025 19:01
tc-mb pushed a commit to tc-mb/vllm that referenced this pull request Aug 27, 2025
Signed-off-by: DarkLight1337 <[email protected]>
Co-authored-by: Lukas Geiger <[email protected]>
Signed-off-by: tc-mb <[email protected]>
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 28, 2025
xiao-llm pushed a commit to xiao-llm/vllm that referenced this pull request Aug 28, 2025
Signed-off-by: DarkLight1337 <[email protected]>
Co-authored-by: Lukas Geiger <[email protected]>
Signed-off-by: Xiao Yu <[email protected]>
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Aug 28, 2025
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Sep 3, 2025
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

multi-modality Related to multi-modality (#4194) 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