-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Avoid pulling existing Singularity image #177 #178
Conversation
When using Singularity and in case nodejs is not installed on your system this image is pulled from Docker every time. In case of offline processing and for often pull requests to Docker this should be avoided.
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.
Thanks @adrabent ! This would for us be an important feature to have, as we're hitting the pull rate limit because of the node:slim container pulling happening many times.
I have some nit suggestions to make the code a bit more clear -- it is a bit hard to grasp what happens currently.
cwl_utils/sandboxjs.py
Outdated
or len(dockerimgs.split("\n")) <= 1 | ||
or force_docker_pull | ||
): | ||
# pull node:slim docker container | ||
): # pull node:slim docker container |
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.
Did you accidentally move this comment to the previous row?
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.
Looks good to me! I don't know if a core developer also wants to review this?
This pull request introduces 1 alert when merging 61f83ed into 740615f - view on LGTM.com new alerts:
|
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.
Thank you @adrabent for this , and to @aroffringa for the reviews.
What is the testing plan for this change?
Hi @mr-c Nevertheless, I still encounter an error on the compute nodes which do not have any outbound internet access:
So still internet access is needed to run any CWL workflow using Javascript. On my supercomputer I can load Cheers, |
The node:slim container requiring network access is very odd and certainly not intended, do you think you could track down what is happening? |
Dear @tetron, I am not very familiar how these javascript checks in
The
Even if nodejs is not really using outgoing connections, but could it be it uses some local (network) protocols for the threading, which might be typically disabled on nodes in computing facilities for security reasons? -Alex |
@mr-c |
@adrabent Can you run That would fix https://github.com/common-workflow-language/cwl-utils/actions/runs/3496853174/jobs/5855251466#step:7:32 |
@mr-c Done. |
Codecov Report
@@ Coverage Diff @@
## main #178 +/- ##
==========================================
+ Coverage 37.30% 37.84% +0.53%
==========================================
Files 27 27
Lines 22721 22730 +9
Branches 6435 6436 +1
==========================================
+ Hits 8477 8602 +125
+ Misses 12380 12249 -131
- Partials 1864 1879 +15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thank you @adrabent ! I'll go make a new cwl-utils release now |
This pull requests should allow cwltool or toil to make use of an already existing node_slim.sif image in CWL_SINGULARITY_CAHCE or in the current working directory.