Reinstallation mode #590

muffato · 2022-09-19T23:01:42Z

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 ?

vsoch

Here is some quick feedback!

shpc/main/modules/base.py

shpc/tests/test_client.py

muffato · 2022-10-02T02:41:22Z

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 · 2022-10-02T03:48:33Z

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

muffato · 2022-10-02T19:37:43Z

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 · 2022-10-02T19:50:43Z

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 · 2022-10-02T20:51:53Z

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 · 2022-10-02T21:07:12Z

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 · 2022-10-02T23:47:34Z

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 · 2022-10-03T00:38:32Z

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 · 2022-10-09T12:53:31Z

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 · 2022-10-09T12:55:27Z

Yes let’s remove the third option I think!

as per singularityhub#590 (comment)

* 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 · 2022-11-09T04:59:03Z

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 ?

vsoch

okay review in! I'll ignore failed tests for now since we are still discussing design. Thanks @muffato !

vsoch · 2022-11-09T12:08:23Z

shpc/client/__init__.py

+        description="reinstall a recipe.",
+        formatter_class=argparse.RawTextHelpFormatter,
+    )
+    reinstall.add_argument(


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).

vsoch · 2022-11-09T12:13:46Z

shpc/client/install.py

@@ -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,


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.

vsoch · 2022-11-09T12:16:14Z

shpc/client/install.py

@@ -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,


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

vsoch · 2022-11-09T12:23:47Z

shpc/client/reinstall.py

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


Suggested change

__copyright__ = "Copyright 2021-2022, Vanessa Sochat"

__copyright__ = "Copyright 2022, Vanessa Sochat"

vsoch · 2022-11-09T12:25:01Z

shpc/client/reinstall.py

+
+    # 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.")


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.")

vsoch · 2022-11-09T12:35:09Z

shpc/main/modules/base.py

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


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!

vsoch · 2022-11-09T12:40:05Z

shpc/main/modules/base.py

+            else:
+                missing = module_name + ":" + version
+        else:
+            missing = module_name


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

vsoch · 2022-11-09T12:40:18Z

shpc/main/modules/base.py

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


Suggested change

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

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

vsoch · 2022-11-09T12:40:45Z

shpc/main/modules/base.py

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


Suggested change

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

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

vsoch · 2022-11-09T12:42:36Z

shpc/main/modules/base.py

+        else:
+            missing = module_name
+
+        if when_missing:


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 · 2022-11-09T12:53:23Z

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!

vsoch reviewed Sep 20, 2022

View reviewed changes

shpc/main/modules/base.py Outdated Show resolved Hide resolved

shpc/tests/test_client.py Show resolved Hide resolved

muffato force-pushed the feature/reinstall branch 3 times, most recently from 61a738f to cc3fbc9 Compare October 2, 2022 02:40

muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 9, 2022

Removed the --view option of shpc install

85c0410

as per singularityhub#590 (comment)

muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 9, 2022

Removed the --view option of shpc install

7afd3ef

as per singularityhub#590 (comment)

muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 9, 2022

Removed the --view option of shpc install

a15d5bc

as per singularityhub#590 (comment)

muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 10, 2022

Removed the --view option of shpc install

3543dd2

as per singularityhub#590 (comment)

muffato added a commit to muffato/singularity-hpc that referenced this pull request Oct 10, 2022

Removed the --view option of shpc install

df5589e

as per singularityhub#590 (comment)

muffato mentioned this pull request Oct 10, 2022

View install commands #596

Merged

muffato mentioned this pull request Nov 5, 2022

Support accessing remote registries via ssh #589

Merged

muffato force-pushed the feature/reinstall branch from cc3fbc9 to 7ff4962 Compare November 9, 2022 04:28

muffato added 7 commits November 9, 2022 04:44

shpc install now does a clean reinstallation

d4f6373

Updated the tests

7d9aa90

Function to find all the modules that can be upgraded

ea7627b

Skeleton for the reinstall and upgrade modes

4ceacf5

First working version

ae97aeb

Restore initial explanation

197c6d5

The --force argument is not used for core installations

7e90d75

muffato added 11 commits November 9, 2022 04:44

Much simpler implementation

11a4493

Call install

aa93eed

Upgrade the code to the latest pre-commit requirements

d1cf57d

Synchronised with the latest version of install

b936038

Currently not in use

6173ee6

More comments

562bb5f

Completed the bulk of reinstall

8d637f4

Not used

0048c17

Implemented --ignore-missing and --uninstall-missing

c70eb85

Do the checks at the tool level as well

a82ff27

Not needed anymore

04b5b31

muffato force-pushed the feature/reinstall branch 2 times, most recently from b330e47 to 04b5b31 Compare November 9, 2022 04:44

vsoch reviewed Nov 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reinstallation mode #590

Reinstallation mode #590

muffato commented Sep 19, 2022 •

edited

Loading

vsoch left a comment

muffato commented Oct 2, 2022

vsoch commented Oct 2, 2022

muffato commented Oct 2, 2022 •

edited

Loading

vsoch commented Oct 2, 2022

muffato commented Oct 2, 2022

vsoch commented Oct 2, 2022

muffato commented Oct 2, 2022

vsoch commented Oct 3, 2022

muffato commented Oct 9, 2022

vsoch commented Oct 9, 2022

muffato commented Nov 9, 2022

vsoch left a comment

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch Nov 9, 2022

vsoch commented Nov 9, 2022

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

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

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

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

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

Reinstallation mode #590

Are you sure you want to change the base?

Reinstallation mode #590

Conversation

muffato commented Sep 19, 2022 • edited Loading

vsoch left a comment

Choose a reason for hiding this comment

muffato commented Oct 2, 2022

vsoch commented Oct 2, 2022

muffato commented Oct 2, 2022 • edited Loading

vsoch commented Oct 2, 2022

muffato commented Oct 2, 2022

vsoch commented Oct 2, 2022

muffato commented Oct 2, 2022

vsoch commented Oct 3, 2022

muffato commented Oct 9, 2022

vsoch commented Oct 9, 2022

muffato commented Nov 9, 2022

vsoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsoch commented Nov 9, 2022

muffato commented Sep 19, 2022 •

edited

Loading

muffato commented Oct 2, 2022 •

edited

Loading