Arvados

Notes:

It should now correctly respect --project-uuid for collections, templates, pipeline instances, and docker image metadata.

If a collection already exists on the target system, it will create a new collection in the target project using the existing manifest_text from the existing collection.

Reviewing 74784b3.

• The code to take a source collection and turn it into a collection on dst (including necessary munging) seems long enough to be worth DRYing up at this point.
• Why did the log level of the "Skipping message" collection upgrade from DEBUG to INFO? It seems like this could occasionally make the program noticeably more verbose, and I'm not sure how it helps the user to know this.

While I was reading this I noticed some other bugs in the code you're touching. If you're up for fixing them, I'd say go for it. If not, we should file separate bugs about them.

• The collection create calls don't respect args.retries.
• The docker_image_hash link is copied incorrectly. The name should be Docker's native "image ID," not the portable data hash of the corresponding collection.

Thanks.

Brett Smith wrote:

Reviewing 74784b3.

• The code to take a source collection and turn it into a collection on dst (including necessary munging) seems long enough to be worth DRYing up at this point.

Done.

• Why did the log level of the "Skipping message" collection upgrade from DEBUG to INFO? It seems like this could occasionally make the program noticeably more verbose, and I'm not sure how it helps the user to know this.

Took that out entirely and revamped the copying the logic to actually check the owner_uuid, name and portable_data_hash of existing collection at the destination before deciding whether to make a copy.

While I was reading this I noticed some other bugs in the code you're touching. If you're up for fixing them, I'd say go for it. If not, we should file separate bugs about them.

• The collection create calls don't respect args.retries.

Fixed.

• The docker_image_hash link is copied incorrectly. The name should be Docker's native "image ID," not the portable data hash of the corresponding collection.

Fixed. Also looks for (and copies) Docker metadata any time it is copying a collection and not only when copying pipelines.

Bonus assertion added to keep.py because it turns out arv-copy was passing unicode strings to KeepClient.put() (whoops!)

Now at a47dc2b

Reviewing a47dc2b

Peter Amstutz wrote:

• The docker_image_hash link is copied incorrectly. The name should be Docker's native "image ID," not the portable data hash of the corresponding collection.

Fixed. Also looks for (and copies) Docker metadata any time it is copying a collection and not only when copying pipelines.

Thanks, that basically covers #5294.

Bonus assertion added to keep.py because it turns out arv-copy was passing unicode strings to KeepClient.put() (whoops!)

Please change this condition to isinstance(data, str) to accept subclasses, and add a test.

The new line args.project_uuid = dst_arv.users().current().execute()["uuid"] should respect args.retries.

Thanks.

Brett Smith wrote:

Please change this condition to isinstance(data, str) to accept subclasses, and add a test.

Fixed.

The new line args.project_uuid = dst_arv.users().current().execute()["uuid"] should respect args.retries.

Fixed.

Now at 7b0d393

Peter Amstutz wrote:

Brett Smith wrote:

Please change this condition to isinstance(data, str) to accept subclasses, and add a test.

Fixed.

The new line args.project_uuid = dst_arv.users().current().execute()["uuid"] should respect args.retries.

Fixed.

Both of these changes are good. Unfortunately, the change to make CollectionWriter.manifest_text() encode the return string isn't going to fly. The entire interface to CollectionWriter is already bytes-based, so users need to pass in encoded strings for filenames already. For anybody already passing in non-ASCII, this change will basically attempt a double-encode and break:

>>> import arvados
>>> cw = arvados.CollectionWriter()
>>> with cw.open(u'♥'.encode('utf-8')) as cf:
...     pass
...
>>> cw.manifest_text()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "arvados/collection.py", line 588, in manifest_text
    return manifest.encode("utf-8")
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 41: ordinal not in range(128)

If you're okay reverting that change, I think this is good to merge. If there's some nuance I'm missing, let's talk about it. Thanks.

• Don't encode manifest_text by default to avoid double-encode faults.
• Coerce unicode strings to ascii in put(). Will raise an error if it is not a valid conversion.
• Use result.content (returns literal result bytes) instead result.text (returns unicode) when processing Keep responses.
• Updated test.

Peter Amstutz wrote:

• Updated test.

Please assert that the first put returns the right hash (so we're sure it encodes correctly), and ditch the noop foo_locator assignments here. Then go ahead and merge. Thanks.