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.

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

adding support for alias files #557

Merged
merged 17 commits into from
Jul 8, 2022
Merged

adding support for alias files #557

merged 17 commits into from
Jul 8, 2022

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Jun 28, 2022

An alias file is a yaml file "lookup" alongside a container.yaml that can be used to provide tag specific commands for a particular
container tag. I am also adding the two pawsey containers as an example, and (when the main CI passes) we can trigger the container test CI, as I've also updated those tests to assert that all provided alias files exist!

This will close #536

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

an alias file is a yaml file "lookup" alongside a container.yaml
that can be used to provide tag specific commands for a particular
container tag

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch vsoch added the container-recipe Apply this label to indicate adding a new container recipe in the "registry" folder to run its tests label Jun 28, 2022
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch vsoch added container-recipe Apply this label to indicate adding a new container recipe in the "registry" folder to run its tests and removed container-recipe Apply this label to indicate adding a new container recipe in the "registry" folder to run its tests labels Jun 28, 2022
@vsoch
Copy link
Member Author

vsoch commented Jun 28, 2022

Docs preview for alias files:

image

@marcodelapierre
Copy link
Contributor

I love how this looks like!
Will give it a go as soon as I can

@vsoch
Copy link
Member Author

vsoch commented Jun 28, 2022

Also @marcodelapierre when you have some time it would be fun to brainstorm an idea that I’ve been toying with - I really like this idea of providing container transparency (wrt) aliases - it was my initial vision for the scientific filesystem (building the aliases into the entry point) and the interactions here. What I’ve been chewing on is how to move these aliases outside of shpc, meaning making them possible for any container technology to find and use. It could mean something like:

  • a custom container build that allows for customization or automation detection and writing of aliases
  • Saving into some simple metadata file
  • Using Oras OCI registry as storage to upload as a related artifact
  • A custom tool that can find the container and alias file, and “enable” it (and I’m not sure what that means yet)

At the most complex level you could imagine a registry just for aliases, and a tool like shpc would ping it to look up containers. And the simplest level is a command line tool that tries to bring them together in some context.

@marcodelapierre
Copy link
Contributor

I will definitely follow-up on this, I would love to brainstorm on it!
Can you make a dedicated github issue?

@vsoch
Copy link
Member Author

vsoch commented Jun 28, 2022

Yes!

@muffato
Copy link
Contributor

muffato commented Jun 28, 2022

Hello !
This sounds like a great feature. I don't have the use case right now, but I'm sure it's going to come very soon. So very pleased to see this pull request.

One comment: could this be called say overrides rather than alias_files, and the content merged with container.yaml's. This would allow replacing not only the aliases, but also the environment variables, which can equally change over time !

Best,
Matthieu

@vsoch
Copy link
Member Author

vsoch commented Jun 28, 2022

@muffato that's a great idea! Would it be okay to still be tag specific, and are there any fields we don't want to override?

@muffato
Copy link
Contributor

muffato commented Jun 28, 2022

  1. I think that's fine for now. It could be useful at some point to support a filter language, so that multiple tags could be matched to the same override file on the same line. Maybe, to avoid the confusion, it could written down as a "long form" and what you have now would be the "short form" (like how aliases are defined). I'd be happy keeping this for later, when the use case actually materialises, though I've noticed you're more eager than me to be future-proof ;)
  2. Looking at the container.yaml files, I guess we should allow overriding docker_scripts: and singularity_scripts: too. And I think that should be it

@vsoch
Copy link
Member Author

vsoch commented Jun 28, 2022

Ok sounds good! I should be able to update the PR this evening.

@marcodelapierre
Copy link
Contributor

Cool!
Will test with my OpenFoam user case (tag-specific aliases only) after the updates to PR

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Jun 29, 2022

okay - updates to use "overrides" and for a specific set of fields should be in! And:

I think that's fine for now. It could be useful at some point to support a filter language, so that multiple tags could be matched to the same override file on the same line. Maybe, to avoid the confusion, it could written down as a "long form" and what you have now would be the "short form" (like how aliases are defined). I'd be happy keeping this for later, when the use case actually materialises, though I've noticed you're more eager than me to be future-proof ;)

I actually think starting with the "short form" and only adding the long form when it's needed makes sense.

Copy link
Contributor

@muffato muffato left a comment

Choose a reason for hiding this comment

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

It looks amazingly lean and clean ! Nice one :) Just a couple of comments:

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -259,26 +304,29 @@ def get_envars(self):
"""
return dict(self.env) if self.env else {}

def get_aliases(self):
def get_aliases(self, tag=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a very dumb question: why is this function changed ? I can't see the tag parameter being used, and I don't understand the renames aliasesloaded and self.aliasesaliases.
Is it a leftover from the previous version where only aliases could be overridden ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This tag=None should be removed - that was an oversight! When the changes were scoped to aliases, we had lines here that would essentially load the file (and aliases) on demand. Now as overrids, that happens once at the beginning of the install (so we don't need the tag here). It was my oversight to not remove that (now unneeded) tag parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh and I see how I renamed the variables - I think I liked the original design better, so I'll make a fix to this tonight to restore the original function. Thanks for catching this I totally missed it!

vsoch and others added 4 commits June 29, 2022 14:27
Co-authored-by: Matthieu Muffato <mm49@sanger.ac.uk>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch vsoch added container-recipe Apply this label to indicate adding a new container recipe in the "registry" folder to run its tests and removed container-recipe Apply this label to indicate adding a new container recipe in the "registry" folder to run its tests labels Jun 29, 2022
@vsoch
Copy link
Member Author

vsoch commented Jun 30, 2022

I think tests were ok, and the containers just took too long to pull.

shpc/main/client.py Outdated Show resolved Hide resolved
@muffato
Copy link
Contributor

muffato commented Jun 30, 2022

Hi @vsoch . Can I double-check something with you ? I can't manage to install a container any more, even on the main branch. It's not able to separate the tag from the container name

$ git show --oneline 
b1de8eed7 (HEAD -> main, origin/main, origin/HEAD) Merge pull request #556 from marcodelapierre/update/registry/quay.io_pawsey_python
$ git diff
diff --git a/shpc/settings.yml b/shpc/settings.yml
index 557695697..7dfdb106a 100644
--- a/shpc/settings.yml
+++ b/shpc/settings.yml
@@ -5,7 +5,7 @@
 # set a default module system
 # Currently lmod and tcl are supported. To request an additional system, 
 # please open an issue https://github.com/singularityhub/singularity-hpc
-module_sys: lmod
+module_sys: tcl
 
 # config editor
 config_editor: vim
@@ -70,7 +70,7 @@ wrapper_shell: /bin/bash
 # If you want a custom wrapper script for a one-off container, define it there with the script
 # stored alongside the container.yaml.
 wrapper_scripts:
-  enabled: false
+  enabled: true
 
   # use for docker aliases (set to null to disable)
   docker: docker.sh
$ shpc install quay.io/biocontainers/samtools:1.14--hb421002_0
Traceback (most recent call last):
  File "/nfs/users/nfs_m/mm49/.conda/envs/shpc/bin/shpc", line 33, in <module>
    sys.exit(load_entry_point('singularity-hpc', 'console_scripts', 'shpc')())
  File "/nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/shpc/client/__init__.py", line 453, in run_shpc
    main(args=args, parser=parser, extra=extra, subparser=helper)
  File "/nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/shpc/client/install.py", line 30, in main
    cli.install(
  File "/nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/shpc/main/modules/base.py", line 417, in install
    self.container.install(
  File "/nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/shpc/main/container/singularity.py", line 223, in install
    wrapper_scripts = shpc.main.wrappers.generate(
  File "/nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/shpc/main/wrappers/__init__.py", line 71, in generate
    for alias, template_name in listing.items():
AttributeError: 'NoneType' object has no attribute 'items'

Does it work for you ?

vsoch and others added 2 commits June 30, 2022 03:56
Co-authored-by: Matthieu Muffato <mm49@sanger.ac.uk>
Signed-off-by: vsoch <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented Jun 30, 2022

Reproduced - only when I turned on wrapper scripts! The bug was that it needed to default to an {} given that the value was present and None, so I changed to .get or {}.

@muffato
Copy link
Contributor

muffato commented Jun 30, 2022

Thanks, it can now get installed, but overrides don't work. I'm installing the container like this

shpc install quay.io/biocontainers/samtools:1.15--h3843a85_0

I think it's becausetag in shpc/main/modules/base.py:install is still None when config.load_override_file(tag) is called, and it's way below that function that the version is split from the name if there is a colon in it. Because of that load_override_file doesn't think that overrides need to be loaded. Does that make sense ?

@vsoch
Copy link
Member Author

vsoch commented Jun 30, 2022

yep I totally see that! Did you add an overrides file to that container you are testing?

@muffato
Copy link
Contributor

muffato commented Jun 30, 2022

Yes, thanks for checking ;) the overrides files are there, and detected. Here is the content of self.overrides at the beginning of shpc/main/container/config.py:load_override_file:

ordereddict([('1.14--hb421002_0', '1.14--hb421002_0.yaml'), ('1.15--h3843a85_0', '1.15--h3843a85_0.yaml')])

@vsoch
Copy link
Member Author

vsoch commented Jun 30, 2022

@muffato can you give me the complete file set to reproduce? I can also make some test cases from them!

@muffato
Copy link
Contributor

muffato commented Jun 30, 2022

Updated container.yaml and two overrides files: samtools.zip

Unpack in registry/quay.io/biocontainers/samtools/ and try installing the tags listed in the table below. Each will have a different number of aliases, or an environment variable:

Tag Number of aliases 99-shpc.sh
1.13--h8c37831_0 14 no var
1.14--hb421002_0 27 no var
1.15--h3843a85_0 14 REFPATH

…h tag

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@muffato
Copy link
Contributor

muffato commented Jul 1, 2022

@vsoch : these overrides are not meant to make it to the registry. I wrote them just to test different scenarios

@muffato
Copy link
Contributor

muffato commented Jul 1, 2022

Also, seeing that you introduced version earlier in shpc/main/modules/base.py:install, should it not be used instead of tag in these lines too ?

        # Add a .version file to indicate the level of versioning
        self.versionfile.write(os.path.join(self.settings.module_base, uri), tag.name)
        # For Singularity this is a path, podman is a uri. If None is returned
        # there was an error and we cleanup
        if not container_path:
            container_path = self.container.registry_pull(
                module_dir, container_dir, config, tag
            )

@vsoch
Copy link
Member Author

vsoch commented Jul 1, 2022

Agree - and sorry for adding the overrides files to the actual container - I knew the envar wasn't real but I though the aliases might be! Will remove them after dinner (soon).

@vsoch
Copy link
Member Author

vsoch commented Jul 1, 2022

We can't use it here:

if not container_path:
            container_path = self.container.registry_pull(
                module_dir, container_dir, config, tag
            )

because tag needs to be the full tag with digest, etc. But we can use it one level up! This function is admittedly a bit messy - I am going to see if I can neaten it up, at least with respect to not having tag.name and version!

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Jul 1, 2022

okay I tried cleaning it up, and removed the extra overrides! Hopefully it looks better now.

@muffato
Copy link
Contributor

muffato commented Jul 1, 2022

All good for me, I can confirm it all works as intended. Thanks !
Just one thing: you actually need to also revert registry/quay.io/biocontainers/samtools/container.yaml to what it was before (full list of aliases). I edited it that way to sort of make sure I wasn't picking up leftovers from previous installations.

@marcodelapierre
Copy link
Contributor

I will aim to do some quick testing on my end, too, next week.
However feel free to go ahead with the merge any way, as you prefer.

@marcodelapierre
Copy link
Contributor

my testing will be on the lines of singularity + lmod + wrapper scripts + overrides

@marcodelapierre
Copy link
Contributor

done with my testing, all good on my side!

@vsoch vsoch merged commit fc59d9f into main Jul 8, 2022
@vsoch vsoch deleted the add/alias-files branch July 8, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
container-recipe Apply this label to indicate adding a new container recipe in the "registry" folder to run its tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tricky containers: many executables across distinct versions
3 participants