Repository navigation
Add per-asset dataStandard, HED standard, and extensions to StandardsType - #371
yarikoptic wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #371 +/- ##
==========================================
+ Coverage 47.96% 48.15% +0.19%
==========================================
Files 19 19
Lines 2427 2515 +88
==========================================
+ Hits 1164 1211 +47
- Misses 1263 1304 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| dataStandard: Optional[List[StandardsType]] = Field( | ||
| None, | ||
| description="Data standard(s) applicable to this asset.", | ||
| json_schema_extra={"readOnly": True}, |
There was a problem hiding this comment.
readOnly should set to true only in the case that the dataStandard field is assigned and managed solely by the server per https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-validation-00#rfc.section.9.4. Is it the case that the dataStandard field is intended to be assigned and managed solely by the server?
There was a problem hiding this comment.
I would guess the answer is no -- that it is intended to be extracted from the source files by the DANDI CLI running client-side.
There was a problem hiding this comment.
INTERESTING! We also have readOnly on above "TODO" attributes which I guess nobody ever assigned to any asset. Although we are indeed to populate it from client (or on server) but since we have other similar wasGeneratedBy also editable, I see no point in making this readOnly, and I guess it is really our own thing, hence
| json_schema_extra={"readOnly": True}, | |
| json_schema_extra={"nskey": DANDI_NSKEY}, |
right?
There was a problem hiding this comment.
Good point — this is populated client-side by dandi-cli, not server-managed, so readOnly is incorrect. Replaced with "nskey": DANDI_NSKEY in 702f464 RF: address PR review - replace readOnly with nskey, fix extensions field.
|
@yarikoptic this looks great! It will be exciting to be able to query the archive for NWB version and extension usage for data going forward. I agree with @candleindark that this probably should not be readOnly. Other than that, looks good. It would be great to also see a draft of dandi-cli that extracts this information. I'd prefer to have a draft of that before merging this, in case we are able to catch any edge cases. |
It is there waiting for you |
candleindark
left a comment
There was a problem hiding this comment.
Left some minor suggestions for change
| dataStandard: Optional[List[StandardsType]] = Field( | ||
| None, | ||
| description="Data standard(s) applicable to this asset.", | ||
| json_schema_extra={"readOnly": True}, |
There was a problem hiding this comment.
Shouldn't we have an "nskey" key in the json_schema_extra dict? If dataStandard is not defined in a known ontology, we I think we should use ours, i.e.,
json_schema_extra={"nskey": DANDI_NSKEY}
There was a problem hiding this comment.
Agreed — added "nskey": DANDI_NSKEY in 702f464 RF: address PR review - replace readOnly with nskey, fix extensions field.
There was a problem hiding this comment.
it should still have readOnly as this is a metadata element that should not be editable using the UI or API. it is also filled in on the server side.
There was a problem hiding this comment.
IIRC that readOnly only for web ui entry -- we are the ones to provide metadata records via API while uploading assets. So if we forbid that in API, nothing would fill that in AFAIK.
webUI does not ATM care about any asset level metadata entry, so all that readOnly is of no effect and somewhat inconsistent ATM anyways so I would not split hairs over it if someone pushes 'readOnly' there
There was a problem hiding this comment.
i'm pretty positive asset summary is filled by the server validation.
There was a problem hiding this comment.
sorry didn't realize this is asset level. i was thinking dandiset level. however, there are a set of fields that the server unsets when uploaded.
| description="Version of the standard used.", | ||
| json_schema_extra={"nskey": "schema"}, | ||
| ) | ||
| extensions: Optional[List["StandardsType"]] = Field( |
There was a problem hiding this comment.
| extensions: Optional[List["StandardsType"]] = Field( | |
| extensions: Optional[List[StandardsType]] = Field( |
With from __future__ import annotations at the beginning of the file, you don't need the type to be wrapped in a string.
There was a problem hiding this comment.
Right — removed the quotes since from __future__ import annotations handles it. Fixed in 702f464 RF: address PR review - replace readOnly with nskey, fix extensions field.
| extensions: Optional[List["StandardsType"]] = Field( | ||
| None, | ||
| description="Extensions to the standard used in this dataset " | ||
| "(e.g. NWB extensions like ndx-*, HED library schemas).", |
There was a problem hiding this comment.
Shouldn't we set "nskey" key in the json_schema_extra dict? If extensions is not defined in a known ontology, we I think we should use ours, i.e.,
json_schema_extra={"nskey": DANDI_NSKEY}
There was a problem hiding this comment.
Agreed — added "nskey": DANDI_NSKEY to extensions in 702f464 RF: address PR review - replace readOnly with nskey, fix extensions field.
| identifier="DOI:10.25504/FAIRsharing.9af712", | ||
| ).model_dump(mode="json", exclude_none=True) | ||
|
|
||
| hed_standard = StandardsType( |
There was a problem hiding this comment.
Defining hed_standard in lower case is consistent with the established pattern, as in ome_ngff_standard. However, all the standard variables, nwb_standard, bids_standard, ome_ngff_standard, and hed_standard should be specified in upper case laters since they function as constants.
I think that should be done in a separate PR though.
There was a problem hiding this comment.
Agreed — keeping lowercase here for consistency with the existing pattern, and the rename to UPPER_CASE can be done in a separate PR as you suggest.
|
I approve the dandi dli PR. Once we respond to @candleindark 's comments, this is good to go from my end |
…ield Address @candleindark's review comments on PR #371: - BareAsset.dataStandard: replace readOnly=True with nskey=DANDI_NSKEY (field is populated client-side by dandi-cli, not server-managed) - StandardsType.extensions: add nskey=DANDI_NSKEY, fix description ("in this dataset" -> removed, since standard applies per-asset), and remove unnecessary string quotes on forward ref (from __future__ import annotations handles it) Co-Authored-By: Claude Code 2.1.104 / Claude Opus 4.6 <noreply@anthropic.com>
| @@ -880,6 +897,11 @@ class StandardsType(BaseType): | |||
| identifier="DOI:10.25504/FAIRsharing.9af712", | |||
| ).model_dump(mode="json", exclude_none=True) | |||
There was a problem hiding this comment.
@kabilar please add additional constructs for anything LINC related -- like those for bigtiff etc, and then here
as to use them in dandi-cli
…s on StandardsType - Bump DANDI_SCHEMA_VERSION to 0.8.0, add 0.7.0 to ALLOWED_INPUT_SCHEMAS - Add `dataStandard: Optional[List[StandardsType]]` to BareAsset for per-asset standard declarations (NWB, BIDS, HED, OME/NGFF) - Add `version: Optional[str]` to StandardsType - Add `extensions: Optional[List["StandardsType"]]` (self-referencing) to StandardsType for NWB ndx-* extensions, HED library schemas, etc. - Add `hed_standard` constant (RRID:SCR_014074) - Collect per-asset dataStandard in aggregate_assets_summary(), with deprecated path/encoding heuristic fallbacks (remove after 2026-12-01) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…klist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ield Address @candleindark's review comments on PR #371: - BareAsset.dataStandard: replace readOnly=True with nskey=DANDI_NSKEY (field is populated client-side by dandi-cli, not server-managed) - StandardsType.extensions: add nskey=DANDI_NSKEY, fix description ("in this dataset" -> removed, since standard applies per-asset), and remove unnecessary string quotes on forward ref (from __future__ import annotations handles it) Co-Authored-By: Claude Code 2.1.104 / Claude Opus 4.6 <noreply@anthropic.com>
- Bump DANDI_SCHEMA_VERSION to 0.9.0 since 0.8.0 is already released without dataStandard on assets and version/extensions on StandardsType; allow 0.8.0 as an input and downgrade target. - migrate(): when downgrading below 0.9.0, drop per-asset dataStandard (LOSSY_DOWNGRADES: it is extracted automatically, so nothing user-provided is lost) and strip version/extensions from assetsSummary.dataStandard. dandi-cli applies migrate() to outgoing asset metadata for older servers (the production one is at 0.7.0), and refusing to downgrade would leave such assets with a schemaVersion the server cannot validate. - aggregate_assets_summary(): merge entries per standard and version, uniting extensions, and drop the bare entries added by heuristics for older clients' assets when a versioned entry exists. Otherwise every dandiset with new-client uploads would list e.g. NWB twice (the web UI shows only names). - Reword the "remove after 2026-12-01" note: heuristics are needed for as long as there are assets without dataStandard on the server. - Tests marked ai_generated; imports moved to module top. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P3VJkUsu17gjmUEf4ZY1RW
Adds per-asset
dataStandard(BareAsset.dataStandard: List[StandardsType]),versionandextensions(e.g. NWBndx-*, HED library schemas) onStandardsType, and ahed_standardconstant, so dandi-cli can declare which standards (and versions/extensions) each asset uses and the server can aggregate them intoassetsSummary. Schema version is bumped to 0.9.0 (0.8.0 is already published).aggregate_assets_summary()collects per-assetdataStandard, merging entries per standard and version (extensions united); the path/encoding heuristics remain for assets from older clients, and their bare entries are dropped when a versioned entry for the same standard exists, so e.g. NWB is not listed twice (the web UI shows only names).migrate()downgrades below 0.9.0 by dropping per-assetdataStandard(newLOSSY_DOWNGRADES: it is extracted automatically, nothing user-provided is lost) and strippingversion/extensionsfromassetsSummary.dataStandard. This matters since dandi-cli now downgrades all outgoing metadata, assets included, to the server's schema (production is at 0.7.0).Companion: dandi/dandi-cli#1811 (tested together end-to-end, see below).
Details: review responses, design notes, testing
Review responses
readOnlyonBareAsset.dataStandard: dropped in favor ofnskey: dandi, since it is populated by the client via the API (@candleindark, @bendichter; @satra later noted it is asset-, not dandiset-level, metadata).nskeyadded toextensions; description no longer says "in this dataset"; forward-ref quotes removed.*_standardconstants: left for a separate PR, as agreed.Changes since the previous review round (rebased onto current master)
BareAsset/Asset, downgrade framework from Add ability to downgrade schema all the way to 0.6.10 #342, schema 0.8.0).migrate()docstring updated: it also downgrades Asset metadata.DANDI_SCHEMA_VERSION0.8.0 → 0.9.0; add 0.8.0 toALLOWED_INPUT_SCHEMASandALLOWED_TARGET_SCHEMAS.dataStandard-- the heuristics also serve already-uploaded assets.RRID:SCR_014074is HED's primary RRID.Design note: dropping vs keeping
dataStandardon downgradeSince
DandiBaseModelignores extra fields, keepingdataStandardin metadata relabeled as 0.7.0 would also validate on old servers and preserve it for later aggregation, but it would make records claim a schema version they do not conform to. I went with explicit removal; the metadata can be re-sent (e.g.dandi upload --existing force) once the server is upgraded.Known follow-up for dandi-archive
The web UI lists
assetsSummary.dataStandardentries bynameonly. A dandiset mixing e.g. NWB 2.7.0 and 2.8.0 files gets one entry per version, which would show as two identical "Neurodata Without Borders (NWB)" rows until the UI also showsversion(and possiblyextensions).Testing
tox-equivalent locally: 265 passed, 19 skipped (no network); flake8/black/isort/codespell clean; mypy clean.ndx-hed/ndx-pose, a.ome.zarr, and bids-exampleseeg_ds003645s_hed_library(HEDVersion: ["8.4.0", "sc:score_1.0.0", "test:testlib_1.0.2"]):Valid(3 NWB/Zarr, 32 BIDS)ValidschemaVersiondataStandardndx-hed0.2.0 (+ndx-pose0.4.0); OME/NGFF; BIDS 1.11.1 on every BIDS asset; HED 8.4.0 +score1.0.0,testlib1.0.2 ondataset_description.jsonassetsSummary.dataStandardndx-hed,ndx-pose}; BIDS 1.11.1, HED 8.4.0 + {score,testlib} -- one entry per standard(Images from mirror.gcr.io; the 0.9.0 server is the stock
dandiarchive/dandiarchive-apiwith the dandischema wheel from this branch installed, astools/api-with-schema.Dockerfiledoes. The summary is aggregated only fromValidassets when the draft becomes pending, so the test touched the dandiset metadata after validation to get it re-aggregated.)🤖 Generated with Claude Code
https://claude.ai/code/session_01P3VJkUsu17gjmUEf4ZY1RW