Add a get_instance() function #186

jwodder · 2020-08-11T12:59:00Z

Closes #184

codecov · 2020-08-11T13:00:47Z

Codecov Report

Merging #186 into master will increase coverage by 1.69%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   76.56%   78.26%   +1.69%     
==========================================
  Files          41       41              
  Lines        3124     3322     +198     
==========================================
+ Hits         2392     2600     +208     
+ Misses        732      722      -10

Flag	Coverage Δ
#unittests	`78.26% <100.00%> (+1.69%)`	⬆️

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

Impacted Files	Coverage Δ
dandi/download.py	`72.72% <100.00%> (ø)`
dandi/exceptions.py	`100.00% <100.00%> (ø)`
dandi/register.py	`70.37% <100.00%> (+1.13%)`	⬆️
dandi/tests/test_utils.py	`100.00% <100.00%> (ø)`
dandi/upload.py	`64.51% <100.00%> (ø)`
dandi/utils.py	`73.44% <100.00%> (+4.81%)`	⬆️
dandi/tests/test_download.py	`100.00% <0.00%> (ø)`
dandi/support/tests/test_digests.py	`100.00% <0.00%> (ø)`
dandi/tests/fixtures.py	`99.05% <0.00%> (+1.68%)`	⬆️
... and 1 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 92474c7...cf73932. Read the comment docs.

yarikoptic

Let's just use semantic_version which we already have in dependencies.

Now that /server-info is provided, PR should include changes to make use of get_instance + add a few dedicated tests to validate correct handling of scenarios code has envisioned to handle.

dandi/utils.py

yarikoptic · 2020-08-11T16:36:07Z

dandi/utils.py

+    )
+
+
+class CliVersionError(Exception):


Let's centralize exception definitions in dandi.exceptions. I would probably subclass from RuntimeError instead of a generic Exception

probably worth adding a docstring (or make __str__ explicitly an @abc.abstractmethod) to note that this is just a base exception, without __str__ and thus derived classes should be used

dandi/utils.py

jwodder · 2020-08-11T17:33:54Z

@yarikoptic Some of the tests I wrote for get_instance() use dandi.__version__, but since the GitHub Actions workflow only checks out the most recent commit, we end up with a version string like 0+untagged.1.g08f7fb1, which semantic_version errors on. Thoughts on how to address this?

yarikoptic · 2020-08-11T17:56:03Z

Thoughts on how to address this?

do a full checkout?

jwodder · 2020-08-11T18:13:14Z

That works, though I was hoping for something involving a smaller checkout (not that this repo is all that large in the first place, though).

New problem: The redirector uses the GIRDER_URL environment variable to connect to Girder, yet it also reports the same URL in the /server-info response. When the redirector is running inside a Docker container, this causes problems[1], as Girder's internal-to-the-Docker-network URL is not the same as its external URL (and the external URL is on localhost, so it can't be used internally by the redirector container). Should we add another environment variable to the redirector script for specifying the internal/external Girder URL?

[1] Except when I ran the tests on my computer, for some reason.

setup.cfg

yarikoptic · 2020-08-11T19:20:54Z

TL;DR: most likely indeed it would be best to just go with additional environment variable/configuration (e.g. GIRDER_URL_INTERNAL defaulting to the value of GIRDER_URL) for redirector, even though I do not quite like that, but at least it would be the most lightweight solution ATM.

Some details: rright... just some additional digesting for myself: redirector itself needs to be able to correspond to girder instance. For that reason currently GIRDER_URL: http://girder:8080 is set in dandi/tests/data/dandiarchive-docker/docker-compose.yml, but for outside client we would need to provide something like http://localhost:8079 . So I thought may be redirector could just contact http://localhost:8079 but it would then need to be not a localhost within docker but localhost on the host or the external IP.

On https://stackoverflow.com/a/43541681 found out about magical host.docker.internal but it is not there for linux docker/for-linux#264 . But even if it was supported - it would have probably interferred with possible firewall etc.

Also the same SO page talks about linux only "Use --network="host" in your docker run command, then 127.0.0.1 in your docker container will point to your docker host." but tells that it was removed and did not come back.

The same SO page references https://github.com/qoomon/docker-host as a solution to establish NAT container as part of the setup... IMHO too heavy weight and seems might still need to require docker tune up to get it working.

yarikoptic · 2020-08-11T19:24:37Z

re [1] -- from digesting SO it seems that there is a good amount of differences between how docker behaves across OSes at that level of network routing etc, and IIRC you are using OS X now.

jwodder · 2020-08-11T19:30:25Z

Yes, I am using macOS, though if the tests can resolve the girder hostname while running on my computer, I'd expect host, ping, nc, or curl to be able to resolve the hostname as well while the containers are up, yet they can't. Odd. (Maybe it's a process session thing?)

jwodder · 2020-08-11T19:31:05Z

I created a PR to add the environment variable to the redirector: dandi/redirector#25. Once that's merged, the docker-compose.yml can be updated to specify both variables, and then the tests should pass on GitHub Actions again.

yarikoptic · 2020-08-11T19:35:07Z

hm, if I add network_mode: host to the redirector's part in the compose

$> git diff
diff --git a/dandi/tests/data/dandiarchive-docker/docker-compose.yml b/dandi/tests/data/dandiarchive-docker/docker-compose.yml
index ef0a109..0b9ad2e 100644
--- a/dandi/tests/data/dandiarchive-docker/docker-compose.yml
+++ b/dandi/tests/data/dandiarchive-docker/docker-compose.yml
@@ -41,6 +41,7 @@ services:
       - girder
     ports:
       - "8079:8080"
+    network_mode: host
     environment:
       GIRDER_URL: http://girder:8080
       GUI_URL: http://localhost:8086

with

$> docker --version
Docker version 19.03.7, build 7141c19
$> docker-compose --version
docker-compose version 1.25.0, build unknown

then it seems to be able to access curl http://localhost:8081 from within the container, thus reaching the girder... and looking at ip addr withing redirector container -- indeed the full set of host interfaces is there. So we just need to be careful to not choose some if it also works on OSX -- might be a solution without tuning the redirector for that, but might need a tune up to be able to run it on a different than 8080 port?

jwodder · 2020-08-11T19:52:07Z

Setting network_mode: host and updating GIRDER_URL to http://localhost:8081 appears to work on my computer, though now I can't figure out why the tests can apparently access the redirector yet at the same time in another window curl -fsSL localhost:8079/server-info (and also with port 8080) gives me "connection refused".

yarikoptic · 2020-08-11T19:56:02Z

if I parsed the sentence correctly, 8079: mapping is no longer in effect with "host" (I think), so I cannot access it either from the host:

$> curl -fsSL localhost:8079/server-info
curl: (7) Failed to connect to localhost port 8079: Connection refused

$> curl -fsSL localhost:8080/server-info
{"version":"1.0.0","cli-minimal-version":"0.5.0","cli-bad-versions":[],"services":{"girder":{"url":"http:\/\/girder:8080"},"webui":{"url":"http:\/\/localhost:8086"},"api":{"url":"https:\/\/publish.dandiarchive.org\/api"},"jupyterhub":{"url":"https:\/\/hub.dandiarchive.org"}}}%

jwodder · 2020-08-11T20:02:48Z

Neither port 8079 nor port 8080 works for me now, and I've just realized that the tests might be failing to connect to the redirector as well, in which case they're falling back to the hard-coded URLs and emitting a (captured & suppressed) warning.

satra · 2020-08-11T21:25:44Z

just some notes from local testing on macos.

i made the following changes to the docker-compose file, which allows me to access the relevant services via localhost, since they are exposed from the docker network.

    environment:
      GIRDER_URL: http://localhost:8081
      GUI_URL: http://localhost:8086
      ABOUT_URL: http://www.dandiarchive.org

the GUI_URL doesn't seem to work for me (i.e. it doesn't show up), but girder is accessible and the redirector is accessible.

yarikoptic · 2020-08-12T13:15:40Z

GIRDER_URL: http://localhost:8081

This would work for dandi-cli testing if we only talk to /server-info endpoint and not a path in redirector which requires its communication with girder. So, to be able to test (in the future) any other /path of redirector we would need smth like dandi/redirector#25 . But for the purpose of this PR, we don't need any other path, so could indeed use this value, right @jwodder ?

the GUI_URL doesn't seem to work for me

yeap, as we discovered in troubleshooting session with @satra yesterday, reason is: dandi/dandiarchive-legacy#474 requiring a fix in the container first. dandi-cli anyways is not concerned with web ui client ATM anyhow anyways.

jwodder · 2020-08-12T13:25:05Z

@yarikoptic Yes, I believe that would work for now.

yarikoptic

Thanks for addressing all previous comments! We are converging, I just left two other requests.

Cheers!

dandi/utils.py

dandi/tests/test_utils.py

jwodder · 2020-08-12T13:37:47Z

The tests pass with GIRDER_URL set to http://localhost:8081.

…or URLs

yarikoptic

Left a comment but let's just proceed for now! Thank you @jwodder !

yarikoptic · 2020-08-12T15:22:51Z

dandi/utils.py

+    try:
+        minversion = Version(server_info["cli-minimal-version"])
+        bad_versions = [Version(v) for v in server_info["cli-bad-versions"]]
+    except ValueError as e:


thinking out loud: sorry for being slow, it got me thinking (since we do not test for version yet etc), what if any of those two is missing? and it kinda makes sense if server does not demand any specific version of the client somehow... I think later some time we might want to allow them to be "optional" but for now we would demand them, and if not present -- blow informatively... or let's just add that later ;)

satra · 2020-08-12T15:30:52Z

dandi-cli anyways is not concerned with web ui client ATM anyhow anyways.

don't you have a download test for subject folders based on the web ui link? but it's more about processing the link than for the ui to actually work :)

yarikoptic · 2020-08-12T16:05:23Z

Yes, but in those we go to central deployment ATM. Indeed we would need it later for self contained tests

[GH-184] Add a get_instance() function

20c8650

yarikoptic requested changes Aug 11, 2020

View reviewed changes

yarikoptic mentioned this pull request Aug 11, 2020

ENH(TEMP): disallow use of a known to be outdated version of dandi client #135

Closed

jwodder added 4 commits August 11, 2020 12:47

Use semantic_version instead of packaging

2aa0aea

Move CLI version exceptions to dandi/exceptions.py

431bb0a

CliVersionTooLowError → CliVersionTooOldError

d226fe8

Test get_instance()

08f7fb1

Replace some occurrences of known_instances with get_instance()

7efdbd4

Fetch all commits in test workflow

2889208

yarikoptic reviewed Aug 11, 2020

View reviewed changes

setup.cfg Show resolved Hide resolved

Make pytest ignore DeprecationWarnings from responses

6874325

jwodder mentioned this pull request Aug 11, 2020

Add GIRDER_LOCAL_URL environment variable dandi/redirector#25

Merged

yarikoptic requested changes Aug 12, 2020

View reviewed changes

dandi/utils.py Outdated Show resolved Hide resolved

dandi/tests/test_utils.py Outdated Show resolved Hide resolved

Make redirector report http://localhost:8081 as Girder URL

1cf0d70

Raise helpful error if /server-info returns invalid version

389a19a

jwodder force-pushed the gh-184 branch from b73ef6a to 389a19a Compare August 12, 2020 13:46

get_instance(): Immediately return known instances that lack redirect…

cf73932

…or URLs

yarikoptic approved these changes Aug 12, 2020

View reviewed changes

yarikoptic merged commit 3137916 into master Aug 12, 2020

yarikoptic deleted the gh-184 branch August 12, 2020 15:24

yarikoptic mentioned this pull request Aug 12, 2020

ENH: upload - add locking of the dandiset #130

Merged

jwodder mentioned this pull request Aug 20, 2020

Add "resume" functionality to download #189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a get_instance() function #186

Add a get_instance() function #186

jwodder commented Aug 11, 2020

codecov bot commented Aug 11, 2020 •

edited

Loading

yarikoptic left a comment

yarikoptic Aug 11, 2020

yarikoptic Aug 11, 2020

jwodder Aug 11, 2020

jwodder commented Aug 11, 2020

yarikoptic commented Aug 11, 2020

jwodder commented Aug 11, 2020

yarikoptic commented Aug 11, 2020

yarikoptic commented Aug 11, 2020

jwodder commented Aug 11, 2020

jwodder commented Aug 11, 2020 •

edited

Loading

yarikoptic commented Aug 11, 2020

jwodder commented Aug 11, 2020

yarikoptic commented Aug 11, 2020 •

edited

Loading

jwodder commented Aug 11, 2020

satra commented Aug 11, 2020

yarikoptic commented Aug 12, 2020

jwodder commented Aug 12, 2020

yarikoptic left a comment

jwodder commented Aug 12, 2020

yarikoptic left a comment

yarikoptic Aug 12, 2020

satra commented Aug 12, 2020

yarikoptic commented Aug 12, 2020

		)


		class CliVersionError(Exception):

Add a get_instance() function #186

Add a get_instance() function #186

Conversation

jwodder commented Aug 11, 2020

codecov bot commented Aug 11, 2020 • edited Loading

Codecov Report

yarikoptic left a comment

Choose a reason for hiding this comment

yarikoptic Aug 11, 2020

Choose a reason for hiding this comment

yarikoptic Aug 11, 2020

Choose a reason for hiding this comment

jwodder Aug 11, 2020

Choose a reason for hiding this comment

jwodder commented Aug 11, 2020

yarikoptic commented Aug 11, 2020

jwodder commented Aug 11, 2020

yarikoptic commented Aug 11, 2020

yarikoptic commented Aug 11, 2020

jwodder commented Aug 11, 2020

jwodder commented Aug 11, 2020 • edited Loading

yarikoptic commented Aug 11, 2020

jwodder commented Aug 11, 2020

yarikoptic commented Aug 11, 2020 • edited Loading

jwodder commented Aug 11, 2020

satra commented Aug 11, 2020

yarikoptic commented Aug 12, 2020

jwodder commented Aug 12, 2020

yarikoptic left a comment

Choose a reason for hiding this comment

jwodder commented Aug 12, 2020

yarikoptic left a comment

Choose a reason for hiding this comment

yarikoptic Aug 12, 2020

Choose a reason for hiding this comment

satra commented Aug 12, 2020

yarikoptic commented Aug 12, 2020

codecov bot commented Aug 11, 2020 •

edited

Loading

jwodder commented Aug 11, 2020 •

edited

Loading

yarikoptic commented Aug 11, 2020 •

edited

Loading