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.

Sign up for GitHub

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

Jump to bottom

Singularity compatibility update #2079

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Singularity compatibility update #2079

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion idmtools_platform_slurm/idmtools_platform_slurm/assets/_run.sh.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ do
{% if simulation.task.command.cmd.startswith('singularity') %}
{{simulation.task.command.cmd}}
{% else %}
singularity exec {{simulation.task.sif_path}} {{simulation.task.command.cmd}}
singularity exec -B $(pwd) -B $(pwd)/../Assets --pwd $(pwd) {{simulation.task.sif_path}} {{simulation.task.command.cmd}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckirkman-IDM , here are some comments for your reference:

  1. -B $(pwd) is not necessary as it was done by default in Singularity

  2. --pwd $(pwd) is not necessary as script ran under $pwd already and Singularity takes current working directory already by default

  3. for -B $(pwd)/../Assets

SlurmPlatform support all kind of tasks (not just EMODTask), in certain case there may not have Assets folder (remark: we may have with current code due to code defect). You may consider the following instead

-B $(pwd)/../ <your sif path>
  1. Workaround

This PR only solves NYU's one special case, like Sharon pointed out the NU case, it doesn't solve the general binding cases. Instead of fixing a special case with PR, you may use the same work around like Sharon's case:

task.sif_path = '-B $(pwd)/../Assets <your sif path>'

or simply

task.sif_path = '-B $(pwd)/../ <your sif path>'
  1. My proposal of fixing general biding issue as below:

Default: idmtool has experiment folder binded in _run.sh file

singularity exec -B {{exp_folder}} {{simulation.task.sif_path}}

Then we separate binding from sif_path in workaround above.

Approach 1:

Since we alrady did

task.sif_path = '<your sif path>'

we may add one more variable for binding, like

task.sig_bind = '<Singularity bindings>'

Notes
singularity exec may have many other optional parameter in addition to --bind. If needed, other options can be built/constructed into

task.sig_bind = '<user-path-binding>

For example,

(a) with default experiment folder binded, it solves Clark's case automatically

(b) for Sharon't case, we can do

task.sif_path = '/projects/b1139/dtk_run_rocky_py39.sif'
task.sig_bind = '--bind /projects'

Approach 2:

Instead of doing (adding variables to task)

task.sif_path = '<user-sif-path>'

we can instroduce two input parameters in suite.run(...) and experiment.run(...),

  • SIF_PATH
  • SIG_BIND

Let's take suite as an example, same for experiment:

suite.run(..., SIF_PATH='<user-sif-path>')

if user needs more extra path binding, we can do

suite.run(..., SIF_PATH='<user-sif-path>', SIG_BIND='<user-path-binding>')

Notes
singularity exec may have many other optional parameter in addition to --bind. If needed, other options can be built/constructed into

SIG_BIND='<user-path-binding>

For example,

(a) with default experiment folder binded, it solves Clark's case automatically

(b) for Sharon't case, we can do

suite.run(..., SIF_PATH='/projects/b1139/dtk_run_rocky_py39.sif', SIG_BIND='--bind /projects')

Remark: with appraoch #2, we need to consider code back compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply, working on digesting it all :)

{% endif %}
{% else %}
{{simulation.task.command.cmd}}
Expand Down