Skip to content

test: Cover the universal torch split in add_cpu_torch_hashes - #6980

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
haoxu0:test/cpu-torch-hashes-cpu-pin
Oct 9, 2026
Merged

ntkathole merged 1 commit into
feast-dev:masterfrom
haoxu0:test/cpu-torch-hashes-cpu-pin

Conversation

@haoxu0

@haoxu0 haoxu0 commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

#6906 taught add_cpu_hashes to leave an existing +cpu pin alone, which is what make lock-python-dependencies-all needed to get past:

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

That behaviour is still untested, and the gap is the reason the target could ship unable to run at all.

Both existing tests pass a bare torch==2.13.0 as the requirements being patched, and put +cpu only in cpu_requirements — the lookup table. So neither exercises a +cpu entry on the side that gets rewritten, which is exactly the input a real resolve produces:

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

What is added

Two tests, no production code:

  • one for a single marker split
  • one for the torch + torchvision pair the committed py3.*-ci-requirements.txt actually contain

Both assert idempotency. The second also asserts the +cpu line is left exactly as the resolver wrote it rather than merged into, so a later change cannot widen the set of artifacts that pin accepts without failing here — that property is the reason skipping is preferable to normalising the lookup key, and nothing currently holds it in place.

Verification

sdk/python/tests/unit/infra/scripts/test_cpu_torch_hashes.py: 4 passed. They pass against #6906's fix as merged, with the script untouched — so they describe the behaviour rather than one implementation of it.

ruff check and ruff format --check clean.

I had opened #6959 proposing the same fix before #6906 merged; the approach there was identical (return match[0] for a +cpu pin), and #6906 additionally normalises pins in main(), which mine did not. I have closed #6959 in favour of this, which keeps only the part that is still missing.

@codecov-commenter

codecov-commenter commented Oct 8, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' 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.92%. Comparing base (af20fe4) to head (bb1dc75).
⚠️ Report is 8 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6980   +/-   ##
=======================================
  Coverage   49.92%   49.92%           
=======================================
  Files         443      443           
  Lines       55511    55511           
  Branches     8096     8096           
=======================================
  Hits        27714    27714           
  Misses      25874    25874           
  Partials     1923     1923           
Flag Coverage Δ *Carryforward flag
go-feature-server 30.58% <ø> (ø)
python-unit 51.35% <ø> (ø) Carriedforward from af20fe4

*This pull request uses carry forward flags. Click here to find out more.


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 725a147...bb1dc75. 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.

feast-dev#6906 taught `add_cpu_hashes` to leave an existing `+cpu` pin alone, which
is what `make lock-python-dependencies-all` needed to get past

    ValueError: Missing CPU hashes for torch==2.14.1+cpu

That behaviour is still untested. Both existing tests pass a bare
`torch==2.13.0` as the requirements being patched and put `+cpu` only in
the lookup table, so neither exercises a `+cpu` entry on the side that
gets rewritten -- which is why the lock target could ship unable to run.

Two tests are added for the shape a real `--universal --torch-backend cpu`
resolve emits:

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

one for a single package and one for the torch and torchvision pair the
committed locks actually contain. Both assert idempotency, and the second
asserts the `+cpu` line is left exactly as the resolver wrote it rather
than merged into, so a future change cannot widen the set of artifacts
that pin accepts without failing here.

No production code is touched; these pass against feast-dev#6906's fix as merged.

Signed-off-by: hao-xu5 <hxu44@apple.com>
@ntkathole
ntkathole force-pushed the test/cpu-torch-hashes-cpu-pin branch from 65840a8 to bb1dc75 Compare October 9, 2026 07:37
@ntkathole
ntkathole merged commit bb7bb55 into feast-dev:master Oct 9, 2026
17 of 23 checks passed
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.

3 participants