[Core][MultiModalHasher] Don't convert memoryviews to bytes during hashing

[lgeiger]

Purpose

As mentioned in #22044 hashing large multimodal input can be inefficient. #19484 removed a data copy of numpy arrays by relying on memory views for c-contiguous data.

blake3 already accepts uint8 buffers so this PR removes the need to convert everything back to bytes in item_to_bytes

vllm/vllm/multimodal/hasher.py

Line 81 in 5bcc153

return b''.join(kb + vb for kb, vb in cls.iter_item_to_bytes(key, obj))

Implementation wise serialize_item now yields either bytes or a memoryview which blake3 directly consumes.

Test Plan

Correctness should be covered by the existing hasher tests on CI.

The performance can be measured using:

import numpy as np
from vllm.multimodal.hasher import MultiModalHasher

np.random.seed(42)
data = np.random.randn(3840, 2160, 4)

%timeit MultiModalHasher.hash_kwargs(data=data)

Test Result

For a 4k image this speeds up hashing by ~30%. This is not massive, but for a lot of multimodal input this might become noticeably.

# main
168 ms ± 892 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
# This PR
120 ms ± 1.09 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

[gemini-code-assist]

Code Review

This pull request introduces a performance optimization for hashing multimodal inputs by avoiding intermediate byte conversions and copies. The use of generators to yield bytes or memoryview objects, which are then directly consumed by the blake3 hasher, is a solid approach. The changes are well-reasoned and the performance gains are evident. However, I've identified a pre-existing critical issue in the handling of bfloat16 tensors that will cause a runtime error. This should be addressed to ensure the stability of the hashing mechanism.

[lgeiger]

Unrelated to this PR: what's the reason for converting all images to RGBA before hashing? This not only introduces some compute costs but also increases the numpy data which needs to be hashed by 30%.

[ywang96]

I remember this was due to some security concerns. See #17378

[lgeiger]

Looking at the unittests, this prevents hash collisions for images with different modes or palettes. How do you feel about hashing the mode, palette and data separately which should prevent the need for converting all images first. I'm happy to make a followup PR for something like this.

data = {"mode": obj.mode, "data": np.asarray(obj)}
if obj.palette is not None:
    data["palette"] = obj.palette.palette
yield from cls.iter_item_to_bytes("image", data)

[ywang96]

hashing the mode, palette and data separately

Yea I was going to suggest that and that should be much cheaper than needing to convert the image itself

[DarkLight1337]

Feel free to do it

[lgeiger]

I'll make a followup PR, since this would actually change the hash.

[lgeiger]

.view(np.uint8) needs to be used since blake3 only supports uint8/int8 buffers. Though calling .view(np.uint8) is very fast since it doesn't need to copy the data.

[ywang96]

Very much appreciate the contribution and optimization! @lgeiger

blake3 already accepts uint8 buffers so this PR removes the need to convert everything back to bytes in item_to_bytes

I actually didn't know this 😅 and I left a question

[ywang96]

LGTM but @DarkLight1337 should also take a look

[DarkLight1337]

Thanks for optimizing this!

[kexinoh]

This solution still has problems. In fact, there are still many parameters in the image that may affect its state, such as the tRNS state.
@lgeiger

[DarkLight1337]

This PR aims at addressing efficiency. The uniqueness of the hash should be same as before. If that's not the case (or if you feel that the uniqueness has some problems), please open a separate issue and provide more details