fix: keep params named size or nfiles in the stage hash - #11079
Open
adarshsm wants to merge 1 commit into
Open
Conversation
size and nfiles are file metadata and are excluded from the run-cache hash, but the exclusion was applied to the whole lockfile. A parameter with either name was therefore dropped before hashing, so changing it left the hash untouched: the stage was reported as cached, the outputs were checked out from the previous run, and the new parameter value silently never took effect. Those fields only ever appear under deps and outs, so restrict the exclusion to those two sections. A stage without such a parameter hashes exactly as before, leaving existing run-cache entries valid. Fixes treeverse#10296
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11079 +/- ##
==========================================
+ Coverage 90.68% 90.98% +0.30%
==========================================
Files 504 505 +1
Lines 39795 41153 +1358
Branches 3141 3263 +122
==========================================
+ Hits 36087 37443 +1356
- Misses 3042 3071 +29
+ Partials 666 639 -27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Fixes #10296.
Problem
A parameter named
sizeornfilesis invisible to the run-cache, so changing it does notre-run the stage and the outputs from the previous run are checked out instead. Reproduced
on current
main:The failure is silent, which is what makes it nasty — the stage reports as cached and the
outputs look valid.
Cause
_get_cache_hash()excludessizeandnfilesbecause they are file metadata rather thanpart of the stage definition, but it passes the exclusion to
dict_sha256()for the wholelockfile, and
dict_filter()is recursive:The lockfile has four top-level sections —
cmd,deps,params,outs— and metadataonly ever appears under
depsandouts.paramsholds arbitrary user keys, so aparameter literally named
sizeornfileswas stripped along with the metadata and neverreached the hash.
This matches @skshetry's reading on the issue: "We are recursively excluding
nfilesandsizebefore hashing for stage cache, which is incorrect. Most likely, we'll be able toremove
sizeandnfilesonly from outputs that are not parameter dependencies."Fix
Apply the exclusion to the
depsandoutssections only.Existing run-cache entries stay valid. For a stage without a parameter of either name,
the filtered dictionary is identical to what the old code produced, so the hash is
unchanged. Only stages that were mis-hashed get a new hash, which is the point.
Verified directly:
size: 30vssize: 40, old codesize: 30vssize: 40, new codesizeunderdeps/outschangedAnd end to end, on the reproduction above:
sizechanges now re-run the stage andout.txttracks the parameter, while an unchanged pipeline still reportsData and pipelines are up to date.Tests
Two unit tests in
tests/unit/stage/test_cache.py:test_cache_hash_tracks_params_named_like_meta— the regression. Fails onmain(the two hashes are equal), passes here.
test_cache_hash_still_excludes_deps_and_outs_meta— guards the behavior that must notchange; passes both before and after.
tests/unit/stage/also reports two failures in my environment(
test_fill_from_lock_use_appropriate_checksum,test_dump_nondefault_hash), bothImportError: s3 is supported, but requires 'dvc-s3' to be installed. They are unrelated:the failing test ids are identical with and without this change.
ruff check,ruff format --checkandmypy dvc/stage/cache.pyare clean.Note on scope
@TimCosemans reported the same symptom on the issue for parameters named
time_dummiesandmethod. Those names are not excluded anywhere in the hashing path, so I believe that is adifferent problem and have deliberately left it alone rather than widen this change; happy
to look at it separately if it is still reproducible.