Repository navigation
Conversation
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>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6980 +/- ##
==========================================
- Coverage 49.49% 49.48% -0.01%
==========================================
Files 443 443
Lines 55451 55451
Branches 8085 8085
==========================================
- Hits 27443 27442 -1
Misses 26110 26110
- Partials 1898 1899 +1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This branch has not been deployed
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#6906 taught
add_cpu_hashesto leave an existing+cpupin alone, which is whatmake lock-python-dependencies-allneeded to get past: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.0as the requirements being patched, and put+cpuonly incpu_requirements— the lookup table. So neither exercises a+cpuentry on the side that gets rewritten, which is exactly the input a real resolve produces:What is added
Two tests, no production code:
py3.*-ci-requirements.txtactually containBoth assert idempotency. The second also asserts the
+cpuline 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 checkandruff format --checkclean.I had opened #6959 proposing the same fix before #6906 merged; the approach there was identical (
return match[0]for a+cpupin), and #6906 additionally normalisespinsinmain(), which mine did not. I have closed #6959 in favour of this, which keeps only the part that is still missing.