singularityhub / singularity-hpc

Local filesystem registry for containers (intended for HPC) using Lmod or Environment Modules. Works for users and admins.
https://singularity-hpc.readthedocs.io
Mozilla Public License 2.0
111 stars 26 forks source link

Change location of wrapper scripts #653

Closed theklaloizou closed 1 year ago

theklaloizou commented 1 year ago

Hello,

I was wondering if there is an option to change the location of wrapper script into another location. If we have many binaries in container.yaml file then many wrapper scripts are being created and Lmod issues a warning of having too many non-module files in modules folder.

Thanks, Thekla

vsoch commented 1 year ago

We don't have support for that - but it's a great idea! I'll see if I can make some time soon to do a PR for you to test!

theklaloizou commented 1 year ago

Thank you for your prompt reply!

theklaloizou commented 1 year ago

@vsoch I just wanted to let you know that I also had a look at the PR you created for this issue. I tested it on a Rocky Linux system with Lmod and Singularity installed. I was also getting the extra bracket in the wrapper script but then I noticed that there was indeed an extra bracket in the code: https://github.com/singularityhub/singularity-hpc/blob/b7ff0e406a71b681388f9d282b4721669d83eab6/shpc/main/wrappers/templates/bases/shell-script-base.sh which I removed from the specific file and then it was ok. I also noticed that the wrapper_base path provided in the settings.yml file is relative to the directory shpc is installed. Even if I add the full path for wrapper_base as we do for module_base this full path is created inside the shpc directory (or maybe I am doing something wrong :) ) Finally, I get the same issue for the 99-shpc.sh file that is located inside the module directory instead of the wrapper scripts directory.

marcodelapierre commented 1 year ago

Wow many thanks @theklaloizou , much appreciated.

Time for me to get a pair of glasses for work .. I had hunted for the cheeky bracket in the code, but could not spot it.

vsoch commented 1 year ago

@theklaloizou amazing you found the extra character! I absolutely could not, my eyes definitely weren't good enough.

I'm off to bed (almost 2am) but can you provide an example to reproduce what you would see - e.g., the setting, what you get, and what you would expect? I'm quite busy in the next month or so but this is something we can continue working on, and likely little bits at a time should work.

I just pushed the fix - looking forward to finishing this up, hopefully sooner than later!

theklaloizou commented 1 year ago

Thank you @vsoch! I tried to reproduce my issue using the latest commit in the branch and I cannot :) so maybe I was doing something wrong. wrapper_base path works ok now and when loading a module and having a separate wrapper directory binaries are correctly captured from there. The only issue I see at the moment is the location of 99-shpc.sh file. The wrapper scripts are looking for the file in the wrappers directory but it is actually located in the modules directory.

vsoch commented 1 year ago

The wrapper scripts are looking for the file in the wrappers directory but it is actually located in the modules directory.

Ah interesting - I'm looking at a newly generated output and I see for the wrappers they reference the script in the moduleDir

bin/python-run:singularity ${SINGULARITY_OPTS} run ${SINGULARITY_COMMAND_OPTS} -B $moduleDir/99-shpc.sh:/.singularity.d/env/99-shpc.sh   /home/vanessa/Desktop/Code/shpc/containers/python/3.12-rc/python-3.12-rc-sha256:8c0e8838e1423fda05db197764e269bea6cb33748525e5382dacfd824829897b.sif "$@"

but... the moduleDir is actually wrapper_dir!

#!/bin/bash

script=`realpath $0`
wrapper_bin=`dirname $script`
moduleDir=`dirname $wrapper_bin`

singularity ${SINGULARITY_OPTS} run ${SINGULARITY_COMMAND_OPTS} -B $moduleDir/99-shpc.sh:/.singularity.d/env/99-shpc.sh   /home/vanessa/Desktop/Code/shpc/containers/python/3.12-rc/python-3.12-rc-sha256:8c0e8838e1423fda05db197764e269bea6cb33748525e5382dacfd824829897b.sif "$@"

I think we'd probably want to be consistent in our choice, whatever that may be. Given no custom wrapper directory, then the wrapper directory IS the module directory so they are one and the same! But given a custom wrapper directory, we could either move it to be alongside wrappers (and reduce the module directory size) or be consistent and keep it in the module directory, because likely if someone wants to debug the module environment it might be alongside the module.

@marcodelapierre do you have any opinions about where this should go?

marcodelapierre commented 1 year ago

We are talking of the 99-shpc.sh file, right? I think it is a bash component of the wrappers/scripts machinery, and distinct/stranger from the lua/tcl modulefiles (of Lmod/Envmodules concern), so I would consider having it with the wrappers.

vsoch commented 1 year ago

@theklaloizou I think I've gotten the kinks out if you want to give it another test! https://github.com/singularityhub/singularity-hpc/pull/654

It was more than I anticipated because that environment file was hard coded with moduleDir in several places. Probably for some future refactor we should have it be it's own variable, but I think what we have is OK for now (warranted it works)!

theklaloizou commented 1 year ago

@vsoch I confirm that now things work as expected :) Thank you for your efforts!

vsoch commented 1 year ago

Awesome! I say we merge this then. @marcodelapierre Ill wait the AOK (or request for more tests, changes, etc.) from your review.

marcodelapierre commented 1 year ago

Ubuntu 22.04
Python 3.10.12
Lmod 8.7.31

With wrapper_base unset in the settings,

There is an issue with the PATH variable:

> echo $PATH
$moduleDir/bin:[...]

It has $moduleDir instead of its value.

marcodelapierre commented 1 year ago

Summary of my tests:

theklaloizou commented 1 year ago

I haven't tested the case where wrapper_base is not set. I have tested it as well now and I confirm that it is not working. PATH is not set correctly, it includes: $moduleDir/bin.

vsoch commented 1 year ago

it includes: $moduleDir/bin.

Why is this incorrect? When a wrapper base is unset we default to the module directory when wrappers are enabled.

theklaloizou commented 1 year ago

The issue is that instead of getting the value of $moduleDir/bin in PATH we get "$moduleDir/bin". So, something is not working when evaluating the variable.

vsoch commented 1 year ago

Is this for docker or singularity (or both) and which format, lua or tcl?

vsoch commented 1 year ago

okay I tried removing quotes around a variable for singularity and lua, but I'm not sure that's the right one.

vsoch commented 1 year ago

Update - I think I found those bits! https://github.com/singularityhub/singularity-hpc/pull/654/commits/43c4abb6565c5b3b1ef6888e5e41255d64645155 Give is another try next time you are at keyboard, and it's Friiidaaay so no worries if not until next weekend!

I am w̶a̶t̶c̶h̶i̶n̶g̶ ̶m̶u̶s̶i̶c̶ ̶v̶i̶d̶e̶o̶s̶ working hard today :)

marcodelapierre commented 1 year ago

All right, some SHPC testing to wrap up the weekend 😄

Engine: both Singularity and Docker

... almost there!

vsoch commented 1 year ago

Should that just be in quotes? Or missing brackets? Brackets and quotes?

vsoch commented 1 year ago

@marcodelapierre I maybe fixed it? I'm like the three blind mice here with lua :laughing: https://github.com/singularityhub/singularity-hpc/pull/654/commits/e0fbcd291a401a82bd942f5e1d730a8c0c584350

vsoch commented 1 year ago

Correction - I am one blind mouse, with many bad jokes.

marcodelapierre commented 1 year ago

😂 I will have to look up the three blind mice gag, don't know it ... mentoring at an hackathon right now, just had the coffee break, and I was actually going to go through your last commits, I think I might have some minor comments

marcodelapierre commented 1 year ago

I have two kids and sang heaps of nursery rhymes over the past 7 years ... how come that I did overlook the three blind mice?!?! 😂 must be some form of Australian filtering from the outside world, we our quite proud of our locally bred nursery rhyme flora!! 😄

vsoch commented 1 year ago

Yes! It might be because it's one of those old ones that is a bit dark when you look at the lyrics (being chased around with a carving knife, akin to "Ring Around the Rosie.") But the three blind mice I like because they sometimes pop up in references. Here is my favorite from Shrek:

marcodelapierre commented 1 year ago

Should that just be in quotes? Or missing brackets? Brackets and quotes?

sorry I was carried off the laptop by the amazing Ferrari F1 victory in Singapore!

So:

If you agree with this suggested minor change, please go ahead with committing it, then I will quickly test it, and then I think the PR will be ready for merge from my standpoint!

marcodelapierre commented 1 year ago

Yes! It might be because it's one of those old ones that is a bit dark when you look at the lyrics (being chased around with a carving knife, akin to "Ring Around the Rosie.") But the three blind mice I like because they sometimes pop up in references. Here is my favorite from Shrek:

Something to trial with my 7yo tonight...thanks! 🤩

vsoch commented 1 year ago

okay done! And sorry for long delay, researching K8s storage. I have superhuman mode on until early November (Kubecon!) and unreasonably high expectations for myself.

marcodelapierre commented 1 year ago

Pleeeease look after yourself!! ❤️
You are amazing at normal load, be reasonable with your own expectations .... and you will be surprised by yourself!! :-)

marcodelapierre commented 1 year ago

So ... one last error , now the offender is shpc/main/wrappers/templates/bases/shell-script-base.sh

When wrappers are in the module tree, 99-shpc.sh is searched for in the wrong location (under bin/, should be one directory up).

So I suggest the following, for shell-script-base.sh:

#!{{ settings.wrapper_shell }}

{% if '/csh' in settings.wrapper_shell %}set {% endif %}script=`realpath $0`
{% if '/csh' in settings.wrapper_shell %}set {% endif %}wrapperDir={% if settings.wrapper_base %}"{{ wrapper_dir }}"{% else %}`dirname $script`/..{% endif %}

{% block content %}{% endblock %}

[EDIT] I think it can actually be made even simpler:

#!{{ settings.wrapper_shell }}

{% if '/csh' in settings.wrapper_shell %}set {% endif %}script=`realpath $0`
{% if '/csh' in settings.wrapper_shell %}set {% endif %}wrapperDir=`dirname $script`/..

{% block content %}{% endblock %}

...and you don't need to pass wrapper_dir any more. By definition, 99-sphc.sh sits one directory above any of the wrappers!

I will re-test after your commit.

Sorry - end of local hackathon day 1 - brain out of juice ...!!

marcodelapierre commented 1 year ago

One more message, for extra info. Today:

So I am positive that the above will indeed be the last edit :-)

vsoch commented 1 year ago

I can try the second of the above! But what happened to moduleDir ? We don't need it?

marcodelapierre commented 1 year ago

ModuleDir is not used any more, its function is replaced by WrapperDir

vsoch commented 1 year ago

okay commit! Let's see how it goes!

marcodelapierre commented 1 year ago

Green lights!! 🚀

vsoch commented 1 year ago

wooo!! https://youtu.be/o-AbEO6J8s0?si=dX0Dt-CaLA-ehDqP