fix(cache): make task fingerprints deterministic and collision-free - #1207
Open
AxelNoun wants to merge 1 commit into
Open
fix(cache): make task fingerprints deterministic and collision-free#1207AxelNoun wants to merge 1 commit into
AxelNoun wants to merge 1 commit into
Conversation
set_task derived its cache key from json.dumps(vars(task), default=str). The default=str fallback silently stringifies anything not JSON-serialisable, which is neither injective (str() truncates numpy arrays past 1000 elements) nor stable across processes (set ordering depends on PYTHONHASHSEED, and default reprs embed memory addresses). Two different task configurations could therefore share a cache directory, and identical configurations could fail to reuse one. Replace it with an explicit canonicaliser in pyhealth/tasks/fingerprint.py that raises on anything it cannot represent deterministically, records effective __init__ arguments with defaults applied, covers class-level configuration, and adds an explicit BaseTask.version for logic changes. Also fixes the same bug in the processors cache key, and slugifies task_name before using it as a path component. Cache keys change, so existing task caches under <cache_dir>/tasks are orphaned. This is intentional: some of them are wrong. Refs sunlabuiuc#916 Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Summary
The literal request in #916 is already satisfied on current master:
set_taskalready hashesvars(task)(init args as instance attributes) together with the input/output schemas. This PR does not add that hashing; it replaces the mechanism.json.dumps(..., default=str)is neither injective (str()truncates numpy arrays past 1000 elements) nor stable across processes (PYTHONHASHSEEDreorders sets; defaultrepr()embeds memory addresses). Two different task configurations could share a cache directory, and identical configurations could fail to reuse one. The same bugs apply to the processors cache key.That is why the diff is a typed canonicaliser rather than a one-line hash tweak: nine concrete failure modes (silent collisions, non-determinism, mixed-key
TypeError, class-level config invisible tovars(), no code version, unsanitisedtask_namepath component, and tests that reimplemented the hashing expression). Keep Refs #916 rather than Closes — the original ask was already in place; whether the issue should close is a maintainer call.pyhealth.tasks.fingerprint: deterministic, lossless fingerprints; raisesUnfingerprintableErrorinstead of silently hashingrepr().__init__arguments with defaults applied (Task()≡Task(window=15d)when that is the default), class-level config on the MRO, and an explicitBaseTask.versionfor logic changes.fingerprint_excludedenylist for non-semantic args (num_workers,verbose, …).task_meta.jsonsidecar next to the cache so an opaque digest is auditable;set_tasklogs its path.OSError(they run outside the build lock).CrossMap/ mixed-keycode_vocab).task_nameis slugified before it is used as a path component (BenchmarkEHRShot/guo_los).versionmust be bumped when__call__changes,fingerprint_exclude,UnfingerprintableError.Breaking
Cache only.
FINGERPRINT_VERSION = 2changes every key, orphaning existing<cache_dir>/tasksdirectories. They are not deleted. This is deliberate: an unknown subset of them is wrong — findings 3, 5 and 7 mean two different task configurations could share a directory — and nothing distinguishes the good ones from the bad. Invalidating all of them is the only safe option. Changelog should suggestdu -sh ~/.cache/pyhealthbefore upgrading.API, potential. Strict mode now raises
UnfingerprintableErroron an argument that cannot be fingerprinted, where the old code produced a silent key.PYHEALTH_FINGERPRINT_STRICT=0restores the fallback with a warning. Review call: invert the default for a transition release?Open questions for maintainers
versionvs source hash.PYHEALTH_FINGERPRINT_SOURCE=1folds a structural (AST) hash of__call__/pre_filterinto the key. Off by default: renaming a local variable should not invalidate a 40-minute cache. A version bump at review time is more predictable, but it depends on human discipline.PurePathis hashed as-is, so a cache is not portable across machines if a path appears in the args. That is probably the right default (a different root is a different config) but it should be explicit in the docs.pyhealth.datasets.prune_task_caches()to list/delete orphaned directories, or leave that to the user?Test plan
pytest tests/core/test_task_fingerprint.py -q— 28 passedpytest tests/core/test_caching.py -q— 11 passed on Linux; Windows teardownWinError 32from litdata file locks is pre-existing on master (identical 6 passed / 5 failed / 1 error on unmodified0a75f99)PYTHONHASHSEEDin{1, 42, 999}— identical fingerprints.tmp, validtask_meta.jsontools/check_pr_rules.pyagainst0a75f99Refs #916