Skip to content

[Chore] Separate out hashing utilities from vllm.utils #27151

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2025
Merged

[Chore] Separate out hashing utilities from vllm.utils #27151

merged 1 commit into from
Oct 19, 2025
+74 −62

Conversation

dongbo910220
Copy link
Contributor

@dongbo910220 dongbo910220 commented Oct 18, 2025 •
edited by github-actions bot
Loading

Part of #26900

This PR continues the effort to clean up vllm.utils by extracting hashing-related helper functions into a new, dedicated module vllm/utils/hashing.py.

The following functions have been moved:

  • vllm.utils.sha256 -> vllm.utils.hashing.sha256
  • vllm.utils.sha256_cbor -> vllm.utils.hashing.sha256_cbor
  • vllm.utils.get_hash_fn_by_name -> vllm.utils.hashing.get_hash_fn_by_name

CC: @DarkLight1337

Test Plan

Test Result

@mergify mergify bot added the v1 label Oct 18, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 18, 2025
View reviewed changes
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 refactors hashing-related utility functions from vllm.utils into a new dedicated module vllm.utils.hashing. The changes are well-structured and improve code organization. I've identified a performance issue in the new hashing.py module where pickle is imported dynamically within a function that is likely on a hot path, and I've provided a suggestion to move the import to the module level to avoid repeated execution.

@dongbo910220 dongbo910220 force-pushed the feature/refactor-hashing-utils branch from 8e49069 to 33d35f7 Compare October 18, 2025 16:34
DarkLight1337
DarkLight1337 reviewed Oct 18, 2025
View reviewed changes
vllm/utils/hashing.py Outdated
import cbor2

# Avoid direct `import pickle` to satisfy pre-commit policy on new imports
pickle = importlib.import_module("pickle")
Copy link
Member

Choose a reason for hiding this comment

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

Please update tools/pre_commit/check_pickle_imports.py instead

@dongbo910220 dongbo910220 force-pushed the feature/refactor-hashing-utils branch from 33d35f7 to e048d1d Compare October 18, 2025 17:05
@dongbo910220 dongbo910220 requested a review from hmellor as a code owner October 18, 2025 17:05
DarkLight1337
DarkLight1337 reviewed Oct 18, 2025
View reviewed changes
DarkLight1337
DarkLight1337 reviewed Oct 18, 2025
View reviewed changes
@dongbo910220 dongbo910220 force-pushed the feature/refactor-hashing-utils branch 2 times, most recently from 3c2862d to f93dbcd Compare October 18, 2025 17:18
@mergify mergify bot added the kv-connector label Oct 18, 2025
@dongbo910220
Copy link
Contributor Author

dongbo910220 commented Oct 18, 2025 •
edited
Loading

@DarkLight1337

Thanks for the review! I've addressed all the feedback:

Changes made:

  1. Updated import paths: Migrated all 7 files to use from vllm.utils.hashing import ...
    instead of importing from vllm.utils

  2. Simplified pickle import: Changed hashing.py to use direct import pickle and added it
    to the pre-commit allowlist

  3. Removed backward compatibility: Completely removed the re-exports from
    vllm/utils/__init__.py as suggested

  4. Allowlist note: Kept vllm/utils/__init__.py in the allowlist since it still uses
    cloudpickle in _run_method (line 1842). Let me know if this should be handled differently.

Verification:

  • No remaining references to old import paths

Let me know if anything else needs adjustment.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) October 18, 2025 18:04
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 18, 2025
auto-merge was automatically disabled October 18, 2025 18:59

Head branch was pushed to by a user without write access

@dongbo910220 dongbo910220 force-pushed the feature/refactor-hashing-utils branch from f93dbcd to 6e5925d Compare October 18, 2025 18:59
@mergify Mergify
Copy link

mergify bot commented Oct 18, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @dongbo910220.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 18, 2025
@dongbo910220
refactor(utils): extract hashing helpers to dedicated module
e3e2f33 
- Move sha256, sha256_cbor, and get_hash_fn_by_name from vllm.utils.__init__
  to vllm/utils/hashing.py
- Update import in vllm/v1/engine/core.py to use new module path
- Add vllm/utils/hashing.py to pickle import allowlist
- Use direct 'import pickle' instead of importlib workaround

These hashing functions are internal-only, so no backward compatibility
is needed. This refactoring improves code organization while keeping
the implementation cleaner.

Signed-off-by: dongbo910220 <[email protected]>
@dongbo910220 dongbo910220 force-pushed the feature/refactor-hashing-utils branch from 6e5925d to e3e2f33 Compare October 18, 2025 19:03
@mergify mergify bot removed the needs-rebase label Oct 18, 2025
@dongbo910220
Copy link
Contributor Author

@DarkLight1337
Just fixed a CI failure - missed updating tests/utils_/test_utils.py which was still importing sha256 from vllm.utils.
Also rebased onto latest main to resolve conflicts with #26908 (torch_utils refactor).
Could you please take another look and re-enable auto-merge when you have a moment? Thanks!

@DarkLight1337 DarkLight1337 merged commit 8a29711 into vllm-project:main Oct 19, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DarkLight1337 DarkLight1337 DarkLight1337 approved these changes

@hmellor hmellor Awaiting requested review from hmellor hmellor is a code owner

@ApostaC ApostaC Awaiting requested review from ApostaC ApostaC is a code owner

@WoosukKwon WoosukKwon Awaiting requested review from WoosukKwon WoosukKwon is a code owner

@robertgshaw2-redhat robertgshaw2-redhat Awaiting requested review from robertgshaw2-redhat robertgshaw2-redhat is a code owner

@njhill njhill Awaiting requested review from njhill njhill is a code owner

@ywang96 ywang96 Awaiting requested review from ywang96 ywang96 is a code owner

@comaniac comaniac Awaiting requested review from comaniac comaniac is a code owner

@alexm-redhat alexm-redhat Awaiting requested review from alexm-redhat alexm-redhat is a code owner

@heheda12345 heheda12345 Awaiting requested review from heheda12345 heheda12345 is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kv-connector ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dongbo910220 @DarkLight1337