Models: define id, add various additional types (genotype, etc), boost model version to 0.3.0 #560

satra · 2021-04-13T03:10:47Z

id + identifier
genotype info
~~- [ ] still decide on participant.~~

* upstream/master: (22 commits) Strip trailing slash from API URL used by delete() Refresh dandiset.yaml on download if out of date Support "…/assets/?path=<path>" URLs Get hdmf, pynwb, h5py versions without importing Update CHANGELOG.md [skip ci] DOC: minor tune up to readme just to cut a release adding for Unknown in extract_sex Put package versions on one line and use get_module_version() Log dandi, hdmf, h5py, and pynwb versions to log file Log errors in extracting metadata for upload BF: proper import of importlib_metadata backport BF: remove unused pkg_resources import and correct docstring BF+ENH: fixup typing and use importlib ENH: use get_module_version for cache tokens ENH: get_module_version helper Update CHANGELOG.md [skip ci] BF: please linters ENH: make it work with current and perspective version of dandi-api Update dandiarchive client to use most_recent_published_version Give `ls` a `--metadata` option ...

codecov · 2021-04-13T03:59:06Z

Codecov Report

Merging #560 (ebe267f) into master (6a5f285) will increase coverage by 0.07%.
The diff coverage is 98.21%.

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage   83.70%   83.78%   +0.07%     
==========================================
  Files          62       62              
  Lines        6530     6567      +37     
==========================================
+ Hits         5466     5502      +36     
- Misses       1064     1065       +1

Flag	Coverage Δ
unittests	`83.78% <98.21%> (+0.07%)`	⬆️

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

Impacted Files	Coverage Δ
dandi/model_types.py	`100.00% <ø> (ø)`
dandi/tests/test_metadata.py	`100.00% <ø> (ø)`
dandi/tests/test_models.py	`100.00% <ø> (ø)`
dandi/metadata.py	`79.08% <75.00%> (-0.10%)`	⬇️
dandi/consts.py	`100.00% <100.00%> (ø)`
dandi/models.py	`87.81% <100.00%> (+1.19%)`	⬆️

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 6a5f285...ebe267f. Read the comment docs.

dandi/tests/test_metadata.py

* upstream/master: Update CHANGELOG.md [skip ci] BF: do not assume that service record of redirector is present/has url Fix a typo in the display string for one of the known URL patterns Error with a decent message when trying to delete() a path not in a Dandiset Fix & test for downloading by asset ID URL

* upstream/master: Add further tests of get_instance() and server-info

satra · 2021-04-14T20:55:07Z

@yarikoptic - i would love to get this merged if there are no objections at this point.

this would then trigger a schema update for api and web-ui which should help as well. nothing should technically be different at this point with id, since it is an optional field.

yarikoptic

I am still a bit unsure about disappearance of identifier from test records. Left various questions/recommendations throughout.

yarikoptic · 2021-04-15T15:31:06Z

dandi/metadata.py

+    if "version" in newmeta:
+        newmeta["id"] = f"{newmeta['identifier']}/{newmeta['version']}"
+    else:
+        newmeta["id"] = f"{newmeta['identifier']}/draft"


identifier is made optional in Identifiable (and was/is optional for others such as RelatedParticipant) in this PR, so I guess it could fail or get a bogus None here? may be no id should be assigned if not identifier?

this is a converter and this function will go away as soon as we migrate.

that is good, still don't see why it should be "not robust by design" meanwhile, but ok. I will assign you an issue if I run into one ;-)

dandi/metadata.py

dandi/models.py

yarikoptic · 2021-04-15T15:56:57Z

dandi/tests/data/metadata/metadata2asset.json

@@ -1,5 +1,5 @@
 {
-  "identifier": "0b0a1a0b-e3ea-4cf6-be94-e02c830d54be",
+  "id": "dandiasset:0b0a1a0b-e3ea-4cf6-be94-e02c830d54be",


hm, I thought that with the recent changes we still retain identifier and only provide id at the level of dandi-api queries (and may be only for json-ld serialization), thus minting it in addition/based on the identifier. but even if "not only for json-ld" -- shouldn't original identifier also appear here?

again - one of my arguments was/is - with just an id users would need to "manually" split it inside of just using the value as asset_id for the API queries. So by catering to the LD we are further sacrificing ease of use of this metadata for "more pragmatic" interactions ;-)

i don't think this affects anything in the CLI now, because this is about conversion and the CLI explicitly ignores identifier.

rright -- I think we are relying on API to provide us asset_ids in its listing of assets for a path, so indeed should not get broken. But my comment was not about breakage but rather about changing metadata so there is no 1-to-1 immediate correspondence between those records and API (asset_id).

dandi/tests/test_models.py

Co-authored-by: Yaroslav Halchenko <[email protected]>

yarikoptic · 2021-04-15T20:49:14Z

anyways -- I do not want to hold the progress here, since I might still be just missing on some aspects of identifier-vs-id situation. Time will show. Let's proceed and brace for an impact -- I will also release fresh dandi-cli with this as we just merged another PR which might be good to have for future proofing.

satra added 5 commits April 9, 2021 16:15

remove person from participant

3de434b

wip: current state

75da30b

update licenses to use spdx pointers

23c0eae

a few more fixes

a64a50c

satra added 3 commits April 13, 2021 13:19

fix: some tests

c114736

make linter happy

d923f34

fix: type info for licenses

f0d769b

satra marked this pull request as ready for review April 13, 2021 17:40

satra changed the title ~~WIP: updating models~~ Updating models Apr 13, 2021

yarikoptic reviewed Apr 13, 2021

View reviewed changes

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

satra added 5 commits April 14, 2021 10:40

update asset prefix and id for dandiset

495aa1e

fix: limit uri lengths to 1000 characters for the json schema

d07623d

Merge remote-tracking branch 'upstream/master' into enh/models

4cf208b

* upstream/master: Add further tests of get_instance() and server-info

make id optional till publish

75b3ee7

yarikoptic requested changes Apr 15, 2021

View reviewed changes

satra and others added 3 commits April 15, 2021 12:52

change licenses from dandi to spdx

0f22e72

Apply suggestions from code review

f7725c9

Co-authored-by: Yaroslav Halchenko <[email protected]>

enh: refactor publishable

ebe267f

yarikoptic added minor Increment the minor version when merged release Create a release when this pr is merged schema Issues relating to metadata schema labels Apr 15, 2021

yarikoptic changed the title ~~Updating models~~ Models: define id, add various additional types (genotype, etc), boost model version to 0.3.0 Apr 15, 2021

yarikoptic merged commit 8ee4863 into dandi:master Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Models: define id, add various additional types (genotype, etc), boost model version to 0.3.0 #560

Models: define id, add various additional types (genotype, etc), boost model version to 0.3.0 #560

satra commented Apr 13, 2021 •

edited

Loading

codecov bot commented Apr 13, 2021 •

edited

Loading

satra commented Apr 14, 2021

yarikoptic left a comment

yarikoptic Apr 15, 2021

satra Apr 15, 2021

yarikoptic Apr 15, 2021

yarikoptic Apr 15, 2021

satra Apr 15, 2021

yarikoptic Apr 15, 2021

yarikoptic commented Apr 15, 2021

Models: define id, add various additional types (genotype, etc), boost model version to 0.3.0 #560

Models: define id, add various additional types (genotype, etc), boost model version to 0.3.0 #560

Conversation

satra commented Apr 13, 2021 • edited Loading

codecov bot commented Apr 13, 2021 • edited Loading

Codecov Report

satra commented Apr 14, 2021

yarikoptic left a comment

Choose a reason for hiding this comment

yarikoptic Apr 15, 2021

Choose a reason for hiding this comment

satra Apr 15, 2021

Choose a reason for hiding this comment

yarikoptic Apr 15, 2021

Choose a reason for hiding this comment

yarikoptic Apr 15, 2021

Choose a reason for hiding this comment

satra Apr 15, 2021

Choose a reason for hiding this comment

yarikoptic Apr 15, 2021

Choose a reason for hiding this comment

yarikoptic commented Apr 15, 2021

satra commented Apr 13, 2021 •

edited

Loading

codecov bot commented Apr 13, 2021 •

edited

Loading