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

Reinstallation mode #590

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Conversation

muffato
Copy link
Contributor

@muffato muffato commented Sep 19, 2022

Closes #501

Hi @vsoch . Working my way through #501. It's not as straightforward as I though it would be !

This is a draft pull-request. So far I've expanded shpc install to do clean re-installations, and I've got a stub function to identify what "reinstall all" will do. Can I get feedback on this first ?

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.

Here is some quick feedback!

shpc/main/modules/base.py Outdated Show resolved Hide resolved
shpc/tests/test_client.py Show resolved Hide resolved
@muffato muffato force-pushed the feature/reinstall branch 3 times, most recently from 61a738f to cc3fbc9 Compare October 2, 2022 02:40
@muffato
Copy link
Contributor Author

muffato commented Oct 2, 2022

Hi. FYI I've got shpc reinstall and shpc upgrade working. The code is very fresh and rough and I want to work more on it, but the core is there.

@vsoch
Copy link
Member

vsoch commented Oct 2, 2022

Nice! Let me know when you’d like a review or want help debugging tests.

@muffato
Copy link
Contributor Author

muffato commented Oct 2, 2022

I feel much more comfortable than when I started, and I think I understand the corner cases.

Could you still please review the following ? That'll help me finish this off.

  • shpc install takes a tool name, and optionally a version. If no version is given, defaults to the latest one.
    • if the tool/version is not yet installed, install as usual
    • if the tool/version is already installed: quit, unless --force is given. In which case, do a proper uninstallation first and then the regular install
    • I will change the view parameter so that multiple views can be requested, which will allow the reinstallation to restore the tool in all necessary views.
  • shpc reinstall takes either a tool name (possibly versioned) or --all
    • scope:
      • if one tool and one version is requested: only do this one version
      • if one tool is requested, but no specific version: do all its versions that are installed
      • if --all is given, list all the tools and versions that are installed
    • procedure:
      1. check whether we still have the required recipes in the registry.
      2. bail out if a recipe is missing, unless --force
      3. call install on all required recipes in "force" mode to force the reinstallation, in the same views as they were
  • shpc upgrade takes either a tool name (cannot be versioned) or --all
    • scope:
      • if one tool is requested: only do this one
      • if --all is given, list all the tools that are installed
    • procedure:
      1. check whether we still have recipes for those tools in the registry.
      2. bail out if a recipe is missing, unless --force
      3. identify the latest version of each recipe
      4. install the latest versions, in "force" mode to force a reinstallation if necessary. The latest version is added to all the views that had any version of that tool
      5. uninstall all the other installed versions

@vsoch
Copy link
Member

vsoch commented Oct 2, 2022

shpc install takes a tool name, and optionally a version. If no version is given, defaults to the latest one.

correct, where latest is determined by the container.yaml.

if the tool/version is not yet installed, install as usual

yes!

if the tool/version is already installed: quit, unless --force is given. In which case, do a proper uninstallation first and then the regular install

yes, although there might be a nice human-friendly error message that says "X is already installed - use --force to uninstall and reinstall." Basically, the user always knows exactly what is going to happen (e.g., doesn't assume it will reinstall the same version, etc.)

I will change the view parameter so that multiple views can be requested, which will allow the reinstallation to restore the tool in all necessary views.

Can you show me what this would look like on the command line? E.g., right now we have:

   shpc view install <view> <module>
$ shpc view install mpi ghcr.io/autamus/clingo

And I'm not seeing how this could be made to accept multiple views without getting ugly/hairy.

shpc reinstall takes either a tool name (possibly versioned) or --all

Okay so:

$ shpc reinstall --all

Case 1: missing a tool in the registry:

We cannot find <module> to reinstall. Use force to continue and permanently uninstall <module>.

And then it seems like there is no option to force but leave the module?

For the rest of reinstall and upgrade I'm having trouble understanding your notes (e.g., with scope and procedure) could you write out the command options and then the possible outcomes verbatim? E.g., it's not clear to me why reinstall/upgrade would do a listing, but if you write out the interaction in more detail I think I'll understand.

@muffato
Copy link
Contributor Author

muffato commented Oct 2, 2022

Can you show me what this would look like on the command line? E.g., right now we have:

   shpc view install <view> <module>
$ shpc view install mpi ghcr.io/autamus/clingo

And I'm not seeing how this could be made to accept multiple views without getting ugly/hairy.

I was only thinking of changing shpc install to allow this:

shpc install quay.io/biocontainters/samtools --view view1 --view view2

Right now, one would have to run two commands:

shpc install quay.io/biocontainters/samtools --view view1
shpc view install view2 quay.io/biocontainters/samtools

Okay so:

$ shpc reinstall --all

Case 1: missing a tool in the registry:

We cannot find <module> to reinstall. Use force to continue and permanently uninstall <module>.

And then it seems like there is no option to force but leave the module?

That's indeed the question: what should happen if a tool and/or version is not found in the registry ? Should it be uninstalled or left there ?
Maybe, rather than --force, the CLI options could explicitly let the user decide what to do: no option (= abort) vs --ignore-missing vs --uninstall-missing


For the rest of reinstall and upgrade I'm having trouble understanding your notes (e.g., with scope and procedure) could you write out the command options and then the possible outcomes verbatim? E.g., it's not clear to me why reinstall/upgrade would do a listing, but if you write out the interaction in more detail I think I'll understand.

What I described is how the command should work internally, as essentially wrappers around other existing internal methods of shpc, namely: list installed modules, find modules in the registry, install a module, and uninstall a module. The commands are not meant to be showing their inner working to the user.
As wrappers, the only thing they may print themselves on stdout/stderr would be info/warning about missing modules/versions. But a lot more may be written on stdout/stderr by the existing install and uninstall methods if I use them as-is, e.g.:

singularity pull --name /nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/containers/quay.io/biocontainers/samtools/1.12--h9aed4be_1/quay.io-biocontainers-samtools-1.12--h9aed4be_1-sha256:5fd5f0937adf8a24b5bf7655110e501df78ae51588547c8617f17c3291a723e1.sif docker://quay.io/biocontainers/samtools@sha256:5fd5f0937adf8a24b5bf7655110e501df78ae51588547c8617f17c3291a723e1
INFO:    Using cached SIF image
Module quay.io/biocontainers/samtools:1.12--h9aed4be_1 was created.
Creating link $module_base/quay.io/biocontainers/samtools/1.12--h9aed4be_1/module.lua -> $views_base/view1/samtools/1.12--h9aed4be_1.lua

or

$container_base/quay.io/biocontainers/samtools/1.12--h9aed4be_1 and all subdirectories have been removed.
$module_base/quay.io/biocontainers/samtools/1.12--h9aed4be_1 and all subdirectories have been removed.
/nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/views/view1/samtools/1.12--h9aed4be_1.lua has been removed.

Imagine that for dozens of modules ! I was considering adding a quiet flag in these to skip those messages and instead have just one line per module, e.g.

$ shpc reinstall quay.io/biocontainers/samtools:1.12--h9aed4be_1
Module quay.io/biocontainers/samtools:1.12--h9aed4be_1 was reinstalled.
$ shpc reinstall quay.io/biocontainers/samtools
Module quay.io/biocontainers/samtools:1.12--h9aed4be_1 was reinstalled.
Module quay.io/biocontainers/samtools1.13--h8c37831_0 was reinstalled.
$ shpc reinstall --all
Module quay.io/biocontainers/samtools:1.12--h9aed4be_1 was reinstalled.
Module quay.io/biocontainers/samtools1.13--h8c37831_0 was reinstalled.
Module quay.io/biocontainers/bcftools:1.14--h88f3f91_0 was reinstalled.

or

$ shpc upgrade quay.io/biocontainers/samtools
Module quay.io/biocontainers/samtools was upgraded to 1.14--hb421002_0
$ shpc reinstall --all
Module quay.io/biocontainers/samtools was upgraded to 1.14--hb421002_0
Module quay.io/biocontainers/bcftools was upgraded to 1.14--h88f3f91_0

@vsoch
Copy link
Member

vsoch commented Oct 2, 2022

I was only thinking of changing shpc install to allow this:
shpc install quay.io/biocontainters/samtools --view view1 --view view2

Hmm - so I don't like that we would provide two ways to do the same thing - that's a confusing client interaction. I also like the consistency of shpc view <command>. If someone really wants to do multiple views, they can just do:

for view in mpi1 mpi2
  do
    shpc view install $view quay.io/biocontainters/samtools
done

But to step back a second - views are symlinks. There isn't really a concept of reinstall that doesn't alter a view. We really should just be updating the views with respect to their reinstall requests, the reason being that there is no concept of doing a reinstall (with a sylinked view) without completely changing that, e.g., "keep the view but do the reinstall" is not possible, so adding/not adding the --view flag is irrelevant. Here is an example interaction that might make sense:

I am starting with samtools installed, and it happens to be under two views (two versions)

$ shpc reinstall quay.io/biocontainters/samtools
You currently have:
   samtools@v1 (view1)
   samtools@v2
   samtools@v3 (view2)
Are you sure you would like to reinstall all versions to update all views (y/n)?

And then they would be like "OH" I really just want to do that specific version...

$ shpc reinstall quay.io/biocontainters/samtools@v1
You currently have:
   samtools@v1 (view1)
Are you sure you would like to reinstall all versions to update all views (y/n)?

For the case where someone wants to change a version installed to a view, this would fall under an upgrade (and not reinstall, if I understand the distinction).

#  Case 1: missing a tool in the registry:
$ shpc reinstall --all
We cannot find <module> to reinstall. Use force to continue and permanently uninstall <module>.

Perfecto. That is clear - if you want to reinstall all and you are missing something? It just gets removed.

That's indeed the question: what should happen if a tool and/or version is not found in the registry ? Should it be uninstalled or left there ?

I do think this would be a reasonable desire (not to remove) and I like your suggestion, e.g.,:

#  Case 1: missing a tool in the registry:
$ shpc reinstall --all
We cannot find <module> to reinstall! Your choices:
> Use --uninstall-missing to permanently uninstall <module> (this would be akin to force, but more specific)
> Use --ignore-missing to uninstall only those that are found and ignore (keep) those that are missing.

And please tweak the language above as you see fit! I think as long as we give a clear message of the current state, and the options available to take, it's a fairly good user interaction.

And yes ! I definitely like the quiet command. It's more verbose now to show exactly what is happening, although that was intended for just one install (and not this multi case) so a --quiet flag would make sense.

@muffato
Copy link
Contributor Author

muffato commented Oct 2, 2022

But to step back a second - views are symlinks. There isn't really a concept of reinstall that doesn't alter a view. We really should just be updating the views with respect to their reinstall requests, the reason being that there is no concept of doing a reinstall (with a sylinked view) without completely changing that, e.g., "keep the view but do the reinstall" is not possible, so adding/not adding the --view flag is irrelevant. Here is an example interaction that might make sense:

And we agree on that. My reinstall and upgrade commands don't have --view options. The example I had there was shpc install which already has --view.
As you said, reinstall needs to preserve the views. For that, the most practical is to make the internal install() method understand multiple view requests (currently it only allows one). And once that's implemented, I would as well expose it in shpc install itself.
Re. your comment about the client having multiple ways of doing things: then there's already a problem ! Right now, one can do

shpc install $module
shpc view install $view $module

or

shpc install --view $view $module

Would you want to remove the --view option of shpc install ?

@vsoch
Copy link
Member

vsoch commented Oct 3, 2022

Gotcha - I missed that it was just for install.

shpc install $module
shpc view install $view $module

or

shpc install --view $view $module

These are subtly different - the first is installing to core shpc, the second is just interacting with a view, and the third is installing (and adding to a view to save time). As a user, at least, I'd expect to have both of those. If anything, I'd probably update the client to remove the --view flag entirely to be more consistent.

@muffato
Copy link
Contributor Author

muffato commented Oct 9, 2022

Hi @vsoch . I found that shpc view install $view $module too does a core installation transparently, and it is a design choice: https://github.com/singularityhub/singularity-hpc/blob/main/shpc/client/view.py#L167-L168
which gives 3 ways of installing a module into a view

# Intended method
shpc install $module
shpc view install $view $module
# Shortcut n˚1
shpc view install $view $module
# Shortcut n˚2
shpc install --view $view $module

But, more importantly, both shortcuts (shpc view install $view $module and shpc install --view $view $module) ignore the default_view setting when doing the core installation.

My view on this:

  • I would first make sure the default_view setting is always honoured.
  • I'm OK with view install doing a core installation, but can remove that behaviour if you want.
  • I dislike the --view option of shpc install accepting a single value. Either the option is removed altogether, or it can be repeated so that one can list all the views to add the module to.

@vsoch
Copy link
Member

vsoch commented Oct 9, 2022

Yes let’s remove the third option I think!

muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 9, 2022
muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 9, 2022
muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 9, 2022
muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 10, 2022
muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 10, 2022
@muffato muffato mentioned this pull request Oct 10, 2022
vsoch added a commit that referenced this pull request Oct 12, 2022
* Removed the `--view` option of `shpc install`

as per #590 (comment)

* bugfix: always install in the default view, alongside additionally requested views
Renamed the variables to make it clear what is a view and what is a view name.

* Print what bash is running
* Test views on tcsh too
* Moved the code to install a module into a view to a separate function
* addition of intermediate module class

This will help to carry around some common data
attributes and functions, and reduce redundancy within
the ModuleBase class. It also allows for providing
fewer variables to the rendered templates since
they can come from the module.

* ensure we maintain parsed name
* Moved the view uninstallation to a separate function to mirror view_install
* No need to make this local variable
* Use parsed_name for consistency
* Document the "force" flags

Signed-off-by: vsoch <[email protected]>
Co-authored-by: vsoch <[email protected]>
@muffato muffato force-pushed the feature/reinstall branch 2 times, most recently from b330e47 to 04b5b31 Compare November 9, 2022 04:44
@muffato
Copy link
Contributor Author

muffato commented Nov 9, 2022

Still some work to do (and I need to fix the tests !) but shpc reinstall works. Could I get a review on it, @vsoch ?

There's also one difficulty I haven't managed to overcome. Modules that were installed against a preexisting container image won't match their container.yaml and ca't be reinstalled the normal way. How would you suggest dealing with that ? Parsing the existing module to find out the path to the container ?

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 review in! I'll ignore failed tests for now since we are still discussing design. Thanks @muffato !

description="reinstall a recipe.",
formatter_class=argparse.RawTextHelpFormatter,
)
reinstall.add_argument(
Copy link
Member

Choose a reason for hiding this comment

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

This argument is likely redundant with one that exists - find a match and then loop through the list to add it is what I usually do! --all might be something we have too (can't remember off the top of my head).

@@ -27,7 +27,6 @@ def main(args, parser, extra, subparser):
# And do the install
cli.install(
args.install_recipe,
force=args.force,
container_image=args.container_image,
Copy link
Member

Choose a reason for hiding this comment

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

Okay so I see you removed force but we've added "allow_reinstall" - I think "force" implies that, so let's keep the same functionality but have the variable named force to be consistent with the design of other functions. And I don't think there is any reason we should remove the ability to do install --force from the command line, that's also something someone would intuitively want to do.

@@ -36,5 +35,4 @@ def main(args, parser, extra, subparser):
cli.settings.default_view,
args.install_recipe,
force=args.force,
container_image=args.container_image,
Copy link
Member

Choose a reason for hiding this comment

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

okay, and this will work because we are symlinking the module file, which has the path to the container, wherever it may be!

@@ -0,0 +1,46 @@
__author__ = "Vanessa Sochat"
__copyright__ = "Copyright 2021-2022, Vanessa Sochat"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
__copyright__ = "Copyright 2021-2022, Vanessa Sochat"
__copyright__ = "Copyright 2022, Vanessa Sochat"


# It doesn't make sense to give a module name and --all
if args.reinstall_recipe and args.all:
logger.exit("Conflicting arguments reinstall_recipe and --all, choose one.")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the user knows what the argument name is, at least without looking.

Suggested change
logger.exit("Conflicting arguments reinstall_recipe and --all, choose one.")
logger.exit("Conflicting arguments: choose a single module name to reinstall OR --all.")

# Reinstall the required version(s) one by one
for version in versions:
self._reinstall(module_name, version, when_missing)
else:
Copy link
Member

Choose a reason for hiding this comment

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

okay I see what you are doing - without a name we hit the else and this is "reinstall all." I will think more about this one - I don't have a better design idea at 5:30am! But not reading the code, it's not obvious this function handles single / all use cases (beyond the comment in the docstring). I think given that we are ultimately looping through a list of modules (and the versions come from a lookup) or would otherwise just be None) it might be nice to have logic to populate the list of modules and versions, and one common loop through them to reinstall. The other option is to have two clearly distinguished functions, but that might be overkill because we already have two!

Comment on lines +501 to +504
else:
missing = module_name + ":" + version
else:
missing = module_name
Copy link
Member

Choose a reason for hiding this comment

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

The double nested if - is there a better way to write this? I think we can just plop the entire thing (name and version) into missing? And since we return above we don't need the elses.

Suggested change
else:
missing = module_name + ":" + version
else:
missing = module_name
missing = (module_name + ":" + version) if version else module_name

if result:
config = container.ContainerConfig(result)
if version in config.tags:
return self.install(module_name + ":" + version, allow_reinstall=True)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return self.install(module_name + ":" + version, allow_reinstall=True)
return self.install(module_name + ":" + version, force=True)

if when_missing:
if when_missing == "ignore":
logger.info(
"%s is not in the Registry any more. Ignoring as instructed."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"%s is not in the Registry any more. Ignoring as instructed."
"%s is not in the registry any more. Ignoring as instructed."

else:
missing = module_name

if when_missing:
Copy link
Member

Choose a reason for hiding this comment

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

Can we collapse these into a single if elif elif, e.g.,:

if not when_missing:
     ...
elif when_missing == "ignore":
    ...
elif when_missing == "uninstall":

else:
   # maybe mention whatever they provided isn't a valid option?

@vsoch
Copy link
Member

vsoch commented Nov 9, 2022

Modules that were installed against a preexisting container image won't match their container.yaml and ca't be reinstalled the normal way. How would you suggest dealing with that ? Parsing the existing module to find out the path to the container ?

We could certainly try! If we can clean up the module files a bit to make it easy to parse (and find the container path), this could actually be feasible do do. Would we honor the previous location? Pull a new one? I think there are two cases:

  • Reinstall updated version - and for this I'm tempted to say if it's not in the module /container folder maintained by shpc, to just leave it there. E.g., imagine the BioContainers case - we wouldn't want to try and delete / repull down to there. We'd probably try a repull, and if that fails, we could either give an error message or allow the user to use the previous SIF
  • Reinstall the same version - parse the module file to get the exact container path (and re-use). This would maybe be the fallback for the case above.

Let me know what you think!

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.

Command to rebuild all modules / wrapper scripts
2 participants