-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
base: main
Are you sure you want to change the base?
Reinstallation mode #590
Changes from all commits
d4f6373
7d9aa90
ea7627b
4ceacf5
ae97aeb
197c6d5
7e90d75
11a4493
aa93eed
d1cf57d
b936038
6173ee6
562bb5f
8d637f4
0048c17
c70eb85
a82ff27
04b5b31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
keep_path=args.keep_path, | ||
) | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,46 @@ | ||||||
__author__ = "Vanessa Sochat" | ||||||
__copyright__ = "Copyright 2021-2022, Vanessa Sochat" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
__license__ = "MPL 2.0" | ||||||
|
||||||
import shpc.utils | ||||||
from shpc.logger import logger | ||||||
|
||||||
|
||||||
def main(args, parser, extra, subparser): | ||||||
|
||||||
from shpc.main import get_client | ||||||
|
||||||
shpc.utils.ensure_no_extra(extra) | ||||||
|
||||||
cli = get_client( | ||||||
quiet=args.quiet, | ||||||
settings_file=args.settings_file, | ||||||
module_sys=args.module_sys, | ||||||
container_tech=args.container_tech, | ||||||
) | ||||||
|
||||||
# Update config settings on the fly | ||||||
cli.settings.update_params(args.config_params) | ||||||
|
||||||
# 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.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
# One option must be present | ||||||
if not args.reinstall_recipe and not args.all: | ||||||
logger.exit("Missing arguments: provide reinstall_recipe or --all.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if args.ignore_missing and args.uninstall_missing: | ||||||
logger.exit( | ||||||
"Conflicting arguments --ignore-missing and --uninstall-missing, choose one." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
|
||||||
# And do the reinstall | ||||||
cli.reinstall( | ||||||
args.reinstall_recipe, | ||||||
when_missing=( | ||||||
"ignore" | ||||||
if args.ignore_missing | ||||||
else "uninstall" | ||||||
if args.uninstall_missing | ||||||
else None | ||||||
), | ||||||
) |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -375,15 +375,19 @@ def get_module(self, name, container_image=None, keep_path=False): | |||||||||||
return module | ||||||||||||
|
||||||||||||
def install( | ||||||||||||
self, name, force=False, container_image=None, keep_path=False, **kwargs | ||||||||||||
self, | ||||||||||||
name, | ||||||||||||
allow_reinstall=False, | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I mentioned above, let's make this
Suggested change
|
||||||||||||
container_image=None, | ||||||||||||
keep_path=False, | ||||||||||||
**kwargs | ||||||||||||
): | ||||||||||||
""" | ||||||||||||
Given a unique resource identifier, install a recipe. | ||||||||||||
|
||||||||||||
For lmod, this means creating a subfolder in modules, pulling the | ||||||||||||
container to it, and writing a module file there. We've already | ||||||||||||
grabbed the name from docker (which is currently the only supported). | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
"force" is currently not used. | ||||||||||||
""" | ||||||||||||
# Create a new module | ||||||||||||
module = self.get_module( | ||||||||||||
|
@@ -393,6 +397,23 @@ def install( | |||||||||||
# We always load overrides for an install | ||||||||||||
module.load_override_file() | ||||||||||||
|
||||||||||||
# Check previous installations of this module | ||||||||||||
if os.path.exists(module.module_dir): | ||||||||||||
if not allow_reinstall: | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
logger.exit( | ||||||||||||
"%s is already installed. Do `shpc reinstall` to proceed with a reinstallation." | ||||||||||||
% module.tagged_name | ||||||||||||
) | ||||||||||||
logger.info("%s is already installed. Reinstalling." % module.tagged_name) | ||||||||||||
# Don't explicitly remove the container, since we still need it, | ||||||||||||
# though it may still happen if shpc is configured to store | ||||||||||||
# containers and modules in the same directory | ||||||||||||
self._uninstall( | ||||||||||||
module.module_dir, | ||||||||||||
self.settings.module_base, | ||||||||||||
"$module_base/%s" % module.name, | ||||||||||||
) | ||||||||||||
|
||||||||||||
# Create the module and container directory | ||||||||||||
utils.mkdirp([module.module_dir, module.container_dir]) | ||||||||||||
|
||||||||||||
|
@@ -421,12 +442,12 @@ def install( | |||||||||||
logger.info("Module %s was created." % module.tagged_name) | ||||||||||||
return module.container_path | ||||||||||||
|
||||||||||||
def view_install(self, view_name, name, force=False, container_image=None): | ||||||||||||
def view_install(self, view_name, name, force=False): | ||||||||||||
""" | ||||||||||||
Install a module in a view. The module must already be installed. | ||||||||||||
Set "force" to True to allow overwriting existing symlinks. | ||||||||||||
""" | ||||||||||||
module = self.get_module(name, container_image=container_image) | ||||||||||||
module = self.get_module(name) | ||||||||||||
|
||||||||||||
# A view is a symlink under views_base/$view/$module | ||||||||||||
if view_name not in self.views: | ||||||||||||
|
@@ -440,3 +461,58 @@ def view_install(self, view_name, name, force=False, container_image=None): | |||||||||||
# Don't continue if it exists, unless force is True | ||||||||||||
view.confirm_install(module.module_dir, force=force) | ||||||||||||
view.install(module.module_dir) | ||||||||||||
|
||||||||||||
def reinstall(self, name, when_missing=None): | ||||||||||||
""" | ||||||||||||
Reinstall the module, or all modules | ||||||||||||
""" | ||||||||||||
if name: | ||||||||||||
module_name, _, version = name.partition(":") | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some redundancy here with functionality - could we parse the name using our Module class? |
||||||||||||
# Find all the versions currently installed | ||||||||||||
installed_modules = self._get_module_lookup( | ||||||||||||
self.settings.module_base, self.modulefile, module_name | ||||||||||||
) | ||||||||||||
if (module_name not in installed_modules) or ( | ||||||||||||
version and version not in installed_modules[module_name] | ||||||||||||
): | ||||||||||||
logger.exit("%s is not installed. Nothing to reinstall." % name) | ||||||||||||
versions = [version] if version else installed_modules[module_name] | ||||||||||||
# Reinstall the required version(s) one by one | ||||||||||||
for version in versions: | ||||||||||||
self._reinstall(module_name, version, when_missing) | ||||||||||||
else: | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||||||||||||
# Reinstall everything that is currently installed | ||||||||||||
installed_modules = self._get_module_lookup( | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I wonder if there is some way to simplify the logic here (and not call the function in both blocks but use a shared call?) See duplicate of (almost) identical code always makes me pause! |
||||||||||||
self.settings.module_base, self.modulefile | ||||||||||||
) | ||||||||||||
for module_name, versions in installed_modules.items(): | ||||||||||||
for version in versions: | ||||||||||||
self._reinstall(module_name, version, when_missing) | ||||||||||||
|
||||||||||||
def _reinstall(self, module_name, version, when_missing): | ||||||||||||
""" | ||||||||||||
Reinstall (and possibly upgrade) all the current modules, possibly filtered by pattern. | ||||||||||||
""" | ||||||||||||
result = self.registry.find(module_name) | ||||||||||||
if result: | ||||||||||||
config = container.ContainerConfig(result) | ||||||||||||
if version in config.tags: | ||||||||||||
return self.install(module_name + ":" + version, allow_reinstall=True) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
else: | ||||||||||||
missing = module_name + ":" + version | ||||||||||||
else: | ||||||||||||
missing = module_name | ||||||||||||
Comment on lines
+501
to
+504
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||
|
||||||||||||
if when_missing: | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||||||||
if when_missing == "ignore": | ||||||||||||
logger.info( | ||||||||||||
"%s is not in the Registry any more. Ignoring as instructed." | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
% missing | ||||||||||||
) | ||||||||||||
elif when_missing == "uninstall": | ||||||||||||
self.uninstall(module_name + ":" + version, force=True) | ||||||||||||
else: | ||||||||||||
logger.exit( | ||||||||||||
"%s is not in the Registry any more. Add --uninstall-missing or --ignore-missing." | ||||||||||||
% missing | ||||||||||||
) |
There was a problem hiding this comment.
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).