Skip to content

Conversation

marcodelapierre
Copy link
Contributor

@marcodelapierre marcodelapierre commented Jan 20, 2022

With this PR, I am adding a beta version for the wrapper scripts functionality that I was mentioning in issue #383 . The main scope is to enable interaction of the command aliases with mpirun/srun, which don't work with shell aliases.

In this first iteration, it is an on/off functionality controlled in the settings.yml; I have added a 2nd setting to control the shell type in the wrapper scripts (I just added it in compliance with other shell usages in SHPC):

# for container aliases, use wrapper shellscripts instead of shell aliases, defaults to false
wrapper_scripts: false

# shell for wrapper shellscripts
wrapper_shell: /bin/bash

Setting wrapper_scripts to false (default), gives the traditional behaviour of shpc.
When it is set to true, 2 things change:

  1. under the module directory for that container, a bin/ subdirectory is created, containing one wrapper script per command alias
  2. modulefile: instead of the command aliases, PATH is prepended with "module_dir/bin"
    Note that aliases are still created for the generic commands -exec, -shell and so on.

Support:

  • shells: bash, sh, csh
  • container techs: singularity, docker/podman
  • module systems: lmod, tcl

Implementation notes:

  • there are two new templates, singularity.sh and docker.sh
  • edited the util function write_file, to add an option for making the file executable
  • most changes are in the container.install() method in singularity.py and docker.py
  • in this first iteration, the code block that generates the wrapper scripts is inside the install() method; it might be turned into a stand-alone method for better modularity and readability
  • I have changed the api to install() and add() (singularity only), to pass wrapper_template between module and container methods. Maybe this could be made cleaner?

Other notes:

  • are unit tests needed?
  • documentation still to be updated

What do you think?

Copy link
Member

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

Ah okay I think I get this conceptually! I'm going to need an evening or two to test and review, and order wise I want to get #472 in first just for review order of operations. So stay tuned! One thing to think about in the meantime is, aside from having the aliases be the driver for the wrapper scripts (e.g., it looks like each alias becomes an executable to basically run a container) what other functionality could be provided? E.g., what would be a different kind of wrapper script, or even one that is custom provided by a user? I will think on this too - so expect to hear from me probably in a few days, latest the weekend!

Copy link
Member

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

Okay some early comments are below! Sorry for the delay, busy week and had some other shpc things to work on. Given that I tested outside of a container I think this would have worked, but it's led me to realize two things:

  • we need to figure out how to not hard code paths into the modules (and make them un-usable given a bind or change of location).
  • we should likely load the default config first and then update with the user to be able to better handle issues like forgetting to set a value.

And of course when this is more ready we need some good docs to describe wrapper scripts and show how they work and what happens when you set to true!

@vsoch
Copy link
Member

vsoch commented Jan 22, 2022

okay and here is a quick proposal to handle the bug I ran into here (not adding the wrapper_shell) #475

@marcodelapierre
Copy link
Contributor Author

thanks @vsoch ! should be able to work on SHPC later today

@marcodelapierre
Copy link
Contributor Author

OK, I will deep dive on this one tomorrow, for now just replying to this very first comment of yours:

what other functionality could be provided? E.g., what would be a different kind of wrapper script, or even one that is custom provided by a user?

Great point!
Indeed, the script opens up to various possibilities, the first few that come to my mind being:

  1. additional features provided directly by SHPC might include adding pre- (and eventually post- ..?) execution lines in the wrapper script. One use I can think of is for defining environment variables; this is done already in other ways in SHPC, and know I can't think of why one should use this method instead, but I am just thinking based on in-house scripts I did use
  2. interface for users to edit the template (powerful and risky at same time of course)..and eventually even the possibility of having a user define template in the dot directory in the home, same as the settings. This would actually render 1. not needed...and I also now realise it's exactly what you suggested...! my brain is done for the day I guess eheh :)

For point 2. (ie your point) then, it would be about providing similar features to shpc config edit/shpc config inituser/shpc config edit -c.

More coming tomorrow..

@marcodelapierre
Copy link
Contributor Author

OK, I think I am done for today.

I have provided and tested fixes for some of your feedback.

Just summarising from the above what seem to be the current open points, which I can go back to next week:

  1. dynamic path in the modulefiles for wrapper scripts: feel free to give feedback to my idea, and I will then proceed with an implementation
  2. flexible templatefile in the codebase
  3. function to add wrapper scripts
  4. further extension of the feature (for this PR, or a later one): we were talking about allowing users to provide a custom templatefile. Do you think that providing functionalities similar to the settings YAML is enough? (namely CLIs to edit the template, create a user-specific one, and edit the latter)
  5. documentation for the wrapper script functionality

@vsoch
Copy link
Member

vsoch commented Jan 28, 2022

  1. dynamic path in the modulefiles for wrapper scripts: feel free to give feedback to my idea, and I will then proceed with an implementation

I love this idea and think it's so important - in practice I'm not sure how to implement it, because the module files are sort of "written once and then done." I was trying to look for a way to get this relative path via the module software but maybe there is a better way?

  1. flexible templatefile in the codebase

This is an interesting idea that I'd like to think through more.

  1. function to add wrapper scripts

Can you expand on this?

further extension of the feature (for this PR, or a later one): we were talking about allowing users to provide a custom templatefile. Do you think that providing functionalities similar to the settings YAML is enough? (namely CLIs to edit the template, create a user-specific one, and edit the latter)

I think for now having a variable for a custom file would be perfect. E.g., use the default (the container ones) or write a custom one and use it. I think if we have more than one what we ultimately will want is an organized folder with named files, and then perhaps a default, but a way for an install to request using one or more (probably via the container.yaml) and then it could be provided with the container for a one off script relevant to it, or somewhere else in shpc for more generic templates. Given install of a module and request for a name, we'd first search the container.yaml directory, and then go to the shpc install.

documentation for the wrapper script functionality

100% !

@marcodelapierre
Copy link
Contributor Author

Sorry @vsoch , been busy, will be back on it soon :)

@vsoch
Copy link
Member

vsoch commented Feb 15, 2022

okay we should be able to rebase with main and then pick up discussion here!

@marcodelapierre
Copy link
Contributor Author

Yep, in my to do list for today! ;)

@marcodelapierre
Copy link
Contributor Author

Ok, so the big win of the day is the dynamic path in the modulefiles for wrapper scripts!
Hopefully tomorrow I will give a pass to refactoring some repeating code into a function (as per a comment of yours above).

@marcodelapierre
Copy link
Contributor Author

Ok, so, after the modularisation of the code that generates the wrapper scripts, I can still see two outstanding points from our thread above.

The 1st one is about allowing for custom template files for the wrapper scripts.
I am proposing to leave this feature out of this PR and work on this later (time permitting). To be entirely honest, it's really because my time is crunching right now, and I would love to see the basic wrapper scripts go public :).
I would also add that this is inline with the fact that at present there is no customisation allowed for the Lua and Tcl templates neither.

In terms of what we could do in the future for these custom templates, as we said previously, there are few possibilities:

  1. one custom template (could sit in the dotdir, similar to the user settings.yml?): if the sys admins need to customise the template, they can just do it in the one that ships with SHPC, so it might make sense to offer the custom one for users (although I can't think of an immediate use for this right now)

  2. custom templates that are container specific: these would go in the registry along side the container.yaml. No immediate use right now neither, but maybe there could be specific use cases.

If you're happy with keeping this in the backlog, I can turn this comment in a separated github issue.

@marcodelapierre
Copy link
Contributor Author

The 2nd outstanding point is documenting the new wrapper scripts feature, which I will try to do asap! :-)

@marcodelapierre
Copy link
Contributor Author

On a separate note, a couple of days ago I also did a commit to this PR, to ensure wildcard arguments are "quoted" appropriately in the wrapper templates, a bit like a did in the modulefile templates the other day.

Look at the weird syntax that csh needs inside a script:

{% if '/sh' in wrapper_shell or '/bash' in wrapper_shell %}"$@"{% elif '/csh' in wrapper_shell %}$argv:q{% endif %}

The wildcard is $argv:q, where one must use $argv instead of $*, and where :q takes care of possible quotes inside the argument .... this was wild! It took time me quite some time to dig this information in the internet, mostly because it was not clear to me which were the keywords to search. A couple of references, so that this doesn't get lost again:

https://stackoverflow.com/questions/42263202/bash-csh-echo-the-strings-argv-and-bin-csh
https://www.staff.tugraz.at/reinfried.o.peter/unix/cshell.html

@vsoch
Copy link
Member

vsoch commented Feb 16, 2022

The 1st one is about allowing for custom template files for the wrapper scripts. I am proposing to leave this feature out of this PR and work on this later (time permitting). To be entirely honest, it's really because my time is crunching right now, and I would love to see the basic wrapper scripts go public :).

Let me take a shot at this, and I'll work on your branch here (but won't push in case you don't like the changes of the work!) I think it's important to have this in, for the first shot.

I would also add that this is inline with the fact that at present there is no customisation allowed for the Lua and Tcl templates neither.

This is actually an interesting idea I was pondering - if the lua / tcl templates could be considered under the same namespace as wrapper, and then just with a different output directory.

And I like all your ideas for custom templates! I'm hoping I can figure out something that the user can provide one, one off, and the container-specific ones make sense (if there is a use case).

@marcodelapierre
Copy link
Contributor Author

marcodelapierre commented Feb 16, 2022

Thanks @vsoch !

I agree with what you say, especially around the template/wrapper customisation benefitting Lua/Tcl templates, and hence the need for a wrapper class...or should it be called a template class, to effectively include lua/tcl modulefile templates?

Please go ahead and feel free to work on this branch/PR!

On my side, I expect to shoot one more commit today with some basic documentation on the feature, and then be quite for a bit ;)

@vsoch
Copy link
Member

vsoch commented Feb 16, 2022

Hold off on the docs - I can do that fairly quickly, and with the refactor I'm thinking it would be not the best use of your time. I can't promise to get the PR in tonight because it's late, but I have a good start so it will be sometime this week!

@marcodelapierre
Copy link
Contributor Author

Ok sure, will wait on the docs!
And no rush on the refactoring :-)

@vsoch
Copy link
Member

vsoch commented Mar 3, 2022

Added with #496 !

@vsoch vsoch closed this Mar 3, 2022
@marcodelapierre marcodelapierre deleted the feature/wrapper_scripts branch June 7, 2022 13:19
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