Repository navigation
Conversation
Currently clean_tmpdir() removes the whole cache_dir/tmp, so when one job finishes a build, it deletes the temp folders of another job which is still building on the same cache_dir. Now each dataset keeps the list of temp folders it created and removes only them, and tmp/ itself is removed only when it's empty. rmtree uses ignore_errors=True, because it runs in finally blocks and a cleanup error should not hide the build's own error. Added 4 tests (TestTmpDirOwnership), a paragraph in docs/api/datasets.rst and examples/dataset_cache_shared_tmpdir.py.
This branch has not been deployed
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
1 participant
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.
Contributor: Kostiantyn Dementiev (konstde00@gmail.com)
Contribution Type: Bug fix
Description:
clean_tmpdir()removed the wholetmp/folder, so a job could delete another job's temp folders on the samecache_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.rstexamples/dataset_cache_shared_tmpdir.pyPart of #1295.
create_tmpdir()records each folder it creates, andclean_tmpdir()removes only those, thentmp/if it's empty.rmtreeusesignore_errors=True, because it runs infinallyand a cleanup error should not hide the build's own error. The layout (tmp/<uuid>) is unchanged, soFHIRDatasetand_proc_transformneed 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.pypasses, and the fulltests/coresuite passes with both fixes from #1295 applied (1,525 OK, 96 skipped).