-
-
Notifications
You must be signed in to change notification settings - Fork 237
feat: handle singularity/apptainer sandbox images #2166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: handle singularity/apptainer sandbox images #2166
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2166 +/- ##
==========================================
+ Coverage 85.13% 85.17% +0.04%
==========================================
Files 46 46
Lines 8368 8392 +24
Branches 1956 1960 +4
==========================================
+ Hits 7124 7148 +24
Misses 779 779
Partials 465 465 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you @natthan-pigoux ; can you run |
|
@mr-c , thank you to already have a look. I will be away next week, so I will probably not answer before 2nd November. |
35a7346 to
6f96b54
Compare
|
@mr-c , sorry I should have put this PR as Draft before ... but now it should be ready for review. The tests should cover quiet well the changes. |
c1f86f3 to
8bdc6f8
Compare
8bdc6f8 to
8aa63a8
Compare
|
Hello @mr-c , I have change a bit the tests so that the coverage should be happy. Can you run them and tell me if its good for you? |
8aa63a8 to
383c66b
Compare
|
What about now ? ^^ |
|
@natthan-pigoux Code coverage of the tests looks great, thank you! I'll investigate the mypy/mypyc issue |
mr-c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this checks with respect to the current working directory? What if the user wants to store their container sandbox image in another place? Where does apptainer/singularity put them? Should we re-use CWL_SINGULARITY_CACHE for this purpose?
I started from the statement that the sandbox image is already deployed somewhere accessible and that the user only wants to use it. Here I didn't implement any mechanism to "pull" a sandbox image. If that is of interest I can do it in another PR maybe? Is that what you were asking? The image path can be specified within |
No absolute paths and no relative paths that go "up"; we need to check for both and not allow them. They are bad for portability of the We need to decide on a default location for sandbox/unpacked singularity/apptainer images, and way for the user to specify an alternative root folder for those (command line option to |
Indeed that seems necessary. What would be nice is that this variable corresponds to the base path from where to get the image e.g., for example If we also add a cwltool cli option name could follow the same semantic I don't have a strong opinion on the naming of this env variable and cli option. Maybe |
Sure, both the current working directory plus whatever other environment variable or command line option we come up with should be checked.
Yes, but I'd like to re-use
How about |
Running
cwltoolwithdockerRequirements(with pull disabled) using a local Singularity/Apptainer sandbox image is not currently handle.cwltoolis only searching for files (e.g. sif images) and not path as it should for sandbox images.Here I provide:
singularity inspectto verify that a path is a singularity imageget_imagemethoddockerPullSome questions I have:
candidatessearch, I don't know if we wish thatsingularity inspectis strong enough to ensure the presence of the imageIt is the first time I contribute here so I might have miss some things :)
closes #2165