singularityhub / singularity-cli

streamlined singularity python client (spython) for singularity
https://singularityhub.github.io/singularity-cli/
Mozilla Public License 2.0
57 stars 31 forks source link

spython pull if newer version available, otherwise ignore #48

Closed mwiens91 closed 6 years ago

mwiens91 commented 6 years ago

Hey @vsoch,

I looked into this a bit myself, but wasn't able to find out if what I'm looking for is possible: basically, for the Client.pull method (https://github.com/singularityhub/singularity-cli/blob/master/spython/main/pull.py), is it possible to to have the following logic:

  1. If the image has already been pulled and is the latest version, do nothing.
  2. If the image has already been pulled and is the latest version, pull the image and overwrite the existing image.
  3. If the image has not already been pulled, pull the image.

Thank you! (and keep up the great work :smiley:)

vsoch commented 6 years ago

This should already be supported by native singularity, given that you pull with --commit or --hash. If you have the same SINGULARITY_CACHEDIR set and the image already exists, I don't think it will pull it again.

mwiens91 commented 6 years ago

Okay, cool. I imagine that --commit and --hash are available within spython, too? I'll check this out tomorrow and report back if there are any issues.

vsoch commented 6 years ago

Yeah!!

https://singularityhub.github.io/singularity-cli/commands-images#pull <--

mwiens91 commented 6 years ago
Traceback (most recent call last):
  File "./singularity_pull_test.py", line 8, in <module>
    name_by_commit=True)
TypeError: pull() got an unexpected keyword argument 'name_by_commit'

The name_by_commit and name_by_hash don't appear to be implemented in spython:

https://github.com/singularityhub/singularity-cli/blob/f9fdcc9b223485a248f189606c47588c2fa46cd5/spython/main/pull.py#L26-L33

I'll plan to submit a pull request today for this.

vsoch commented 6 years ago

Ah this must have been old functionality for when it was party of the singularity package (that I forgot to implement!) I can definitely get this added soon if you don't have bandwidth, otherwise looking forward to the PR!

mwiens91 commented 6 years ago

I don't mind working on it.

One issue is in returning the final image path when using --commit or --hash, which can't really be determined until after the image is pulled. One way of getting the image path is to process the output text from the command, but this would presumably only work nicely with stream=False. Not sure what the best way forward is here.

Another issue is to do with Singularity native: it appears that when you use --commit or --hash it always overwrites the existing image, even if it's the latest available version for that image. I guess I should probably raise an issue on the main Singularity repo about this.

vsoch commented 6 years ago

There are two ways to go about it! For shub containers, tou actually can get this information, at least from singularity hub, via the api! Take a look here:

http://singularity-hub.org/api/container/details/vsoch/hello-world:latest@e279432e6d3962777bb7b5e8d54f30f4347d867e

Notice that you have "version" (hash) and "commit." The second way (and what has traditionally been done for other kinds of containers) is for the singularity client to manage the name, and then for spython (or other) to parse what is returned. For example, you can always add the --name flag and define SINGULARITY_PULLFOLDER or SINGULARITY_CACHEDIR to control where it's pulled and how it's named, and then take a hash, and rename it with shutil,move(). Would that work?

For the latter part about it always replacing the file, +1 on opening an issue! You can catch it here with the client, but it's better addressed at the root. har harhar, linux container humor :P

vsoch commented 6 years ago

haha yep, here it is! https://github.com/singularityware/singularity-python/blob/0cda933ee69da15ef4da16043a522a438fdaf91d/singularity/cli/__init__.py#L276 You should be able to take advantage of a lot of that code :)

mwiens91 commented 6 years ago

Sounds good. I'll have a look at it tomorrow.

mwiens91 commented 6 years ago

I can definitely get this added soon if you don't have bandwidth

Yeah, this is a slight issue for me right now. I suspect I'll have some time in a week or so from now.

vsoch commented 6 years ago

hey no worries about this ok? I am very happy to do the changes for you. I put together a quick PR here:

https://github.com/singularityhub/singularity-cli/pull/53

Would you care to test? I have to run now but I'll be able to check up later. Hope your schedule clears up and you can relax a bit!

vsoch commented 6 years ago

And if you are able, do you want to suggest a test that we can add for the CI? Just for the commit / hash naming. It can be something hard coded (e.g., a hash / commit that you pull in advance and want to make sure returns the same) if that works best. The shub://vsoch/hello-world image isn't going to change any time soon, and even better you can pull and specify the commit or hash so you are also sure it won't!

vsoch commented 6 years ago

ping @mwiens91 ! I made this PR a week ago, just updated with recent changes. Could you please take a look and test if it's working for you? Thank you! --> #53