Updating BIDS validator and schema to contemporary upstream equivalent by TheChymera · Pull Request #1050 · dandi/dandi-cli

TheChymera · 2022-07-08T04:17:28Z

Brought our code back in sync with upstream BIDS, as we finally have upstream ome.zarr support and there is no longer any reason to maintain a fork of the specification.
This also fixes: #1037

Ideally this will be the last update of bundled code as we are working on importing the built-in Python validator as a library.
Should the need for specification forking arise in the future, this should best be handled via the parameterized schema directory.

~~Draft for now as there might be inconsistencies I haven't spotted.~~

codecov · 2022-07-08T04:29:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.48%. Comparing base (50ca7b0) to head (468e39c).
Report is 1023 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1050      +/-   ##
==========================================
+ Coverage   88.41%   88.48%   +0.07%     
==========================================
  Files          72       73       +1     
  Lines        9251     9291      +40     
==========================================
+ Hits         8179     8221      +42     
+ Misses       1072     1070       -2

Flag	Coverage Δ
unittests	`88.48% <100.00%> (+0.07%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheChymera · 2022-07-08T04:54:22Z

Additionally, we might want to start using the reference bids-examples repository for validation (rather than our own fork).
To be able to perform all relevant tests, this would require having at least one broken dataset in the examples, so that we can verify errors as well: bids-standard/bids-examples#327

In any case, using vanilla upstream would add another ~10s to the BIDS validator tests; as our fork had stripped a lot of the datasets. While we will prospectively no longer run the validator tests themselves (these would be run by the bids-schemacode package), we still need BIDS example data to construct our derivative BIDS datasets for higher-level function testing...

Any ideas if we can somehow check out only some of the files to reduce download time? @yarikoptic

dandi/bids_validator_xs.py

e956afc69f7901a5848d2d91187d3036a6d7eb4b

bids-standard/bids-specification@e1eaea3

lgtm-com · 2022-07-09T02:09:44Z

This pull request introduces 1 alert when merging 4257ef8 into 50ca7b0 - view on LGTM.com

new alerts:

1 for Unused local variable

yarikoptic

only started, need to switch, please attend to --report option meanwhile

yarikoptic · 2022-07-14T14:37:08Z

dandi/cli/cmd_validate.py

-    help="Whether to write a report under a unique path in the current directory. "
-    "Only usable if `--report` is not already used.",
+    help="Whether to write a report under a unique path in the current directory. ",
 )


so we are breaking CLI ... why not just to keep --report to be an option to provide the path and if not specified, assume that no report writing was requested - print to the screen?
Note: I might not be even able to write to current directory (dataset might be owned by someone else).

functionality wasn't broken, help text was just incorrect.

You removed --report-flag and made --report from taking a string (path) into being is_flag -- so, it got broken that user no longer would be able to say --report myvalidation.log. And I think it is worth removing somewhat odd --report-flag so ok to break but we better "make it right" this time and avoid future breakage.
If you really want to be fancy and support also bool like behavior, I guess you would need to make it nargs='?' (if click supports it, @jwodder could help) and then if not report - assign that default path to the log, so user could do both --report and --report mylog.log.
Alternative -- not bother with "bool" like behavior in CLI, and just make it demand path string.

NB: please do not mark such comments Resolved - let original Author decide if they were resolved or not since it makes it harder for a reviewer to locate prior comments while re-reviewing and see if prior concerns were addressed.

yarikoptic

--report handling still needs to be fixed and tested better

dandi/validate.py

yarikoptic · 2022-07-15T13:21:19Z

dandi/cli/cmd_validate.py

-    help="Whether to write a report under a unique path in the current directory. "
-    "Only usable if `--report` is not already used.",
+    help="Whether to write a report under a unique path in the current directory. ",
 )


You removed --report-flag and made --report from taking a string (path) into being is_flag -- so, it got broken that user no longer would be able to say --report myvalidation.log. And I think it is worth removing somewhat odd --report-flag so ok to break but we better "make it right" this time and avoid future breakage.
If you really want to be fancy and support also bool like behavior, I guess you would need to make it nargs='?' (if click supports it, @jwodder could help) and then if not report - assign that default path to the log, so user could do both --report and --report mylog.log.
Alternative -- not bother with "bool" like behavior in CLI, and just make it demand path string.

NB: please do not mark such comments Resolved - let original Author decide if they were resolved or not since it makes it harder for a reviewer to locate prior comments while re-reviewing and see if prior concerns were addressed.

TheChymera · 2022-07-19T23:10:38Z

@yarikoptic

so, it got broken that user no longer would be able to say --report myvalidation.log

It didn't get broken, the feature was simply removed. I added it initially because I thought it would be helpful, and I decided that in fact it is not. The validation() function which I understand we want to merge BIDS into at some point does not have such a parameter, and since DANDI doesn't manage logging this way I see no reason to do it for this function specifically...

dandi/cli/cmd_validate.py

dandi/support/bids/validator.py

dandi/cli/cmd_validate.py

dandi/support/bids/validator.py

jwodder · 2022-07-21T14:11:43Z

dandi/support/bids/validator.py

 def write_report(
    validation_result,
-    report_path="{logdir}/bids-validator-report_{datetime}-{pid}.log",
+    report_path="/var/tmp/bids-validator/report_{datetime}-{pid}.log",


I suggest still using a variable for the log base directory, but set it to $TMPDIR (falling back to /tmp) by default.

This is for upstream usage, we wrap this command and pass the DANDI log directory via appdirs. /tmp/ is tricky as it's ephemeral on some systems, and users might not know how their system handles it. /var/tmp is the safer bet.

Compare:
https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s15.html
to
https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch03s18.html
Particularly:

Programs must not assume that any files or directories in /tmp are preserved between invocations of the program.

is pretty much a non-starter for logging which might need to be consulted over a longer period.

jwodder · 2022-07-21T14:14:33Z

dandi/support/bids/validator.py

 def validate_bids(
    bids_paths,
-    schema_reference_root="{module_path}/support/bids/schemadata/",
+    schema_reference_root="/usr/share/bids-schema/",


Under what conditions is this default used, and why would we expect anything to be in this directory?

In the DANDI use case this is never used, but this is bundled code from upstream, which we will soon no longer track but import from a python dependency. Prospectively, the dependency would find the BIDS schema index in that directory, as that is where architecture-independent read-only user data belongs as per the FHS.

dandi/tests/test_validate.py

Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

…date

dandi/support/bids/validator.py

Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

…date

TheChymera · 2022-07-27T00:51:46Z

@jwodder thanks for your input :3 good to go now?

yarikoptic · 2022-07-28T19:24:06Z

ok, I think that main discussions/questions were addressed/resolved. Let's proceed and see where it takes us. Thank you @TheChymera !

TheChymera marked this pull request as ready for review July 8, 2022 06:22

yarikoptic reviewed Jul 8, 2022

View reviewed changes

dandi/bids_validator_xs.py Show resolved Hide resolved

yarikoptic reviewed Jul 8, 2022

View reviewed changes

dandi/bids_validator_xs.py Show resolved Hide resolved

TheChymera marked this pull request as draft July 9, 2022 00:55

TheChymera force-pushed the bids_update branch 2 times, most recently from 45b386f to 036e41d Compare July 9, 2022 01:01

TheChymera added 5 commits July 8, 2022 21:59

Upstream validation features up to and including:

a13418a

e956afc69f7901a5848d2d91187d3036a6d7eb4b

Schema update

46988a3

Reverting to DANDI fork of bids-examples

8405a99

Docstring fix

fc5a720

Record BIDS schema version, analogous to:

4baf689

bids-standard/bids-specification@e1eaea3

TheChymera force-pushed the bids_update branch from 036e41d to 4257ef8 Compare July 9, 2022 02:02

Logging to appdirs when BIDS validation is called via DANDI wrapper

34a2af2

TheChymera force-pushed the bids_update branch from 4257ef8 to 34a2af2 Compare July 9, 2022 02:11

TheChymera added 2 commits July 12, 2022 13:48

Moved external modules to dandi/support/*/

a67c91e

formatting changes to stay in sync with upstream BIDS

cff8c89

TheChymera marked this pull request as ready for review July 14, 2022 09:23

TheChymera requested a review from yarikoptic July 14, 2022 09:23

yarikoptic requested changes Jul 14, 2022

View reviewed changes

Corrected help text

2ed8929

TheChymera requested a review from yarikoptic July 14, 2022 16:08

TheChymera mentioned this pull request Jul 14, 2022

[ENH] BIDS Metadata reporting #1061

Draft

4 tasks

yarikoptic requested changes Jul 15, 2022

View reviewed changes

TheChymera added 3 commits July 19, 2022 17:05

Updated report docstring in wrapper

73a2061

verify report file creation

c367e05

Moved bash usage example to CLI function

b514d8a

TheChymera added 2 commits July 20, 2022 11:46

Dropped debugging print line

d35e5f2

Nicer conditional for recursion

cd0b36c

yarikoptic reviewed Jul 20, 2022

View reviewed changes

dandi/cli/cmd_validate.py Outdated Show resolved Hide resolved

TheChymera added 2 commits July 20, 2022 12:22

Removed deprecated parameter

a444368

Reinstated option for report path specification

4d4a7cf

TheChymera requested review from jwodder and yarikoptic July 20, 2022 21:49

TheChymera marked this pull request as ready for review July 20, 2022 21:51

jwodder suggested changes Jul 21, 2022

View reviewed changes

TheChymera and others added 7 commits July 22, 2022 03:29

code style improvement

3bcc542

Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

style improvements

140a7cf

Merge branch 'bids_update' of github.com:dandi/dandi-cli into bids_up…

335bf62

…date

Improved windows support

b582485

Using pytest's tmp_path

02a6e95

Updated library name

8c1dbcc

removed unused default values from function wrapped with click

41fa4ac

TheChymera requested a review from jwodder July 22, 2022 21:22

jwodder suggested changes Jul 25, 2022

View reviewed changes

dandi/support/bids/validator.py Show resolved Hide resolved

dandi/support/bids/validator.py Outdated Show resolved Hide resolved

jwodder added the BIDS label Jul 26, 2022

TheChymera and others added 3 commits July 26, 2022 18:09

Simplified directory exclusion logic

75e8fc3

Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

Merge branch 'bids_update' of github.com:dandi/dandi-cli into bids_up…

ae6970a

…date

Typo

3f43905

TheChymera mentioned this pull request Jul 26, 2022

bids validation: unclear purpose of an O(N^2) code #1070

Closed

Do not index top-level files from pseudofile directories

468e39c

TheChymera mentioned this pull request Jul 27, 2022

dandi validate-bids crashes with IndexError: list index out of range #1037

Closed

TheChymera mentioned this pull request Jul 28, 2022

bids validation -- report error on invalid json of dataset_description.json #988

Closed

yarikoptic merged commit 3655aa1 into master Jul 28, 2022

yarikoptic deleted the bids_update branch July 28, 2022 19:24

Conversation

TheChymera commented Jul 8, 2022 • edited by yarikoptic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheChymera commented Jul 8, 2022

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Jul 9, 2022

Uh oh!

yarikoptic left a comment

Choose a reason for hiding this comment

Uh oh!

yarikoptic Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

TheChymera Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

yarikoptic Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

yarikoptic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yarikoptic Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

TheChymera commented Jul 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jwodder Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

TheChymera Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwodder Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

TheChymera Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheChymera commented Jul 27, 2022

Uh oh!

yarikoptic commented Jul 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheChymera commented Jul 8, 2022 •

edited by yarikoptic

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

TheChymera Jul 22, 2022 •

edited

Loading