Skip to content

Key the dataset cache on the config and source files - #1265

Merged
jhnwu3 merged 1 commit into
sunlabuiuc:masterfrom
solarsys:feat/dataset-cache-key-content
Oct 2, 2026
Merged

jhnwu3 merged 1 commit into
sunlabuiuc:masterfrom
solarsys:feat/dataset-cache-key-content

Conversation

@solarsys

@solarsys solarsys commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Problem

The dataset cache key (BaseDataset._init_cache_dir) covered only {root, tables, dataset_name, dev}. So:

  • rewriting a source file at the same path (for example regenerating a cohort extract) silently reused the stale cached global_event_df;
  • editing the YAML config (attributes, joins, timestamp columns) also reused the stale cache.

A downstream EHR project hit both and had to delete caches by hand.

Change

The key now also includes:

  • a SHA-256 of the YAML config (DatasetConfig.model_dump);
  • for each requested table, [file, size, mtime_ns] of every source file it reads: the table file and its .csv/.csv.gz twin, joined files, glob matches, and the files under a Parquet directory (MEDS shards).

Design choices:

  • Size plus modification time, not a content hash, like make. Startup stays cheap for multi-GB sources. Copying files with new timestamps triggers a rebuild, which errs on the safe side.
  • URLs are keyed by the URL alone, so creating a dataset makes no network requests.
  • Datasets without a config are unchanged. MEDSDataset builds on the base key, and the FHIR dataset already hashes its YAML in its own key.

Scope

This is the dataset half. The task cache key (task code and attributes) is being reworked in #1207, so this PR doesn't touch it.

Upgrade note

Existing caches get a new key, so they're rebuilt once after upgrading. Old cache folders are left in place; the docs say they can be deleted.

Tests, docs, example

  • New tests/core/test_dataset_cache_key.py, on a real CSV and YAML in a temp folder:

    • a source rewritten at the same path is reloaded (3 → 5 patients, new cache folder);
    • an unchanged rerun reuses the cache;
    • an edited config gives a new cache;
    • a changed modification time gives a new cache.

    Three of these fail on master.

  • Existing base-dataset, caching and MEDS tests pass.

  • docs/api/datasets.rst ("How PyHealth Loads Data") explains what "the same configuration" means now.

  • New examples/dataset_cache_refresh.py: v1 has 2 patients; after rewriting the file, v2 has 4 patients in a fresh cache, and an unchanged rerun reuses it.

  • Full core suite: Ran 1385 tests … OK (skipped=76). tools/check_pr_rules.py passes.

This PR and #1264 (the default cache warning) edit different parts of docs/api/datasets.rst, so they merge in either order.

🤖 Generated with Claude Code

The dataset cache key covered only {root, tables, dataset_name, dev}.
Rewriting a source file at the same path, or editing the YAML config,
silently reused the stale cached event table.

- The key now also includes a SHA-256 of the YAML config and, for each
  requested table, [file, size, mtime_ns] of every source file it reads:
  the table file, its .csv/.csv.gz twin, joined files, glob matches, and
  files under a Parquet directory. Contents are not hashed, so startup
  stays cheap on large sources; URLs are keyed by the URL alone, so no
  network calls are made at init. Datasets without a config are unchanged.
- MEDSDataset builds on the base key and gets this too.
- tests/core/test_dataset_cache_key.py: a rewritten source is reloaded
  (3 -> 5 patients), an unchanged rerun reuses the cache, an edited
  config and a changed mtime give a new cache.
- docs/api/datasets.rst: what "same configuration" means for the cache.
- examples/dataset_cache_refresh.py.

Existing caches get a new key once after upgrading; old cache folders are
left in place.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@solarsys
solarsys requested a review from jhnwu3 October 2, 2026 13:08

@jhnwu3 jhnwu3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jhnwu3
jhnwu3 merged commit 85bfcf7 into sunlabuiuc:master Oct 2, 2026
2 checks passed
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.

2 participants