Prevents files from being staged twice #43
Merged
Conversation
This file contains 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
BWMac
changed the title
Codecov Report
@@ Coverage Diff @@
## main #43 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 1150 1164 +14
Branches 189 192 +3
=========================================
+ Hits 1150 1164 +14
|
thomasyu888
reviewed
Jul 21, 2023
thomasyu888
reviewed
Jul 21, 2023
thomasyu888
approved these changes
Jul 24, 2023
| @@ -62,6 +64,27 @@ def test_that_a_local_temporary_path_is_created_when_staging_a_remote_file(test_ | |||
| assert remote_file.local_path == staged_path | |||
|
|
|||
|
|
|||
| def test_that_error_is_raised_when_a_file_has_been_staged_multiple_times(test_files): | |||
There was a problem hiding this comment.
Did we need a test for when the file already exists?
There was a problem hiding this comment.
Yes, and I also realized that now that we are preventing files from being staged more than once, that outcome is occurring after the first successful execution of stage in a test for a remote file (or when I create fake staged files in a test. I.e. we are not testing what we used to be testing after that first test. I will add a helper function to the tests that clears out the dcqc-staged-* directories to be used where necessary.
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.
This PR implements a fix for the bug first reported by @adamjtaylor regarding a name collision error in Nextflow Tower runs of
nf-dcqc. The error occurred because files were being staged twice in the Nextflow processworkdirectory. I identified the error as having to do with one failed Batch job attempt leading to a successful re-try which was presumable happening after the file was initially staged. So, after the second attempt succeeded there were two copies of the target file in theworkdirectory for the Nextflow process.In order to prevent this from happening in this and other edge cases, I have added the
file.already_stagedmethod, which looks in the temporary directory to see if the file has already been staged in adcqc-staged-*pattern-matched folder. If it has already been staged more than once somehow, it throws an error ahead of the inevitable Nextflow name collision error. If the file has been staged 1 or 0 times,already_stagedreturns a list of file paths.In
file.stage, if this list of file paths is empty the file has yet to be staged and the function continues as it has, staging the file. If it has already been staged, thePathto the previously staged file is returned.This change has been tested in local and tower runs of
nf-dqc. Unfortunately, because of the inconsistent and non-reproducible nature of the batch error which prompted this work it is difficult to be certain if it will prevent future issues.