Extract the module graphs through a staging directory - #5
Merged
Conversation
Nextflow dispatches one BLIMMP task per genome, so several processes extract the module graphs into the same directory at once. On a 9-genome run, 4 of 8 tasks died with [FATAL] Failed to extract KEGG_Graphs_Generated_March26.zip before running any analysis. This commit adds the test without the fix, so CI records the failure first. The existing smoke test runs a single process and stays green throughout, which is why the bug shipped.
Nextflow dispatches one BLIMMP task per genome, so several processes extract the module graphs into the same directory at once. Extracting straight into that directory let them destroy each other's work: one process would rmtree __MACOSX or flatten the nested folder while another was still reading from it. The OSError that followed looked to the caller like an unwritable destination, so it moved on to the next candidate and then reported that all of them had failed. Extraction now happens in a private staging directory that is renamed into position. A rename onto a missing or empty directory is atomic on POSIX, so the first process to finish wins and the others adopt its copy instead of failing. The test added in the previous commit went from 6 of 8 workers dead to all 8 agreeing on one directory with 340 graphs and no staging directories left behind. 0.1.4 makes this unreachable in the published image, since the graphs ship pre-extracted and the extraction path is never entered. It still runs for anyone installing from source or from a wheel. Implemented with assistance from Claude (Opus 5)
ensure_module_graphs prints a progress line when it does the extracting, so the worker that wins the race emits two lines and the others emit one. Comparing whole stdout blobs made the workers look like they disagreed when they had all returned the same directory.
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
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.
Concurrent BLIMMP processes could not extract the module graphs at the same time. On a
9-genome Nextflow run, 4 of 8 tasks died before any analysis ran:
Nextflow dispatches one BLIMMP task per genome, so the tasks landed on one node together
and all extracted into the same directory.
_extract_module_graphs()extracted in placeand then mutated the result, removing
__MACOSXand flattening the nested folder. Oneprocess would delete or move files while another was still reading them. The
OSErrorthat followed looked to the caller like an unwritable destination, so it tried the next
candidate and then reported that every candidate had failed.
Extraction now happens in a private staging directory that is renamed into position. A
rename onto a missing or empty directory is atomic on POSIX, so the first process to
finish wins and the others use its copy instead of failing.
Commits
The first commit adds the test without the fix, so CI records the failure before the fix
lands:
3401adatest only1efc959fixbc68edaassertionScope
0.1.4 makes this unreachable in the published image, because the graphs ship pre-extracted
and the extraction path is never entered. This matters for source and wheel installs, and
for any future image that stops pre-extracting. No release is needed to keep the container
behaving correctly.
The existing smoke test runs one process and stayed green through all of it, which is how
the bug shipped.
tests/concurrency_test.pyruns 8 workers against one directory andchecks that they all exit cleanly, agree on the result, and leave no staging directories
behind.