RF/ENH: download - reimplementation to support original and upcoming UI/API #134

yarikoptic · 2020-07-22T19:51:37Z

RFed downloaders. I thought to make it pretty but it ended up quite messy code. Default UI though looks familiar since uses pyout as for ls and upload.

Should be well tested before releasing!

removes use of tqdm, Closes tqdm: errors in the tests with "AssertionError: attempt to release recursive lock not owned by thread" #111

TODOs

finish support logic for actually downloading via new API
on the fly (during download) digesting (checksum) of the data
implement frontend (pyout) analogous to upload
verify that upload or anything else got broken (we have some upload unittests going, so let's hope it is all good)
May be later (for now "static" rendering from pyout if not interactive should suffice): provide an alternative, simpler "format" for really basic reporting so could be used in CI etc. Probably will just report as json stream or alike.

TODOs after

(not here) possibly add proper "sync" etc functionality
while at it simplify all the mess here. May be it is not worth bothering trying to start downloading while still traversing remote dataset (actually I am not longer sure it actually works that way since I think I have not seen counts in the summary having > - I must have broke it).
ETA would be wrong ATM for downloads where some files are already present. Probably while reworking for "sync" functionality - a fix would come naturally.

codecov · 2020-07-22T19:52:40Z

Codecov Report

Merging #134 into master will increase coverage by 2.13%.
The diff coverage is 83.69%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   78.11%   80.24%   +2.13%     
==========================================
  Files          42       45       +3     
  Lines        3303     3817     +514     
==========================================
+ Hits         2580     3063     +483     
- Misses        723      754      +31

Flag	Coverage Δ
#unittests	`80.24% <83.69%> (+2.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dandi/girder.py	`82.01% <74.72%> (+12.70%)`	⬆️
dandi/download.py	`79.15% <78.68%> (+6.42%)`	⬆️
dandi/dandiapi.py	`85.71% <85.71%> (ø)`
dandi/utils.py	`74.80% <94.11%> (+1.36%)`	⬆️
dandi/cli/cmd_download.py	`85.71% <100.00%> (+1.09%)`	⬆️
dandi/cli/cmd_ls.py	`68.75% <100.00%> (ø)`
dandi/cli/formatter.py	`92.85% <100.00%> (ø)`
dandi/consts.py	`100.00% <100.00%> (ø)`
dandi/support/iterators.py	`100.00% <100.00%> (ø)`
dandi/support/pyout.py	`75.40% <100.00%> (+0.40%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dfcb6a...20718e2. Read the comment docs.

yarikoptic · 2020-08-01T01:14:04Z

@satra @mgrauer -- "Perfect is the enemy of good". I a bit "overengineered", but it should be a workable solution. Needs testing.
Give it a try while its "hot" and I can figure out what I have done (hopefully ;)).

For downloads of "released" dandisets dandi/dandi-publish#112 should be addressed first though.

yarikoptic · 2020-08-01T01:21:55Z

here is some kind of a demo:

mgrauer · 2020-08-03T15:12:31Z

Thanks for the asciicast, that is some fun viewing 🍿

The UI looks nice, especially the way it updates multiple lines in parallel.

I have a couple points of discussion I'd like to raise for the CLI commands, they don't necessarily need to be addressed on this PR, but at least to start thinking about them.

having the output folder can make it harder to give the user a specific command from the GUI that they can then give to the CLI, because the output folder is specific to the user's setup. i.e. it is easy to tell the user, from the GUI dandi download DANDI:000007 but not dandi download -o testdandi DANDI:000007. Maybe we could put the folder flag at the end or even put a template value in, when we display the command to run from the GUI, e.g. dandi download DANDI:000007 -output <OUTPUT_DIR> ?
there is still some inconsistency in how people download drafts vs released dandisets currently:

dandi download -o testdandi DANDI:000007 for latest release
dandi download -o testdandi 'http://GUI_url_to_draft' for a draft

Could we change this to something like the following (I'm omitting output folder to simplify):

dandi download DANDI:000007 downloads latest published version
dandi download DANDI:000007:0.200714.1807 downloads this specific published version
dandi download DANDI:000007:draft downloads the draft dandiset in its current form

At first I was thinking that maybe dandi download DANDI:000007 should download a draft if there are no published versions, but upon thinking about it more, the draft is a different enough beast from a release (it is both volatile and mutable) that I'd want the user to have to be explicit about dealing with drafts.

satra · 2020-08-03T15:29:17Z

DANDI:000007:draft

@mgrauer - this is not an acceptable value for the dandi prefix presently in identifiers.org, so we would either have to add that as an allowed regex or force people not to use identifiers.org for drafts. also it would be / not : for version information

dandi download DANDI:000007/0.200714.1807 downloads this specific published version
dandi download DANDI:000007/draft downloads the draft dandiset in its current form

having a persistent identifier for a draft seems a little weird. so i'm hesitant to add that there, but open to thoughts. instead we could provide the following option through the CLI

$ dandi download DANDI:000007
Please select from available versions:
0) Current draft dataset
1) 0.200714.1807
2) ...

and the draft url would always be: https://dandiarchive.org/dandiset/000007/draft. this would also force people to publish a version to use an identifier.

satra · 2020-08-03T15:31:30Z

also, we would never mint a doi for a dandiset which does not have a published version. i can also add a note to the redirector to specifically point to the draft URL for an unpublished dandiset.

yarikoptic · 2020-08-03T16:17:43Z

@jwodder I invite you to try this one as well.

yarikoptic · 2020-08-05T20:31:29Z

e.g. dandi download DANDI:000007 -output <OUTPUT_DIR> ?

that is already supported out of the box since that is how argparse (which I hope click uses underneath) does parsing - it does allow --options to be used at the end of the line. It is very useful for options which could take arbitrary number of values :

$> dandi download DANDI:000007 --output-dir myother
2020-08-05 16:30:24,676 [    INFO] Traversing remote dandisets (000007) recursively
PATH                 SIZE DONE    DONE% CHECKSUM STATUS MESSAGE   
dandiset.yaml                                    done   updated   
Summary:                  0 Bytes                1 done 1 updated 
                          <0.00%                                  
^C
Aborted!

$> ls -l myother
total 4
drwx------ 2 yoh yoh 4096 Aug  5 16:30 000007/

yarikoptic · 2020-08-05T20:45:30Z

@mgrauer - this is not an acceptable value for the dandi prefix presently in identifiers.org, so we would either have to add that as an allowed regex or force people not to use identifiers.org for drafts. also it would be / not : for version information

dandi download DANDI:000007/0.200714.1807 downloads this specific published version
dandi download DANDI:000007/draft downloads the draft dandiset in its current form

I would also prefer here to use what is supported by/through identifiers.org.

Notes:

I would also to be able to specify a path (to a subject or a specific file)
There is a "convention" to use @version in a url to signal version.
- Used to be used by stackbrew to build official docker images, but I think they migrated away from that but some remain: e.g. https://github.com/docker-library/official-images/blob/4967c82e52fe02c24d5c585989fb24ac9eb25ba6/library/couchbase
- DataLad uses it within URLs for RIA remotes, BUT carefully within a #fragment since IIRC @ is not 100% kosher with allowed symbols in url ;) But since here it is not a URL, I think it could be Ok

So I wondered if it could be DANDI:<ID>[@<VERSION>][/PATH] or @ isn't good for identifiers?

Re "interactive" > $ dandi download DANDI:000007 from @satra -- "not sure". I expressed similar desires for more "interactive" within datalad, but somewhat agree (with Kyle) that for commands which by default are expected to be non-interactive, behavior should be explicitly described (e.g. download latest released version if no version specified), and only with explicit --interactive|-i they should offer choices. Similar to what git does. We aren't git here, but we do expect dandi to be used in CIs etc.

yarikoptic · 2020-08-05T20:50:01Z

also, we would never mint a doi for a dandiset which does not have a published version. i can also add a note to the redirector to specifically point to the draft URL for an unpublished dandiset.

I would prefer to avoid "if". That would avoid surprises for people who decide to use dandi datasets in their scripts in CI etc, so they could always know what they are downloading.

a. If we decide that it should be most recent published release, then without explicit version specification of "draft" - it should blow.

b. If we decide that it should not blow -- it should always redirect to a draft version. BUT I do not like this option since then we would need some additional latest to point to the latest release. Thus I would go with a..

…ANDI archive + publish development

…f tuple elements It became obvious after RFing to use a helper function

Common code from girder.py was moved into download.py etc. Found these changes uncomitted after a long period of time, committing before progressing forward with development

…e going through individual elements Probably overdone/ugly and should be easier/better with async. TODO REDO later

…ecksuming

…rs work See referenced in the diff issues about "modified" etc. Also I removed block of code to fetch metadata for an asset if missing - API now returns asset records with metadata

… consistently

yarikoptic · 2020-08-12T17:51:02Z

as no concerns were expressed upon requested "manual" testing, and we just keep breeding conflicts, I will resolve conflicts and if all good would merge.

* origin/master: (24 commits) Remove duplicate "requests" dependency get_instance(): Immediately return known instances that lack redirector URLs Raise helpful error if /server-info returns invalid version Make redirector report http://localhost:8081 as Girder URL Test uploading an unregistered dandiset Assert a certain code path is unreachable Test that lock_dandiset() is called when uploading Tests of lock_dandiset() Remove unused monkeypatches Configure flake8 to run on test code as well Detect nonexistent Dandisets when uploading ENH: upload - add locking of the dandiset Make pytest ignore DeprecationWarnings from responses Fetch all commits in test workflow Replace some occurrences of known_instances with get_instance() Test get_instance() CliVersionTooLowError → CliVersionTooOldError Move CLI version exceptions to dandi/exceptions.py Use semantic_version instead of packaging [GH-184] Add a get_instance() function ... Conflicts: dandi/download.py dandi/girder.py dandi/tests/test_download.py - conflicted with a linting change dandi/tests/test_utils.py - both added tests No conflicts but needed changes: dandi/utils.py -- needed to add dandiapi into returned dandi_instance

…"api" I left module and class still with dandiapi names

jwodder · 2020-08-12T18:39:14Z

dandi/cli/cmd_download.py

@@ -48,12 +55,16 @@
 )
 @click.argument("url", nargs=-1)
 @map_to_click_exceptions
-def download(url, output_dir, existing, jobs=6, develop_debug=False):
+def download(url, output_dir, existing, jobs=6, format="pytout"):


I don't think there's a situation where click would leave format unfilled, and if there is, shouldn't the default be "pyout", not "pytout"?

oh... so we can avoid duplicating default values within kwargs?! let met just strip then them away from download and we could do to the rest in a separate PR. THANK YOU! done in 4798e49

jwodder · 2020-08-12T18:42:14Z

dandi/consts.py

+# Chunk size when iterating a download (and upload) body. Taken from girder-cli
+# TODO: should we make them smaller for download than for upload?
+# ATM used only in download
+MAX_CHUNK_SIZE = os.environ.get("DANDI_MAX_CHUNK_SIZE", 1024 * 1024 * 8)  # 64


You need to cast this to an int, as the value from os.environ is a string.

d'oh, right,THANKS! done in 09b2196

jwodder · 2020-08-12T18:44:00Z

dandi/dandiapi.py

+lgr = get_logger()
+
+
+class HTTPError(requests.HTTPError):


Why not just import HTTPError instead of making a do-nothing subclass?

I wanted to aggregate non-standard exceptions our code might raise within this module. I guess I could have just gone with from requests import HTTPError into this namespace indeed but then linters would not be happy (not used etc), so I somewhat like this explicit subclassing. Any disadvantage?

There's no real disadvantage other than a tiny bit of overhead. I'd personally prefer to import HTTPError and add a noqa comment to tell flake8 to be quiet.

jwodder · 2020-08-12T18:45:16Z

dandi/dandiapi.py

+        """
+        self._session = session if session else requests.Session()
+
+        yield self._session


This should be:

try: yield self._session finally: self._session.close() self._session = None

so that the session gets closed on error.

indeed needs a fix, thanks! but it should close only if session was not provided (should not close outside session). Fixed in f367313

…ffect) It did not rebase easily into merge commit so I decided to keep it as is

jwodder · 2020-08-12T18:49:27Z

dandi/dandiapi.py

+        )
+
+        # If success, return the json object. Otherwise throw an exception.
+        if result.status_code not in (200, 201):


I suggest using "if not result.ok" instead, as that only regards 4xx and 5xx status codes as errors. There are other 2xx codes that aren't errors, plus the 3xx codes.

THANKS! done in a45b550

…download Which also removes a typoed default value - duplication is evil!

yarikoptic · 2020-08-12T19:25:07Z

Thank you @jwodder for the review and suggestions! I have now pushed all the changes I have referenced.

jwodder · 2020-08-12T18:55:42Z

dandi/dandiapi.py

+        return self.get(f"/dandisets/{dandiset_id}/versions/{version}/")
+
+    def get_dandiset_assets(
+        self, dandiset_id, version, location=None, page_size=None, include_metadata=True


include_metadata doesn't seem to be used, and there's no TODO item for it. Is that intentional?

I thought it would become "parametrized" for in the query for dandi/dandi-publish#78 resolution, but now it always includes it, so indeed there is no need for it. Removed in 515538e

jwodder · 2020-08-12T19:08:38Z

dandi/download.py

+    # dandi.cli.formatters are used in cmd_ls to provide switchable
+    pyout_style = pyouts.get_style(hide_if_missing=False)
+
+    rec_fields = ("path", "size", "done", "done%", "checksum", "status", "message")


If we're going to be passing tuples of columns to pyout, we should add != 0.6.0 to the dependency in setup.cfg in order to avoid the version that doesn't support them.

good idea! you did fix for 0.6.0 for upload in cc4d37c but there is still ls, and I just feel it would be just easier to ask to avoid 0.6.0. done in 096c8dd

jwodder · 2020-08-12T19:12:09Z

dandi/download.py

+                    # TODO: yield progress etc
+                    msg = {"done": downloaded}
+                    if size:
+                        if downloaded > size and not warned:


warned never gets set to True. Should that be added here?

yeah, set it to True in 5abd195 ... hopefully we never run into this part of code.

jwodder · 2020-08-12T19:28:13Z

dandi/support/iterators.py

+                yield queue.get(timeout=0.001)
+            except Empty:
+                continue
+            if self.reraise_immediately and self._exc is not None:


Wouldn't it be better to move this check to the top of the while loop in case get times out while an exception is raised?

indeed! done in e52d728

jwodder · 2020-08-12T19:29:43Z

dandi/dandiapi.py

+lgr = get_logger()
+
+
+class HTTPError(requests.HTTPError):


There's no real disadvantage other than a tiny bit of overhead. I'd personally prefer to import HTTPError and add a noqa comment to tell flake8 to be quiet.

Originally it was thought that may be resolution for dandi/dandi-publish#78 would make metadata return optional, but it is always returned, so no need for the option

… names

…e than expected

yarikoptic · 2020-08-12T19:51:07Z

There's no real disadvantage other than a tiny bit of overhead. I'd personally prefer to import HTTPError and add a noqa comment to tell flake8 to be quiet.

I have argued with having our exception in dandi.exceptions but even wasn't there... I removed this subclass for now completely in 20718e2

yarikoptic · 2020-08-12T20:08:51Z

woohoo -- still green, merging before it ripens.

mgrauer mentioned this pull request Jul 22, 2020

Call asset download endpoint from publish app #127

Closed

yarikoptic mentioned this pull request Jul 30, 2020

support dandi identifiers #89

Closed

yarikoptic mentioned this pull request Aug 3, 2020

download: "refresh" on dandiset.yaml should compare to existing file content and refresh #154

Open

yarikoptic mentioned this pull request Aug 3, 2020

Address flake8 warnings #147

Merged

yarikoptic added this to the 0.6.0: support of "publish API" milestone Aug 4, 2020

This was referenced Aug 4, 2020

Fix upload command #158

Closed

Add "publish" (AKA API) service to testing docker compose setup #164

Closed

BF(workaround): skip test if download blows due to tqdm locking issue #165

Merged

yarikoptic added 12 commits August 5, 2020 17:53

ENH: add API server url to the instance records

e06cbac

RF: move girder specifics around

9871ece

ENH: support parsing "dandiapi" style URLs from unversioned current D…

888b2e4

…ANDI archive + publish development

BF(TST): there was one not fixed earlier test with incorrect assert o…

d647032

…f tuple elements It became obvious after RFing to use a helper function

BF: add /api to the API endpoint

b1539aa

RF+ENH: introduce "dandiapi" type of server to download from

d10fa77

Common code from girder.py was moved into download.py etc. Found these changes uncomitted after a long period of time, committing before progressing forward with development

RF: WiP - I remember that download for girder worked

2c1cc94

NF: IteratorWithAggregation - to help receiving/displaying total whil…

764237b

…e going through individual elements Probably overdone/ugly and should be easier/better with async. TODO REDO later

NF: remap_dict helper to remap dicts from one "schema" to another

cb4c80f

ENH/RF: remap girder dandiset into API record, centralize download+ch…

55774be

…ecksuming

ENH: harmonize records from girder to API (+modified), both downloade…

188530c

…rs work See referenced in the diff issues about "modified" etc. Also I removed block of code to fetch metadata for an asset if missing - API now returns asset records with metadata

ENH(TST): a test on 000027 to ensure that both types of download work…

e2bbac1

… consistently

yarikoptic mentioned this pull request Aug 10, 2020

download: DANDI: "targets" to support #183

Closed

RF: download - encapsulating some code + moving around + TODOs

a941485

jwodder mentioned this pull request Aug 12, 2020

Add "resume" functionality to download #189

Closed

yarikoptic requested a review from jwodder August 12, 2020 17:50

yarikoptic added 3 commits August 12, 2020 14:03

BF: making linting happy

aed98c5

RF: "dandiapi" -> "api" and "DANDI API" to match /server-info simple …

327881e

…"api" I left module and class still with dandiapi names

jwodder reviewed Aug 12, 2020

View reviewed changes

BF: moved lock_dandiset into correct position (incorrect merge side e…

a4f2a3d

…ffect) It did not rebase easily into merge commit so I decided to keep it as is

jwodder reviewed Aug 12, 2020

View reviewed changes

yarikoptic added 5 commits August 12, 2020 14:50

BF(TST): yet one more test fix up for incorrect handling while merging

64d9952

RF: do not duplicate defaults within kwargs in @click entrypoint for …

4798e49

…download Which also removes a typoed default value - duplication is evil!

BF: cast env var DANDI_MAX_CHUNK_SIZE to int

09b2196

BF: close the session if we started a new one

f367313

ENH: use result.ok instead of checking only for 200 and 201

a45b550

jwodder reviewed Aug 12, 2020

View reviewed changes

yarikoptic added 5 commits August 12, 2020 15:37

RF: dandiapi - remove not used include_metadata option

515538e

Originally it was thought that may be resolution for dandi/dandi-publish#78 would make metadata return optional, but it is always returned, so no need for the option

BF: ask for pyout != 0.6.0 so we could keep passing tuples with field…

096c8dd

… names

ENH(UX): set warn to True upon issueing first warning that we got mor…

5abd195

…e than expected

RF: quit loop even before the first .get if exception was raised

e52d728

RF: completely remove custom subclass HTTPError for now

20718e2

yarikoptic merged commit 3db0424 into master Aug 12, 2020

yarikoptic deleted the enh-download-api branch August 12, 2020 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RF/ENH: download - reimplementation to support original and upcoming UI/API #134

RF/ENH: download - reimplementation to support original and upcoming UI/API #134

yarikoptic commented Jul 22, 2020 •

edited

Loading

codecov bot commented Jul 22, 2020 •

edited

Loading

yarikoptic commented Aug 1, 2020

yarikoptic commented Aug 1, 2020

mgrauer commented Aug 3, 2020

satra commented Aug 3, 2020

satra commented Aug 3, 2020

yarikoptic commented Aug 3, 2020

yarikoptic commented Aug 5, 2020

yarikoptic commented Aug 5, 2020

yarikoptic commented Aug 5, 2020

yarikoptic commented Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

yarikoptic commented Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic Aug 12, 2020

jwodder Aug 12, 2020

yarikoptic commented Aug 12, 2020

yarikoptic commented Aug 12, 2020

RF/ENH: download - reimplementation to support original and upcoming UI/API #134

RF/ENH: download - reimplementation to support original and upcoming UI/API #134

Conversation

yarikoptic commented Jul 22, 2020 • edited Loading

codecov bot commented Jul 22, 2020 • edited Loading

Codecov Report

yarikoptic commented Aug 1, 2020

yarikoptic commented Aug 1, 2020

mgrauer commented Aug 3, 2020

satra commented Aug 3, 2020

satra commented Aug 3, 2020

yarikoptic commented Aug 3, 2020

yarikoptic commented Aug 5, 2020

yarikoptic commented Aug 5, 2020

yarikoptic commented Aug 5, 2020

yarikoptic commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Aug 12, 2020

yarikoptic commented Aug 12, 2020

yarikoptic commented Jul 22, 2020 •

edited

Loading

codecov bot commented Jul 22, 2020 •

edited

Loading