Add/wrapper scripts

[vsoch]

this will support two kinds of wrapper scripts - global that live in the settings.yaml, and container.yaml specific that go alongside that file. We basically will then generate aliases for any executable names that were not created as wrappers. we will want to follow this up with a command to list wrappers, and also include descriptions for the interested user, and a flag to install to select on the fly

cc @marcodelapierre ! I hope this is getting closer to what we want - being able to support custom wrappers for aliases (the global ones) and alongside containers. It is HUGELY more complicated than your original implementation (and this might be bad) but on the other hand, adding a new wrapper is fairly trivial.Take a look and let me know what you think - it's dangerous for me to stay up so late, and who knows what monster I coded! On the other hand, it was totally worth it because I had a ton of fun!

I also tried to clean up some of the initial module parsing, e.g., instead of passing self.settings variables a million times I'm just using self.settings, and i tried to do that for wrappers too.

Signed-off-by: vsoch vsoch@users.noreply.github.com

[marcodelapierre]

docker OK

found one little issue (singularity, but I guess generic):

this causes an error (wrappers enabled, but generic singularity set to false - to only use custom wrappers in the recipe dir):
also same error if value is left empty (which works eg for features):

wrapper_scripts:
  enabled: true

  # use for docker aliases (set to null to disable)
  docker: docker.sh

  # use for podman aliases (set to null to disable)
  podman: docker.sh

  # use for singularity aliases (set to null to disable)
  singularity: null

$ shpc install quay.io/biocontainers/blast:2.12.0--pl5262h3289130_0
Traceback (most recent call last):
  File "/group/pawsey0001/mdelapierre/PSS/shpc-tests/wrap-18jan22/wrap-vsoch/singularity-hpc/bin/shpc", line 11, in <module>
    load_entry_point('singularity-hpc', 'console_scripts', 'shpc')()
  File "/group/pawsey0001/mdelapierre/PSS/shpc-tests/wrap-18jan22/wrap-vsoch/singularity-hpc/shpc/client/__init__.py", line 372, in run_shpc
    main(args=args, parser=parser, extra=extra, subparser=helper)
  File "/group/pawsey0001/mdelapierre/PSS/shpc-tests/wrap-18jan22/wrap-vsoch/singularity-hpc/shpc/client/install.py", line 17, in main
    container_tech=args.container_tech,
  File "/group/pawsey0001/mdelapierre/PSS/shpc-tests/wrap-18jan22/wrap-vsoch/singularity-hpc/shpc/main/__init__.py", line 28, in get_client
    settings = Settings(kwargs.get("settings_file"), validate)
  File "/group/pawsey0001/mdelapierre/PSS/shpc-tests/wrap-18jan22/wrap-vsoch/singularity-hpc/shpc/main/settings.py", line 280, in __init__
    self.validate()
  File "/group/pawsey0001/mdelapierre/PSS/shpc-tests/wrap-18jan22/wrap-vsoch/singularity-hpc/shpc/main/settings.py", line 50, in validate
    jsonschema.validate(instance=self._settings, schema=shpc.main.schemas.settings)
  File "/group/pawsey0001/mdelapierre/.local/lib/python3.6/site-packages/jsonschema/validators.py", line 934, in validate
    raise error
jsonschema.exceptions.ValidationError: None is not of type 'string'

Failed validating 'type' in schema['properties']['wrapper_scripts']['properties']['singularity']:
    {'type': 'string'}

On instance['wrapper_scripts']['singularity']:
    None

maybe I misunderstood how to setup the case where I want custom wrappers + generic aliases?

[marcodelapierre]

And a very last comment, this is a proposed change for discussion.

Currently, in modulefile templates we're using the dynamic module_dir inspection only to define the PATH for wrapper scripts.
However, we're still using a {{ module_dir }} in other instances:

• modulefile templates: to provide the location of the environment file
• modulefile templates: to define a service variable (MODULEPATH for Lmod, progdir for Tcl)
• wrapper templates: to provide the location of the environment file

I think we should be able resort to a dynamic definition in all these cases, which would make the module tree completely portable, the only (unavoidable probably) hard-coded path remaining the SIF path for Singularity.
The only non trivial one could be in the wrapper templates, if people use a strange subdir instead of bin/ (as in, different directory tree level), although there's probably a solution for this one (?).

Thoughts?

[vsoch]

@marcodelapierre I think I've fixed the null parsing (and related validation) issues! For the module_dir I agree, and if you want to make suggestions on the PR here or just write in comments how to change, I can work on tomorrow! You can also PR to the branch here and then I'll just merge it in. I'm off to bed now but thank you for the review - back tomorrow!

[marcodelapierre]

Ok, can confirm all of the reported issues above are resolved when using the latest commit.

Two minor polishing edits:

• settings.yml, wrapper_subdir value, change "bin" into bin (no quotes) for consistency with other string values
• modules/templates/docs.md : typos regarding shell vs settings.singularity_shell

I have actioned these two edits in a branch of mine, doing the PR onto your branch now ..

[marcodelapierre]

For module_dir, I am unable to edit the code myself now (possibly in the next few days..?), but I was thinking:

• module templates: use the dynamic module_dir syntax to define a service variable, which could be called moduleDir for instance, and then use this variable to build the following:

  1. PATH for wrapper scripts
  2. path of environment file to bind mount
  • by the way, we could unify MODULEPATH (Lua) and progdir (Tcl) to something like moduleDir as said above, or if there is any reason for these names, we will then keep using them

• wrapper templates:

  • first of all, it would be good to add a couple of checks when parsing the value of settings.wrapper_subdir in SHPC, including removing leading/trailing slashes, and possibly only allowing one level of subdirectory (i.e bin is fine, but sub/bin to be made illegal). We should also disallow climbing back in the tree with .., which would mess up things across container versions and even repositories, and we might want to also disallow the current directory . , just to keep things tidy and easy. If we do all of this, then it's super simple to have the dynamic path. For instance in bash:

    wrapperDir="$(dirname $(readlink -f $0) )"
    moduleDir="${wrapperDir}/.."
    

Thoughts?

[vsoch]

first of all, it would be good to add a couple of checks when parsing the value of settings.wrapper_subdir in SHPC, including removing leading/trailing slashes, and possibly only allowing one level of subdirectory (i.e bin is fine, but sub/bin to be made illegal). We should also disallow climbing back in the tree with .., which would mess up things across container versions and even repositories, and we might want to also disallow the current directory . , just to keep things tidy and easy.

Totally agree - and another basic question - is there any need to really allow them to customize bin? Like what else would they change it to? Then we don't have any of those problems about parsing paths / checking for going up in the tree!

[marcodelapierre]

Good point! I think there's no need to expose the choice of the bin string to admins/users, it could just be defined as an attribute of a WrapperScript object.

I can see only two occurrences of wrapper_subdir outside the templates.
They are inside wrappers/base.py, and with an object attribute they could just become: self.settings.wrapper_subdir --> self.wrapper_subdir

[vsoch]

okay done! I didn't even keep the variable I just put bin, we can go back to a variable when we know we need it!

[marcodelapierre]

ok, so two outstanding bits:

1. merge Add/wrapper scripts - minor polishing #497 in here, which implements some minor polishings
2. module_dir changes, see above

on point 2., for the wrapper scripts better to keep it as simple as:

• bash

  moduleDir="$(dirname $0)/.."
  

• csh

  set moduleDir="`dirname $0`/.."
  

• template for both shells (inverted quotes work in bash, too):

  {% if '/csh' in settings.wrapper_shell %}set {% endif %}moduleDir="`dirname $0`/.."
  

[vsoch]

@marcodelapierre before I push any changes - what about:

script=`realpath $0`
wrapper_bin=`dirname $script`
{% if '/csh' in settings.wrapper_shell %}set moduleDir=`dirname $wrapper_bin`{% else %}export moduleDir=$(dirname $wrapper_bin){% endif %}

And then we can have that as a snippet added to the wrappers at the top?

[marcodelapierre]

Hi @vsoch , sure , reads good to me.
feel free to go ahead with all these module_dir changes if you have the time, including in the lua/tcl templates.
I was hoping to go ahead myself soon, but am unable to.

[vsoch]

@marcodelapierre okay I think I had a cool idea! So I was thinking we will likely want to add snippets/bases for different wrapper scripts, so I refactored the directory to organize as such (see docs in 4c7ac07) and now the scripts can do things like extend or include other templates!

This doesn't apply for modules, but I'm thinking if we like the approach and there is huge redundancy in the tcl/lua templates we could do something similar (but for another PR).

Also I wasn't sure what you wanted me to do for tcl/lua templates with the bash/shell so I will need your guidance there.

[marcodelapierre]

hi @vsoch ,

so snippets sound like a great idea!

I have just tested the latest commit.

I can see only one issue, kind of related to a previously fixed one:

• the x11 feature does not produce the extra strings in the modules/wrappers, if the x11 true value in settings is between quotes [if the setting is not in quote, all good]

otherwise all good!

now I am going to see if I can apply those module_dir edits to the templates

[vsoch]

I’m confused with x11 - you mean someone added quotes around true in the config so it’s not a boolean?

[marcodelapierre]

No, it's only a problem if the quotes are there (in my case it was from an old settings.yml).

shpc config set now correctly writes true/false without quotes.
So, maybe mine is a non-issue..? :)

[vsoch]

I would say it’s a non issue if it’s impossible to do again, but if we can be more strict in the schema to not allow it in the first we could try that. I think this PR is getting huge so my thinking is this issue is slightly out of scope especially if we fixed the setting to prevent it! But if someone hits it again we can come back to it.

[marcodelapierre]

agree!

I am working on dynamic module_dir in the templates, hopefully not taking too long

[marcodelapierre]

OK done:

#499

[marcodelapierre]

If I am not missing something...I think this PR might be ready, wow! :-)
What do you think @vsoch ?
(no rush, I am just excited!)

[vsoch]

I agree! 🥳

Will merge and release after dinner.