Skip to content

Comments

View install commands#596

Merged
vsoch merged 11 commits intosingularityhub:mainfrom
muffato:view_install
Oct 12, 2022
Merged

View install commands#596
vsoch merged 11 commits intosingularityhub:mainfrom
muffato:view_install

Conversation

@muffato
Copy link
Contributor

@muffato muffato commented Oct 10, 2022

Hi.

Here is a pull-request to address #590 (comment) and #590 (comment)

  1. shpc install doesn't accept a --view argument anymore.
  2. shpc view install now honours the default view, as set in the settings.
  3. I've renamed the view and disable_view arguments of the install method to resp. extra_view and disable_default_view to clarify what they refer to.
  4. I've updated the CI to test views on tcsh too, so that both shells test (almost1) entirely the same thing.
  5. Both CI tests clean up their modules / views to ensure nothing bleeds from the first one to the next

Footnotes

  1. tcsh still doesn't run shpc test. I've tried to add it, but couldn't manage to make it work: it complains that module is not found even though the module commands work. I found something weird: shpc test always runs the test script with bash even if the user requested a different test shell. I tried using the test shell throughout but it didn't help. And anyway, tcsh is not allowed as a shell. ↩

vsoch
vsoch reviewed Oct 10, 2022
View reviewed changes
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.

Thanks @muffato ! Some comments below!

@muffato
Copy link
Contributor Author

muffato commented Oct 10, 2022

The latest version makes most of the comments obsolete

vsoch
vsoch reviewed Oct 10, 2022
View reviewed changes
vsoch
vsoch reviewed Oct 10, 2022
View reviewed changes
@vsoch
Copy link
Member

vsoch commented Oct 10, 2022

okay I started a rough idea - too tired to finish tonight but will try to soon this week!

@vsoch
Copy link
Member

vsoch commented Oct 10, 2022

https://github.com/muffato/singularity-hpc/pull/4#issue-1403058615

vsoch added 2 commits October 11, 2022 02:36
@vsoch @muffato
addition of intermediate module class
048f677 
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.

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch @muffato
ensure we maintain parsed name
2a1331a 
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
muffato
muffato commented Oct 11, 2022
View reviewed changes
shpc/client/view.py
for install_module in install_modules:
cli.install(install_module, view=view_name, disable_view=False, force=force)

# TODO: can we cut out early if already installed?
Copy link
Contributor Author

@muffato muffato Oct 11, 2022

Choose a reason for hiding this comment

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

OK, I'll keep that in mind when implementing reinstall and upgrade. I could split force into specific flags (like ignore-missing and uninstall-missing in upgrade)

@muffato
Copy link
Contributor Author

muffato commented Oct 11, 2022 •
edited
Loading

Here it is, I've split view_uninstall from uninstall like I did for install

muffato
muffato commented Oct 11, 2022
View reviewed changes
Copy link
Contributor Author

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

I've annotated the occurrences of force. I'll probably tidy some of them up in #590

vsoch
vsoch reviewed Oct 11, 2022
View reviewed changes
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.

This is a heck of a lot neater - I really like it! Some small tweaks, and then given you've taken the basic changes for a spin and are happy, I'm good to merge this into main and keep working. I think your other PR #590 is going to add some really good functionality, and we can finish up there and ping Marco to see what he thinks before doing a release.

And I'm also down to keep iterating on improving the new Module class to better handle some of the logic - let me know if you see / think of something that can be improved and I'll be on it!

muffato and others added 3 commits October 11, 2022 22:22
@muffato @vsoch
No need to make this local variable
b89b823 
Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
@muffato @vsoch
Use parsed_name for consistency
eec370c 
config.name was not wrong

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
@muffato
Document the "force" flags
7e76b6b
@muffato
Copy link
Contributor Author

muffato commented Oct 11, 2022

Looks good to me ! I'll rebase #590 once this is merged.

vsoch
vsoch approved these changes Oct 12, 2022
View reviewed changes
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.

OK this is a lot of changes - but I'm good as well! Let's merge this! 🥳

@vsoch vsoch merged commit ceca803 into singularityhub:main Oct 12, 2022
@muffato muffato deleted the view_install branch October 12, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vsoch vsoch vsoch approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@muffato @vsoch