[konstde00]

Contributor: Kostiantyn Dementiev (konstde00@gmail.com)
Contribution Type: Bug fix
Description: clean_tmpdir() removed the whole tmp/ folder, so a job could delete another job's temp folders on the same cache_dir. Now each dataset removes only the temp folders it created.
Files to Review:

• pyhealth/datasets/base_dataset.py (__init__, create_tmpdir, clean_tmpdir)
• tests/core/test_caching.py (TestTmpDirOwnership, 4 tests)
• docs/api/datasets.rst
• examples/dataset_cache_shared_tmpdir.py

Part of #1295.

create_tmpdir() records each folder it creates, and clean_tmpdir() removes only those, then tmp/ if it's empty. rmtree uses ignore_errors=True, because it runs in finally and a cleanup error should not hide the build's own error. The layout (tmp/<uuid>) is unchanged, so FHIRDataset and _proc_transform need no edits.

Behaviour change: folders left by killed jobs are no longer removed by another job's cleanup (question 3 in #1295).

Testing (macOS, Python 3.13.7): the 4 new tests pass (3 of them fail on master), the 30 existing cache tests pass, tools/check_pr_rules.py passes, and the full tests/core suite passes with both fixes from #1295 applied (1,525 OK, 96 skipped).

[github-actions]

Test Coverage

Total: 67.90% (16704 / 24600 lines)

Modified file	Coverage	Lines
pyhealth/datasets/base_dataset.py	90.82%	465 / 512

Workflow run · a9ccc2d

[Logiquo]

/review

[solarsys]

/review

[github-actions]

AI review: findings

This PR makes each BaseDataset instance record the temp folders it creates in create_tmpdir() and remove only those in clean_tmpdir(), then remove tmp/ only if it is empty, so jobs sharing a cache_dir no longer delete each other's converted tables or Dask scratch. Existing callers (_event_transform, _proc_transform, _scan_csv_tsv_gz, and FHIRDataset._ensure_prepared_tables) keep the same call pattern, and every dataset subclass calls super().init, so _tmp_dirs is always set. No blocking problems found. Non-blocking: a narrow race where another job's rmdir of an empty tmp/ can make a concurrent create_tmpdir() fail with FileNotFoundError (base_dataset.py:560); rmtree errors are swallowed without a log (base_dataset.py:557); the docs don't say that folders left by killed jobs now persist and keep tmp/ from being removed (docs/api/datasets.rst:41); and the failure-path test doesn't check that the failing job's own folders are removed (tests/core/test_caching.py:363). The four new tests cover the cross-instance cases and tmp/ removal, and the example runs the same scenario. I did not run the tests. No existing component duplicates this; it changes existing methods. Overall risk is low.

_{Reviewed commit a9ccc2d}

[github-actions]

[low] Race between tmp/ removal and a concurrent create_tmpdir()

create_tmpdir() calls mkdir(parents=True). When tmp/ is missing, pathlib creates tmp/ and then the child directory in two separate steps. If another job on the same cache_dir runs this rmdir on the still-empty tmp/ between those two steps, the child mkdir raises FileNotFoundError and that job's build fails. The window is very small, but it sits in exactly the shared-cache case this PR is meant to fix. Fix: in create_tmpdir(), retry the mkdir once or in a short loop when FileNotFoundError is raised, or create the directory with a helper that tolerates tmp/ disappearing.

[github-actions]

[low] rmtree errors are silently ignored

ignore_errors=True drops any failure to delete a temp folder without a log line. Leftover folders then accumulate in the shared cache with no trace of why. The ignore_errors choice is reasonable because this runs in finally blocks. Fix: keep the non-raising behavior but log a warning on failure, for example with onerror or a try/except OSError around rmtree.

[github-actions]

[low] Docs omit that folders from killed jobs now persist

Before this change, the next successful cleanup removed all of tmp/, including folders left by killed jobs. Now those folders stay, and they keep tmp/ itself from being removed. The PR description states this behavior change, but the user-facing docs don't. Fix: add a sentence saying that folders left by interrupted runs are not removed automatically and can be deleted by hand.

[github-actions]

[low] Failure-path test does not check the failing job's own folders are removed

test_failed_build_keeps_other_datasets_tmpdirs makes load_data raise before this dataset creates any temp folder. So it only checks that the other job's file survives, and never runs the finally cleanup of this job's own folders. The ignore_errors path is also untested. Fix: have the patched load_data call self.mine.create_tmpdir() and then raise, and assert that this folder is gone while the other job's file remains.

[github-actions]

AI Review

suggestion: human review
decision: not available

Haiku found that this change needs a human review: The change modifies BaseDataset directly: init, create_tmpdir and clean_tmpdir. The behavior of the public clean_tmpdir() changes from removing all of tmp/ to removing only the instance's own folders. Every dataset build path (_event_transform, _proc_transform, _scan_csv_tsv_gz) and FHIRDataset._ensure_prepared_tables depends on these methods.

_{Reviewed commit a9ccc2d}

[solarsys]

Approving. I reproduced the bug on master. When two datasets share a cache directory, the first one to finish removes tmp/ and deletes the other one's in-progress temp files. With this PR, each instance removes only the folders it created. test_caching, test_base_dataset, test_fhir_dataset, test_dataset_cache_key and test_default_cache_dir pass locally, and the new tests fail on master as described.

The AI review's low findings can be follow-ups. The most useful is a sentence in the docs saying that folders left by interrupted runs are no longer removed automatically. Thanks @konstde00.