Skip to content

fix(cache): make task fingerprints deterministic and collision-free - #1207

Open
AxelNoun wants to merge 1 commit into
sunlabuiuc:masterfrom
AxelNoun:fix/task-fingerprint-916
Open

fix(cache): make task fingerprints deterministic and collision-free#1207
AxelNoun wants to merge 1 commit into
sunlabuiuc:masterfrom
AxelNoun:fix/task-fingerprint-916

Conversation

@AxelNoun

Copy link
Copy Markdown
Contributor

Summary

The literal request in #916 is already satisfied on current master: set_task already hashes vars(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 (PYTHONHASHSEED reorders sets; default repr() 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 to vars(), no code version, unsanitised task_name path 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.

  • New pyhealth.tasks.fingerprint: deterministic, lossless fingerprints; raises UnfingerprintableError instead of silently hashing repr().
  • Records effective __init__ arguments with defaults applied (Task()Task(window=15d) when that is the default), class-level config on the MRO, and an explicit BaseTask.version for logic changes.
  • fingerprint_exclude denylist for non-semantic args (num_workers, verbose, …).
  • task_meta.json sidecar next to the cache so an opaque digest is auditable; set_task logs its path.
  • Sidecar writes use a per-writer temp name, skip rewrite on cache hit, and never abort a build on OSError (they run outside the build lock).
  • Processor cache key uses the same canonicaliser (covers CrossMap / mixed-key code_vocab).
  • task_name is slugified before it is used as a path component (BenchmarkEHRShot/guo_los).
  • Docs: what counts as the same configuration, version must be bumped when __call__ changes, fingerprint_exclude, UnfingerprintableError.

Breaking

Cache only. FINGERPRINT_VERSION = 2 changes every key, orphaning existing <cache_dir>/tasks directories. 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 suggest du -sh ~/.cache/pyhealth before upgrading.

API, potential. Strict mode now raises UnfingerprintableError on an argument that cannot be fingerprinted, where the old code produced a silent key. PYHEALTH_FINGERPRINT_STRICT=0 restores the fallback with a warning. Review call: invert the default for a transition release?

Open questions for maintainers

  1. version vs source hash. PYHEALTH_FINGERPRINT_SOURCE=1 folds a structural (AST) hash of __call__ / pre_filter into 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.
  2. Absolute paths. PurePath is 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.
  3. Cache migration. Should we ship 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 passed
  • pytest tests/core/test_caching.py -q — 11 passed on Linux; Windows teardown WinError 32 from litdata file locks is pre-existing on master (identical 6 passed / 5 failed / 1 error on unmodified 0a75f99)
  • PYTHONHASHSEED in {1, 42, 999} — identical fingerprints
  • Concurrent sidecar writes (24 threads) — no leftover .tmp, valid task_meta.json
  • tools/check_pr_rules.py against 0a75f99

Refs #916

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant