dandi / dandi-cli

DANDI command line client to facilitate common operations
https://dandi.readthedocs.io/
Apache License 2.0
22 stars 28 forks source link

"integration" test for `upload` to use main archive dandiset metadata record #240

Closed yarikoptic closed 4 years ago

yarikoptic commented 4 years ago

I am afraid dandi upload is broken ATM since dandiset metadata structure has changed (now metadata is under "dandiset" key), and there were no coordination with having client account for that + release.

But I guess this dandi cli organize and upload for dandiset still uses old schema. So our tests pass (since testing in isolation) -- but real upload fails since on the main deployment metadata records are different... heh

I think we need a test for that: download 000027 from the main archive, and upload to the local fixture instance.

Meanwhile I will look into just fixing up the code so upload works again.

jwodder commented 4 years ago

The change was in the dandiset.yaml file, correct? What should happen to this file between downloading and uploading? The dandiset has to be registered before uploading, which means modifying the dandiset.yaml. Currently, if the file is present when registering, the old keys will be added to the file next to the new "dandiset" key.

yarikoptic commented 4 years ago

My current working idea is that the problem was introduced by me in #233 where I have tried to "unify" metadata records of "draft" (from girder) dandiset metadata records to look like the ones returned by "published" (from API) ones. But that was done without "account" for upload also uploading that (now changed?) dandiset.yaml as metadata record for the dandiset (folder)... so self-inflicted mess ;-) If I got it right - such as test would be handy, I think on current master it should fail to upload.

jwodder commented 4 years ago

OK, but should the test leave the dandiset.yaml that gets downloaded from the archive alone, should it delete it before calling register(), should register() even be called, what?

yarikoptic commented 4 years ago

that is what I wonder -- can we upload (to local instance) without register call? I think upload will just generate that folder and proceed

jwodder commented 4 years ago

If I try calling upload() without registering (using the latest code on master), it fails because dandiset.yaml doesn't contain a top-level "identifier" key:

/Users/jwodder/dartmouth/dandi-cli/dandi/tests/test_upload.py:161: in test_upload_external_download
    upload(paths=[], dandi_instance=dandi_instance_id, devel_debug=True)
/Users/jwodder/dartmouth/dandi-cli/dandi/upload.py:51: in upload
    ds_identifier = dandiset.identifier
/Users/jwodder/dartmouth/dandi-cli/dandi/dandiset.py:90: in identifier
    return self.metadata["identifier"]
E   KeyError: 'identifier'
yarikoptic commented 4 years ago

yeap, that is the bug due to "unification" I was talking about. Just send a PR with a failing test and I am still thinking about proper "resolution" here.