Singularity23 fix #1113
Singularity23 fix #1113
Conversation
|
Can one of the admins verify this patch? |
|
@mr-c here is the new PR whenever you get some time. Let me know if you need anything from me! |
|
Jenkins, Okay to test |
|
Jenkins, okay to test |
Co-Authored-By: drkennetz <38996870+drkennetz@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #1113 +/- ##
==========================================
+ Coverage 73.26% 78.15% +4.89%
==========================================
Files 34 34
Lines 6878 6890 +12
Branches 1750 1753 +3
==========================================
+ Hits 5039 5385 +346
+ Misses 1310 1058 -252
+ Partials 529 447 -82
Continue to review full report at Codecov.
|
|
We will likely have access to singularity2.6 on our cluster in the next few days. I can work on version specific code if that seems like something you'd like to have. I will run some tests to see if Singularity3.x is backwards compatible with 2.x. If it is, I can write code to allow sing3.x to run 2.x and 3.x images, while limiting 2.x to Sing2.x specific images. After diving into this a bit more I am going to put this on hold. This would be relevant if a user is switching between Singularity2.x and 3.x to run the same tool which is bad practice. I think down the road, I can add a feature that will validate $SINGULARITY_CACHEDIR based on a given version, but in the mean time it is not an immediate concern. |
…n env and to find it in singularity_pullfolder if in env.
…ool-singularity23-fix into singularity23_fix necessary to perform a PR
|
I'm learning a lot of git! I pulled the changes you made locally and then added my tests and pushed them now. The tests I added should test for images to be created in SINGULARITY_PULLFOLDER and locally depending on if the environmental variable exists. We shall see. |
|
It seems that the singularity_pull_folder test failed, perhaps I don't understand what get_data() does, but I figured it just ran the cwltool I passed it and would output anything that the tool would output. The tool writes an image file and that's it. Inside of the test directory I created a singularity_pullfolder and set it in env, and then checked to see if the .img was in the folder I designated as pull_folder. Let me know if you see anything wrong with the test! |
tests/test_singularity.py
Outdated
| assert 'debian.img' in file_in_dir | ||
|
|
||
| @needs_singularity | ||
| def test_singularity_pullfolder(): |
There was a problem hiding this comment.
Can you investigate why test_singularity_local() succeeds in this fails? I tested this locally and it generated a dir called 'tmp' im my current working directory and wrote an image file to it. The resultant
assert 'debian.img' in file_in_dir returned True in my local case.
|
Is the default branch failing because commits have been made? |
|
@drkennetz No, that was a transient error on the Jenkins server |
|
Almost there! |
|
I will let you resolve the get_data issue then we can see if this will increase cov. Thanks for your help! |
… This currently has no test coverage.
|
@mr-c have you had a chance to review this at all? I believe I am waiting on your update to the get_data() function to increase coverage. After that, PR complete? |
|
In the latest commit I've changed how the images are named, only This way same named images from different organizations or registries aren't mistaken for each other. @drkennetz This is different than Thia probably means that we shouldn't use the |
|
@mr-c I think that is a good idea. I don't think you should strip any part of the name to keep them as unique as possible. However, I don't know why that would conflict with Singularity by default writes images to
However, you have changed this behavior by including an image name and a write location to singularity image files in Maybe I am misunderstanding you or what you are trying to change. I have a pretty intimate understanding of how cwl and singularity interact. Maybe you could tell me what you are trying to have as default behavior and I could comment on that or provide possible coding changes. |
|
@mr-c I think I misunderstood you the first time. If you guys want to change Maybe we could bring it up in the next meeting. |
|
Seems that Singularity 3.1 and 3.2 don't use the SINGULARITY_PULLFOLDER environment variable 😿 |
I see the changes you've made and they look good and work locally for me for both singularity 2.6 and 3.x once I set CWL_SINGULARITY_CACHE in my env. In the end, SINGULARITY_PULLFOLDER is behaving the same way as CWL_SINGULARITY_CACHE would for 3x. |
I added a fun
_normalize_sif_id()which creates a regex based on how singularity3.x will write out image files.Also added logic which will search for .img or .sif in $SINGULARITY_CACHEDIR and $SINGULARITY_PULLFOLDER only if they exist in users env. If they do not exist, the file will be written locally (this is how you previously had it). If the cwltool is run again, it will use the local copy if it is found in any of the locations
pwd,$SINGULARITY_CACHEDIR, or$SINGULARITY_PULLFOLDER. If those are not in env, the image must be in the same directory as the tool in order to be found.(continuation of #1111)
It is also important to note that any images or sif's generate using singularity ad hoc (without cwl) that follow the singularity naming convention will also be found if they are written to these directories, which I think is desirable behavior.