Update container handling with charliecloud #5300
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
-use writeFake as default -run by name with writeFake and CH_IMAGE_STORAGE -run by path with writeFake when CH_IMAGE_STORAGE is not set -run by path without writeFake, but writeable, check that the path is not CH_IMAGE_STORAGE Signed-off-by: Niklas Schandry <[email protected]>
Signed-off-by: Niklas Schandry <[email protected]>
…GE_STORAGE is preferred, in other cases charliecloud.cacheDir over NXF_CHARLIECLOUD_CACHE over work/charliecloud, prevent usage of CH_IMAGE_STORAGE when not using writeFake Signed-off-by: Niklas Schandry <[email protected]>
Signed-off-by: Niklas Schandry <[email protected]>
✅ Deploy Preview for nextflow-docs-staging canceled.
|
Signed-off-by: Niklas Schandry <[email protected]>
Signed-off-by: Niklas Schandry <[email protected]>
phue
reviewed
Sep 12, 2024
How was it fixed? |
Signed-off-by: Niklas Schandry <[email protected]>
Trying to catch it and retry https://github.com/nextflow-io/nextflow/blob/master/modules/nextflow/src/main/groovy/nextflow/container/CharliecloudCache.groovy#L186 |
phue
approved these changes
Sep 12, 2024
|
Thanks for the helpful discussion and review @phue ! @pditommaso do you have any additional comments on this? |
pditommaso
reviewed
Sep 16, 2024
modules/nextflow/src/main/groovy/nextflow/container/CharliecloudCache.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: Niklas Schandry <[email protected]>
|
Thanks @pditommaso, I have put in the suggested change. |
|
Nice, let's wait for the tests |
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.
Here are changes to the charliecloud driver.
charliecloud.writeFakeis true by default (!)if
charliecloud.writeFakeis true and$CH_IMAGE_STORAGEis set, images will go into$CH_IMAGE_STORAGEand be run from there with --write-fake (will be run by name in that case).if charliecloud.writeFake is true, but
$CH_IMAGE_STORAGEis unset, images will go intocharliecloud.cacheDir,$NXF_CHARLIECLOUD_CACHEorwork/charliecloud(order of priority) and will be run by path (since run by name only works if$CH_IMAGE_STORAGEis set)if
charliecloud.writeFakeis set tofalse, images will need to be run with-wto enable mounting of the workDir. In this case$CH_IMAGE_STORAGEwill not be used to store images, since$CH_IMAGE_STORAGEis incompatible with-w. Images will instead go intocharliecloud.cacheDir,$NXF_CHARLIECLOUD_CACHEorwork/charliecloud(order of priority). If the folder used to store images is the same as$CH_IMAGE_STORAGE, throw an exception. Images will be run by path.In summary:
Give preference to
$CH_IMAGE_STORAGEin the default case ofwriteFakeMake some effort to avoid using
$CH_IMAGE_STORAGEwhenwriteFakeisfalse.No longer create temporary containers for each task.
This is the same as #5212, but without the many merge conflicts created by a failed sign-off.
@phue I have updated the documentation. I have not added a unit test for switching between
imageandimageNamein the builder, since I do not know of a way to set an env var for the tests. I have tested this locally (i.e. running tests with the var set or not) and it produced the expected results. Is it really worth adding a way to inject fake environments into the BuilderTest to test this specific behaviour?