Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Girder support #588

Merged
merged 12 commits into from
Apr 28, 2021
Merged

Remove Girder support #588

merged 12 commits into from
Apr 28, 2021

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Apr 21, 2021

Closes #587.

TODOs

  • Needs tune up of ./dandi/tests/data/dandiarchive-docker/docker-compose.yml to strip docker away - must not be needed.

@jwodder jwodder added the minor Increment the minor version when merged label Apr 21, 2021
@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #588 (87fcde4) into master (242c5af) will increase coverage by 1.62%.
The diff coverage is 95.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #588      +/-   ##
==========================================
+ Coverage   81.26%   82.88%   +1.62%     
==========================================
  Files          62       59       -3     
  Lines        6651     5710     -941     
==========================================
- Hits         5405     4733     -672     
+ Misses       1246      977     -269     
Flag Coverage Δ
unittests 82.88% <95.80%> (+1.62%) ⬆️

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

Impacted Files Coverage Δ
dandi/cli/cmd_ls.py 57.55% <0.00%> (-0.25%) ⬇️
dandi/cli/cmd_upload.py 55.88% <ø> (-3.58%) ⬇️
dandi/delete.py 89.61% <ø> (+0.50%) ⬆️
dandi/tests/test_dandiarchive.py 100.00% <ø> (ø)
dandi/tests/test_delete.py 100.00% <ø> (ø)
dandi/tests/test_download.py 100.00% <ø> (ø)
dandi/dandiarchive.py 80.00% <85.71%> (+5.26%) ⬆️
dandi/download.py 85.80% <95.00%> (+0.98%) ⬆️
dandi/cli/base.py 82.60% <100.00%> (+5.25%) ⬆️
dandi/cli/command.py 45.45% <100.00%> (-0.70%) ⬇️
... and 15 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 242c5af...87fcde4. Read the comment docs.

@yarikoptic
Copy link
Member

pushed a CPed fix from #591 ... will probably proceed with that PR for now for a minimal fix to get client working for migration; added a TODO in description on docker compose recipe

@satra
Copy link
Member

satra commented Apr 23, 2021

@jwodder - as part of this we can also remove migrating to new schema perhaps for the dandiset metadata. the asset related stuff can stay.

@jwodder
Copy link
Member Author

jwodder commented Apr 26, 2021

@satra File that as a separate issue. It'll require changes to validate.py and dandiset.py that go beyond just deleting code.

@jwodder
Copy link
Member Author

jwodder commented Apr 27, 2021

@yarikoptic While we wait for dandi/dandi-archive#251 to be merged, I'd appreciate if it you'd take a look at what's left of dandi/dandiarchive.py and tell me if anything else should be removed.

@yarikoptic
Copy link
Member

While we wait for dandi/dandi-api#251 to be merged,

Why do we need to wait on it - even if docker compose ships girder, we can just not use it (i.e. nothing in dandi-cli somehow senses for girder to be present etc), can't we?

@yarikoptic
Copy link
Member

I'd appreciate if it you'd take a look at what's left of dandi/dandiarchive.py and tell me if anything else should be removed.

git grep -i girder | grep -v -e CHANGELOG -e README -e docker-compose -e '^tools/'
.github/workflows/test.yml:      run: echo DANDI_LOG_GIRDER=1 >> "$GITHUB_ENV"
DEVELOPMENT.md:dandiarchive (both with our web frontend, and girder backend).
DEVELOPMENT.md:as `local-docker` (as opposed from `local` for a plain girder instance).  See note
DEVELOPMENT.md:  such as explicit specification of the girder instance, collection, etc.
DEVELOPMENT.md:- `DANDI_GIRDER_API_KEY` -- avoids using keyrings, thus making it possible to
DEVELOPMENT.md:  "temporarily" use another account etc for the Girder version of the server.
DEVELOPMENT.md:- `DANDI_LOG_GIRDER` -- log REST requests.
dandi/cli/__init__.py:  e.g. girder-client
dandi/consts.py:# Chunk size when iterating a download (and upload) body. Taken from girder-cli
dandi/core/digests/dandietag.py:# Derived from <https://github.com/girder/django-s3-file-field/blob/master/
dandi/dandiapi.py:# Following class is loosely based on GirderClient, with authentication etc
dandi/dandiapi.py:        girder and DANDI api clients.
dandi/dandiarchive.py:Interactions with DANDI archive ATM go either through Girder or through DANDI API.
dandi/dandiarchive.py:Eventually it is largely to be "dissolved" whenever we stop talking to girder.
dandi/dandiarchive.py:          old (girder inflicted): https://gui.dandiarchive.org/#/folder/5e5593cc1a343161ff7c5a92
dandi/dandiarchive.py:          old (girder inflicted): https://gui.dandiarchive.org/#/folder/5dab0830f377535c7d96c2b4
dandi/dandiarchive.py:          old (girder inflicted): https://gui.dandiarchive.org/#/dandiset/5e6d5c6976569eb93f451e4f
dandi/dandiarchive.py:          gui.dandiarchive.org ones into girder.
dandi/dandiset.py:            # girder-based, used before migration to API  TODO: API-migration-remove
dandi/download.py:            # by server while establishing downloader... but it seems that girder itself
dandi/download.py:      A backend (girder or api) specific fixture for downloading some file into
dandi/download.py:            # both girder and we use HttpError
dandi/download.py:    # It seems that girder might not care about setting either mtime, so we will do if we know
dandi/metadata.py:The following section converts metadata schema from the current girder dandiset
dandi/tests/fixtures.py:    GIRDER_URL = "http://localhost:8081"
dandi/tests/fixtures.py:            f"{GIRDER_URL}/api/v1/user/authentication", auth=("admin", "letmein")
dandi/tests/fixtures.py:            f"{GIRDER_URL}/api/v1/api_key",
dandi/tests/fixtures.py:            headers={"Girder-Token": initial_api_key},
dandi/tests/fixtures.py:                f"{GIRDER_URL}/api/v1/user",
dandi/tests/fixtures.py:                headers={"Girder-Token": initial_api_key},
dandi/tests/fixtures.py:            f"{GIRDER_URL}/api/v1/user/authentication", auth=("publish", "Z1lT4Fh7Kj")
dandi/tests/fixtures.py:        # We can't refer to the Girder container as "girder" in
dandi/tests/fixtures.py:        # DJANGO_DANDI_GIRDER_API_URL because Django doesn't recognize
dandi/tests/fixtures.py:        # use the Girder container's in-network IP address.
dandi/tests/fixtures.py:        about_girder = check_output(
dandi/tests/fixtures.py:            ["docker", "inspect", f"{LOCAL_DOCKER_ENV}_girder_1"]
dandi/tests/fixtures.py:        girder_ip = json.loads(about_girder)[0]["NetworkSettings"]["Networks"][
dandi/tests/fixtures.py:        env["DJANGO_DANDI_GIRDER_API_URL"] = f"http://{girder_ip}:8080/api/v1"
dandi/tests/fixtures.py:        env["DJANGO_DANDI_GIRDER_API_KEY"] = publish_api_key
dandi/tests/fixtures.py:                f"{GIRDER_URL}/api/v1/system/setting",
dandi/tests/fixtures.py:                headers={"Girder-Token": publish_api_key},
dandi/tests/fixtures.py:                f"{GIRDER_URL}/api/v1/system/setting",
dandi/tests/fixtures.py:                headers={"Girder-Token": publish_api_key},
dandi/tests/fixtures.py:        yield {"girder_api_key": api_key, "django_api_key": django_api_key}
dandi/tests/test_dandiarchive.py:        # ATM we point to drafts, so girder
dandi/tests/test_dandiarchive.py:        # # And the hybrid for "drafts" where it still goes by girder ID
dandi/tests/test_dandiarchive.py:                "girder": None,
dandi/tests/test_utils.py:                "girder": None,
dandi/tests/test_utils.py:                "girder": {"url": "https://girder.dandi"},
dandi/tests/test_utils.py:                "girder": {"url": "https://girder.dandi"},
dandi/tests/test_utils.py:                "girder": {"url": "https://girder.dandi"},
dandi/upload.py:    # this is a path not a girder id
dandi/utils.py:        )  # note: somehow was ending up with {"girder": None}
doc/demos/basic-workflow1.sh:# top url, would need to know girder id
setup.cfg:    girder-client
hints on a few other pieces which we could strip away, and seems largely just in the docstrings (within dandiarchive.py as well).

@jwodder
Copy link
Member Author

jwodder commented Apr 27, 2021

@yarikoptic I meant, could you please specifically look through the dandiarchive.py file on this branch? I'm not sure what parts of that file (particularly the URL-parsing code) should be kept and which should be deleted.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left minor recommendations.
We still have good parts of code not test covered in that file, but nothing stroke me as "could be gone" ATM in dandiarchive.py

dandi/dandiarchive.py Outdated Show resolved Hide resolved
dandi/dandiarchive.py Outdated Show resolved Hide resolved
dandi/dandiarchive.py Show resolved Hide resolved
@yarikoptic
Copy link
Member

did another skim through diff (still see no coverage report overlay for some reason, but probably unrelated). Let's proceed -- thank you @jwodder ! and if no major issues we run into, let's release tomorrow or so.

@yarikoptic yarikoptic merged commit cf8e0ea into master Apr 28, 2021
@yarikoptic yarikoptic deleted the gh-587 branch April 28, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RM girder support code/logic/instances
3 participants