FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: Stop add_cpu_torch_hashes failing on a universal torch split by haoxu0 · Pull Request #6959 · feast-dev/feast · GitHub

Repository navigation

fix: Stop add_cpu_torch_hashes failing on a universal torch split - #6959

Open
haoxu0 wants to merge 3 commits into
feast-dev:masterfrom
haoxu0:fix/cpu-torch-hashes-local-version
Open

haoxu0 wants to merge 3 commits into
feast-dev:masterfrom
haoxu0:fix/cpu-torch-hashes-local-version

Conversation

haoxu0 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

What is broken

make lock-python-dependencies-all cannot complete on master. It aborts after compiling the first file:

ValueError: Missing CPU hashes for torch==2.14.1+cpu
make: *** [lock-python-dependencies-all] Error 1

Because the target begins with rm -rf sdk/python/requirements/*, a run leaves the tree with 14 of the 15 requirements files deleted and nothing regenerated.

Why

uv pip compile --universal --torch-backend cpu splits torch by marker, so the lock holds two pins per package. This is the shape in all three committed py3.*-ci-requirements.txt today:

torch==2.14.0 ; sys_platform == 'darwin'
torch==2.14.0+cpu ; sys_platform != 'darwin'
torchvision==0.29.0 ; sys_platform == 'darwin'
torchvision==0.29.0+cpu ; sys_platform != 'darwin'

add_hashes looked every entry up by appending +cpu to its version:

hashes = cpu_hashes.get((name, f"{version}+cpu"))

For the second pin of each pair that asks for 2.14.0+cpu+cpu, which no resolve can produce, so the lookup missed and the script raised.

Why the tests did not catch it

test_cpu_hashes_preserve_pypi_hashes_and_pins feeds in a bare torch==2.13.0 and nothing else, so the +cpu pin that a real --universal resolve emits is never exercised. The script and its tests landed together in #6188, and the requirements files were last regenerated before that, so the target appears not to have been run since.

The change

The +cpu pin is already the CPU wheel, so it needs no hashes added and is now returned untouched.

Returning it, rather than normalising the lookup key to version.removesuffix("+cpu") + "+cpu", is deliberate: both stop the crash, but normalising would union the CPU resolve's hashes into that entry and could widen the set of artifacts the pin accepts. Returning it leaves the hash set exactly as the resolver wrote it, so no lock can start accepting an artifact it did not accept before.

A genuine version mismatch still raises, which the existing third test covers unchanged.

Verification

Two tests added, for a single marker split and for the torch + torchvision pair the lock actually contains, both asserting idempotency. sdk/python/tests/unit/infra/scripts/test_cpu_torch_hashes.py: 4 passed.

Checked against real data — add_cpu_hashes run over each committed file with the CPU pins a --torch-backend cpu --no-deps resolve would return:

file before after
py3.10-ci-requirements.txt raised OK, idempotent
py3.11-ci-requirements.txt raised OK, idempotent
py3.12-ci-requirements.txt raised OK, idempotent

ruff check and ruff format --check clean.

I found this trying to satisfy a review on #6958, which asks for regenerated locks and cannot be done until this is fixed.

`make lock-python-dependencies-all` aborts before it writes a single
requirements file:

    ValueError: Missing CPU hashes for torch==2.14.1+cpu
    make: *** [lock-python-dependencies-all] Error 1

`uv pip compile --universal --torch-backend cpu` splits torch by marker,
so the lock it produces holds two pins per package:

    torch==2.14.1 ; sys_platform == 'darwin'
    torch==2.14.1+cpu ; sys_platform != 'darwin'

`add_hashes` looked each entry up by appending `+cpu` to its version. For
the second pin that asks for `2.14.1+cpu+cpu`, which cannot exist, so the
lookup missed and the script raised.

The `+cpu` pin is already the CPU wheel and so needs nothing added, and it
is now returned untouched. Returning it rather than normalising the lookup
key keeps its hash set exactly as the resolver wrote it, so this cannot
alter any artifact a lock already accepts.

The existing tests passed because their input carries only a bare
`torch==2.13.0`, never the split a real `--universal` resolve emits. Two
tests are added for that shape, including the torch and torchvision pair
the lock actually contains, and both assert idempotency. Checked against
all three committed `py3.*-ci-requirements.txt`: each now runs clean and is
idempotent, where each raised before.

The genuine mismatch case still raises, which the untouched third test
covers.

Signed-off-by: hao-xu5 <hxu44@apple.com>
haoxu0 requested a review from a team as a code owner October 7, 2026 07:04

codecov-commenter commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.32%. Comparing base (9d42729) to head (2cded3e).
⚠️ Report is 1 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6959      +/-   ##
==========================================
- Coverage   49.32%   49.32%   -0.01%     
==========================================
  Files         443      443              
  Lines       55094    55094              
  Branches     8017     8017              
==========================================
- Hits        27176    27175       -1     
  Misses      26035    26035              
- Partials     1883     1884       +1     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 50.71% <ø> (-0.01%) ⬇️
see 1 file with indirect coverage changes

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf930e5...2cded3e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL