Various wrapper scripts improvement

[muffato]

Hi @vsoch

First of all, let me explain why I'm making this pull-request. I've got a software that necessitates a different wrapper script than the default singularity.sh, and I expect that other software will require the same change. Therefore, instead of defining those at the container level (hardcoding the tool / command-line parameters), I would like to save the alternative singularity.sh wrapper scripts in a central location. shpc currently only allows a single template per container technology, so there isn't really room for that.

The purpose of this PR is therefore to allow defining alternative template scripts that can be selected for different tools. I hijack the singularity_scripts directive for this. If an entry in singularity_scripts: corresponds to an alias listed in aliases:, then the script is used instead of the default singularity.sh. The template scripts are searched 1) in the container.yaml directory, 2) the user-defined global template directory, 3) shpc's own global template directory.

For instance, in container.yaml I have

aliases:
  longranger: /usr/local/bin/longranger
singularity_scripts:
  longranger: singularity_with_lsf.sh

and singularity_with_lsf.sh lives in the global template directory I've defined in shpc/settings.yml under wrapper_scripts.templates. Importantly it requires variables such as alias.command, etc, to be defined, unlike the usual singularity_scripts.

I've realised after having written the code that the aliases section has a "long" form where aliases can be defined like this:

aliases:
- name: python
  command: /usr/local/bin/python
  docker_options: -it

and I think I would now rather add an option there such singularity_template: singularity_with_lsf.sh instead of hijacking singularity_scripts.

At the same time, I've done a few modifications to the way the search path for templates is defined, so that files are not searched in the current directory.
I also removed the constraint that podman.sh scripts can't be used (they were turned to docker.sh by the code). If the user wants to name their template script podman.sh, why not allow it ?

The PR also includes a couple of minor changes in container/{docker,podman}.py to improve how the classes are connected.

Best,
Matthieu

[vsoch]

@muffato bravo! This is seriously beautiful code, it's a huge improvement over the current design! Let me respond with a few thoughts and suggestions.

The purpose of this PR is therefore to allow defining alternative template scripts that can be selected for different tools. I hijack the singularity_scripts directive for this. If an entry in singularity_scripts: corresponds to an alias listed in aliases:, then the script is used instead of the default singularity.sh.

Could there be a case where there is a common alias and the template script is defined and it is accidentally used? It sounds like you are relying on the container.yaml for the customization, and I'm wondering if we need to put our heads together and think about how this could better be controlled. E.g., is this something that would make sense to define for a view? Should there be an install flag that is a one off "use this wrapper script for this alias?" or some kind of customization file we allow to apply? I think the changes here are good/needed and let's keep thinking about this case of wanting not just customizations for a group of containers, but for a specific container.

The template scripts are searched 1) in the container.yaml directory, 2) the user-defined global template directory, 3) shpc's own global template directory.

Yes I like this design much better.

and singularity_with_lsf.sh lives in the global template directory I've defined in shpc/settings.yml under wrapper_scripts.templates. Importantly it requires variables such as alias.command, etc, to be defined, unlike the usual singularity_scripts.

Will there be an error if something is not? Or more generally, do we have a way to enforce what is needed and alert the user if something is missing?

I've realised after having written the code that the aliases section has a "long" form where aliases can be defined like this: and I think I would now rather add an option there such singularity_template: singularity_with_lsf.sh instead of hijacking singularity_scripts.

Yes indeed - this custom format is mapped into the same alias structure (but has more metadata). For your use case, is the wrapper something you think should be added to the registry (and thus added to this alias options section) or it's a one off for your specific cluster? If the latter (to go back to my comment above) I wonder how this could be an easy workflow. Right now you are saying:

1. Write a custom wrapper
2. Update the container.yaml with it (likely not something you can contribute to the registry)
3. Install

Or if it's the case that it's a general script that could be contributed, should we have the wrapper aliases be a different alias? E.g., for your example:

aliases:
  longranger: /usr/local/bin/longranger
singularity_scripts:
  longranger-lsf: singularity_with_lsf.sh

Or something of that nature? I could imagine something like:

aliases:
- name: longranger-lsf
  singularity_script: singularity_with_lsf.sh
  description: This is the longranger command as a wrapper intended for running with lsf

And that way we could package a bunch of wrapper scripts with a container (for different scenarios) that would be installed, but sort of optional to use. Or maybe we could allow the person to skip aliases, e.g.:

$ shpc install <module> --skip-alias longranger-lsf

If they have strong feelings about not including something :) But it's a fairly small addition so (personally speaking) I wouldn't have issue. And I haven't thought this out fully yet, but if there is some case where you want to apply a wrapper script more globally to all modules in a view, we could have some way to do that. Or perhaps in the case you don't want to edit the container.yaml but you do want a wrapper:

$ shpc install <module> --wrap longranger singularity_with_lsf.sh

The above would:

1. determine your container technology to derive if you want singularity_script, docker_script, or podman_script.
2. dynamically add the alias for longranger and to be associated with the wrapper
3. continue the install as if it had been already in the container.yaml to generate the wrapper!

The issue there would be reproducibility - e.g., if your install tree blows up I hope you saved all the commands :)

So overall I think let's focus on the following:

• it would be really good to be able to test this better, specifically what script is retrieved with different settings. There are examples of tests that "one off" settings, and perhaps if we have a get_wrapper_script function (I know you removed it but I think it would be useful for this testing scenario) those tests would be easy to write!
• Off of that point, I do think we should have proper tests here, at least more than I added because I totally missed some of the details you are fixing here!
• Perhaps we can extend the alias definition (the longer form) above to have a "singularity_script" or "docker_script" and then a description for it?
• And if you agree with the above, the "would be nice" would be the command line updates - to allow disabling a one off wrapper script install, OR enable one, one off.
• And for all of the above, documentation updates to make things more clear! I think your struggle with figuring out what I originally wrote is indication that it could be improved!

[muffato]

TODO list:

• it would be really good to be able to test this better, specifically what script is retrieved with different settings. There are examples of tests that "one off" settings, and perhaps if we have a get_wrapper_script function (I know you removed it but I think it would be useful for this testing scenario) those tests would be easy to write!
• Off of that point, I do think we should have proper tests here, at least more than I added because I totally missed some of the details you are fixing here!
• Perhaps we can extend the alias definition (the longer form) above to have a "singularity_script" or "docker_script" and then a description for it?
• And if you agree with the above, the "would be nice" would be the command line updates - to allow disabling a one off wrapper script install, OR enable one, one off.
• And for all of the above, documentation updates to make things more clear! I think your struggle with figuring out what I originally wrote is indication that it could be improved!

[vsoch]

I will be able to review again tomorrow!

[muffato]

Hi @vsoch . I need a couple of pointers to add the unit tests:

• Does the pytest stuff run on GitHub CI ? I don't see where / how
• In order to test this, I'd need test wrapper templates, test container.yaml, and etc. How would I go about adding those, but also changing the settings, like paths etc, for the tests ?

Matthieu

[vsoch]

Looks like pytest was running on circle, and stopped at some point (I don’t see it running recently). If you want to take a shot at adding to a GitHub recipe go for it (maybe main.yaml) otherwise I can get this added back asap.

singularity-hpc/.circleci/config.yml

Line 98 in eaf978c

pytest -sv test_*.py

And then for test data just add to an appropriate test data folder. And see this example

singularity-hpc/shpc/tests/helpers.py

Line 36 in eaf978c

client.settings.set("module_base", modules)

for creating a test client and changing a config value on the fly.

I’ll take a look into circle soon!

[vsoch]

Ok found the issue! And I’m not a nutter because circle has been running, just not for forks. I just clicked this so hopefully new pushes will run! Thanks for catching this!

[vsoch]

woohoo the circle tests ran!

[vsoch]

@muffato I have some nits above, but it's up to you if you want to change them - I'm happy with the current PR and ready to merge! Let me know your preference.

[muffato]

Then we're good to go !

[vsoch]

😎🥳🎉