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.

Sign up for GitHub

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

Jump to bottom

refactor of "add" to generate a container.yaml first to close #519 #520

Merged
merged 5 commits into from
Mar 22, 2022
Merged

refactor of "add" to generate a container.yaml first to close #519 #520

merged 5 commits into from
Mar 22, 2022

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Mar 17, 2022 •
edited

@vsoch
refactor of "add" to generate a container.yaml first to close #519
e31bf2e 
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch vsoch force-pushed the refactor-add branch 2 times, most recently from 1614424 to 9ec4fad Compare March 17, 2022 03:44
@vsoch vsoch mentioned this pull request Mar 17, 2022
Closed
@vsoch
fixing bug with tag created at bottom
2829e77 
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
adding support for adding a docker uri
4a24b59 
this set of changes re-organizes the container template to be under the container
module, and also moves around some code in modules and container (base.py and config.py)
because in development I find the current locations not intuititve enough.

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

vsoch commented Mar 21, 2022

@georgiastuart and @marcodelapierre will you have time to review this? I think it's mostly ready to go!

@georgiastuart
Copy link
Contributor

#519 (comment)

Running into the above issue, unfortunately!

@vsoch
ensure we test add
816feec 
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Mar 21, 2022

@georgiastuart good to go?

@georgiastuart
Copy link
Contributor

We're getting thrashed by storms (Austin, TX) so I unplugged my computer 😂 I'll run through the steps on our cluster ASAP once I'm back online.

@vsoch
Copy link
Member Author

vsoch commented Mar 21, 2022

Ohno!! Please stay safe!

@georgiastuart
Copy link
Contributor

  • Adding a registry entry for a local sif seems to work GREAT! Thanks so much!
  • Adding a remote registry with a different namespace seems to retain the original namespace when it comes to the modules. For example:
shpc add docker://tensorflow/tensorflow:2.7.1-gpu tensorflow:2.7.1-gpu
shpc install tensorflow

results in

---------------------------------------- /opt/ohpc/pub/unpackaged/shpc/modules -----------------------------------------
   tensorflow/tensorflow/2.7.1-gpu/module

I would expect

---------------------------------------- /opt/ohpc/pub/unpackaged/shpc/modules -----------------------------------------
   tensorflow/2.7.1-gpu/module

I also tried:

shpc add docker://tensorflow/tensorflow:2.7.1-gpu georgia/tensorflow:2.7.1-gpu
shpc install georgia/tensorflow

And it also resulted in tensorflow/tensorflow in the module list.

@vsoch
Copy link
Member Author

vsoch commented Mar 22, 2022

Adding a remote registry with a different namespace seems to retain the original namespace when it comes to the modules. For example:

I think I anticipated this - it was a design decision to have the modules derives from the container name, and then to honor the namespace. So should we even support the custom namespace? Do you see reasons, for example, for having two tensorflow installs with the same container? My suggestion / thinking is that we should tweak the docs to not allow a custom namespace for the docker URI and honor/ maintain the namespace until there is a compelling reason to not.

@georgiastuart
Copy link
Contributor

If it's a design decision, I think not supporting custom namespaces at all would be better since I would expect it to carry over to the module name. I think it's more confusing to have a registry entry named one thing and the module named another, but that's just my two cents!

So I think the options should either be

  1. Support custom namespaces and carry through to module name
  2. Don't support custom namespaces at all

@vsoch
Copy link
Member Author

vsoch commented Mar 22, 2022

Ok great! I totally agree. Let me update the PR to support that.

@vsoch
add should only support maintaining the namespace for the docker:// uri
ca4c7dc 
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Mar 22, 2022

@georgiastuart all set with changes!

@georgiastuart
Copy link
Contributor

I think it's good to go! I was able to run through the add process just fine.

@vsoch vsoch merged commit 3abe0e4 into main Mar 22, 2022
@vsoch vsoch deleted the refactor-add branch March 22, 2022 13:18
muffato
muffato reviewed Apr 4, 2022
View reviewed changes
docs/getting_started/user-guide.rst
Comment on lines +1194 to +1196
Also note that ``add`` is only supported for Singularity, as Docker and Podman containers are
typically provided via registries. If you are looking for support for add for another
container technology, please `open a new issue <https://github.com/singularityhub/singularity-hpc/issues>`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @vsoch . This seems to contradict the lines above 🤔 . Is Docker supported ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The old add didn't support docker/podman - e.g., the old add would say "take this .sif on my filesystem and manually move it into shpc." The new add (which generates a container.yaml for you) indeed has support!

muffato added a commit to muffato/singularity-hpc that referenced this pull request Apr 4, 2022
@muffato
shpc add now supports Docker
987b7b6 
cf singularityhub#520 (comment)
@muffato muffato mentioned this pull request Apr 4, 2022
Merged
vsoch added a commit that referenced this pull request Apr 8, 2022
@vsoch
preview of changes done by shpc update (#541)
586f9e0 
* If the container is deleted, the module has to be deleted too. No need to ask the user (#528)
* Only ask for confirmation once
* The .version file is only in module_dir, not container_dir
* When deleting containers, also delete the parent directories as long as they are empty
* `shpc add` now supports Docker

cf #520 (comment)
* round 2 of attempted updates!
I saved the original registry this time so I should not need to open a new PR for every attempt
* remove debug ipython
* Adding more clear error message when tag is not known (#543)
* adding more clear error message when tag is not known
vsoch added a commit that referenced this pull request Apr 8, 2022
@vsoch
Add support for shpc update (#538)
471c0f9 
* adding support for update
* preview of changes done by shpc update (#541)

* If the container is deleted, the module has to be deleted too. No need to ask the user (#528)
* Only ask for confirmation once
* The .version file is only in module_dir, not container_dir
* When deleting containers, also delete the parent directories as long as they are empty
* `shpc add` now supports Docker

cf #520 (comment)
* round 2 of attempted updates!
I saved the original registry this time so I should not need to open a new PR for every attempt
* remove debug ipython
* Adding more clear error message when tag is not known (#543)
* adding more clear error message when tag is not known

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muffato muffato muffato left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Registry Entry that Points to a Local SIF File
3 participants
@vsoch @georgiastuart @muffato