Skip to content
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

Replace ":" in singularity image name with "_" #342

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

sameeul
Copy link
Contributor

@sameeul sameeul commented Dec 7, 2024

Currently if a CommandLineTool's DockerRequirement lacks a tag and we use Singularity as the container engine, there is a mismatch between the name of the saved singularity container and the normalized name cwltool looks for. Here is an example. Assume the following section in a CommandLineTool:

hints:
  DockerRequirement:
    dockerPull: someuser/somecontainer

Now extract_docker_requirements modifies the dockerPull value to someuser/somecontainer:latest.
Then if Singularity is used as the container engine, SingularityImagePuller saves the image as someuser_somecontainer_latest.sif. This is due to the code here

However, in cwltool, when Singularity looks for the image, it looks for someuser_somecontainer.sif. This is due to the _normalize_sif_id function here

Due to this naming mismatch, cwltool fails to find the image and errors out.
Since the docker pull documentation says that if no tag is specified it always pulls the latest and singularity pull also behaves the same way (I could not find any explicit documentation though), I propose to remove this on the fly modification.

I am also open to other suggestions.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.30%. Comparing base (0fe3ca6) to head (f0fc783).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #342   +/-   ##
=======================================
  Coverage   33.30%   33.30%           
=======================================
  Files          29       29           
  Lines       34864    34864           
  Branches     9396     9396           
=======================================
  Hits        11613    11613           
  Misses      20615    20615           
  Partials     2636     2636           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mr-c
Copy link
Member

mr-c commented Dec 9, 2024

Hello @sameeul and thanks for this PR.

I checked singularity version 4.1.5, and here is how it natively names .sif files for containers without a tag

singularity pull docker://tensorflow/tensorflowtensorflow_latest.sif

Wouldn't it be better if we taught cwltool to also look for _latest.sif if there was no tag present? And to look for images with the repo name in them as well?

Likewise, modify cwl-utils to replace : with _?

That way both tools would work with each other, and better match how singularity itself works?

@sameeul
Copy link
Contributor Author

sameeul commented Dec 9, 2024

@mr-c :

Wouldn't it be better if we taught cwltool to also look for _latest.sif if there was no tag present? And to look for images with the repo name in them as well?

Yes, that should also work. I was not sure which fix was better but now that you pointed out singularity pull behavior with missing tags, I think modifying _normalize_sif_id in cwltool is better.

Likewise, modify cwl-utils to replace : with _?

So, negate this current commit and change CHARS_TO_REPLACE = ["/"] to CHARS_TO_REPLACE = ["/", ":"] here?

@mr-c
Copy link
Member

mr-c commented Dec 9, 2024

@mr-c :

Wouldn't it be better if we taught cwltool to also look for _latest.sif if there was no tag present? And to look for images with the repo name in them as well?

Yes, that should also work. I was not sure which fix was better but now that you pointed out singularity pull behavior with missing tags, I think modifying _normalize_sif_id in cwltool is better.

👍🏻

Likewise, modify cwl-utils to replace : with _?

So, negate this current commit and change CHARS_TO_REPLACE = ["/"] to CHARS_TO_REPLACE = ["/", ":"] here?

Yes, exactly!

@sameeul
Copy link
Contributor Author

sameeul commented Dec 9, 2024

Related PR: common-workflow-language/cwltool#2085

@sameeul sameeul changed the title Do not add 'latest' tag if no tag is present Replace ":" in singularity image name with "_" Dec 9, 2024
@mr-c mr-c enabled auto-merge (squash) December 10, 2024 07:57
@mr-c mr-c merged commit beccfa6 into common-workflow-language:main Dec 10, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants