Fix Python scratch-directory handling under parallel ctest - #3427
Merged
Conversation
Python temporary-file naming now mirrors the C++ scheme. Piped image data is no longer placed within a command's scratch directory, where it risked being erased along with that directory before the receiving command had read it. NO_FILESYSTEM_LOCK is no longer applied to any Python command test, nor even parsed when registering them: every Python command creates a scratch directory in the working directory, which the wildcard cleanup of a concurrently-running test would wipe; a single comment records that this divergence from the C++ tests is deliberate. That wildcard removal of temporary content between tests is retained, since failed Python tests would otherwise accumulate scratch directories and bloat the filesystem. The mrtrix_cleanup tests are repaired. Prompts: 1. > In another Claude session, a flaw has been identified in recent augmentations of the parallel execution of tests in ctest. Most MRtrix3 Python commands will create a scratch directory to store intermediate data. This scratch directory is by default placed in the working directory (as some commands may exceed capacity of a RAM-backed /tmp filesystem). Its name is composed as "<cmdname>-tmp-??????", where "?" is a random alphanumeric. For tests that involve interaction with the filesystem, the working directory is wiped of any temporary content, including a wildcard that captures such paths. The issue encountered is that while some Python command tests have been flagged as NO_FILESYSTEM_LOCK due to not generating any derivative files or passing them down a pipe, they will still nevertheless create a scratch directory. So if another test is run in parallel that does involve filesystem access, that test may wipe the active scratch directory of a test that claims to not require filesystem use. > Possible rectifications: > 1. Do not mark any Python command tests that create a scratch directory as NO_FILESYSTEM_LOCK; safe, but precludes parallelism. > 2. Modify within the ctest execution process modification of the environment variable that sets the location of Python script scratch directories, so that they are created in a location that avoids the wildcard temporary deletion of other tests. > 3. Remove from the deletion of pre-existing temporary data from the test directory the wildcard that deletes Python command scratch directories. > Claude to generate any other plausible proposals. > Adjudicate candidate solutions and make recommendation based on consequences. 2. > Cleanup of *-tmp-* during testing is to remain in place: may have many failed Python command tests cause filesystem bloat. > 1. Fix utils.name_temporary() to mirror C++. > 2. Remove any attempt to place piped image data in the scratch directory. It would endanger erasure of the piped image when the scratch directory is deleted before the receiving command reads from it. > 3. Remove use of NO_FILESYSTEM_LOCK for any Python command. Do not even parse it in the cmake command that creates each test. Only leave behind one brief comment stating that its omission relative to C++ tests is deliberate based on scratch directory formation. > 4. Comment on whether Option 4 could parse the Bash test file, cross-referencing the test content with the content of the test data repository to discover the subset of symlinks that would need to be generated, and whether this would actually have any net benefit over creating a symlink for each and every item in the test data repository. Additionally comment on other candidate approaches for enabling parallel execution of tests, eg. modifying all tests to reference a data directory at a specified environment variable location, giving a unique name to every temporary output, et cetera. Bear in mind that some of the test suite design is carried over from a legacy bespoke design that did not conform to best practises. > 5. Fix the mrtrix_cleanup tests. Generated-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
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.
Extended #33424 in rectifying #3380.
For some Python command tests, the new ability of Python commands to utilise piped input / output images was used as justification to flag those tests with
NO_FILESYSTEM_LOCK. This is however inappropriate: those commands will still typically generate a scratch directory, for which the default location is the current working directory, which will be wiped out byrm -rf *-tmp-*/being executed by other tests in parallel. So the safest option is to just say that all Python tests require a filesystem lock and therefore cannot execute in parallel (technically some Python scripts don't need it as they don't create a scratch directory, but these are rare, and I don't think the maintenance burden is worthwhile).Longer-term, a better solution might be for each test to create its own scratch directory, linking all contents of the test data directory so that the contents of the tests themselves don't need to be modified. That way any two tests could operate in parallel.
Some other fixes that Claude caught along the way:
Previously piped images were intended to go into the scratch directory of that command, but this apparently never worked as that scratch directory was always created after this decision was made.
mrtrix_cleanuptests were non-functional.