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

Logging validation warnings and errors #992

Merged
merged 9 commits into from
May 9, 2022
Merged

Conversation

TheChymera
Copy link
Contributor

Addressing #991

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #992 (803de15) into master (cca19df) will increase coverage by 0.11%.
The diff coverage is 97.67%.

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
+ Coverage   87.49%   87.60%   +0.11%     
==========================================
  Files          64       65       +1     
  Lines        8110     8151      +41     
==========================================
+ Hits         7096     7141      +45     
+ Misses       1014     1010       -4     
Flag Coverage Δ
unittests 87.60% <97.67%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
dandi/bids_validator_xs.py 92.56% <91.66%> (+0.28%) ⬆️
dandi/cli/cmd_validate.py 89.39% <100.00%> (+8.62%) ⬆️
dandi/cli/tests/test_cmd_validate.py 100.00% <100.00%> (ø)
dandi/tests/test_bids_validator_xs.py 100.00% <100.00%> (ø)
dandi/support/threaded_walk.py 92.85% <0.00%> (-1.79%) ⬇️
dandi/validate.py 100.00% <0.00%> (+25.00%) ⬆️

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 cca19df...803de15. Read the comment docs.

@yarikoptic
Copy link
Member

per our brief zoom chat -- let's add raise SystemExit(1) in case of any error detected at the level of the click interface function (similarly to how dandi validate does)

@TheChymera
Copy link
Contributor Author

@yarikoptic should be in, issue being that validation can fail due to 2 separate reasons:

  • missing required files (more severe from a validation point of view).
  • incorrectly formatted files (not really a big deal from the point of view of validation, which is why most datasets have .gitignore) but as I understand it, it is a big deal for DANDI.

The issue is that since we raise errors, it will stop on the first. I imagine someone might fix the first, and then get confused by running into the second. Demo output for the archive from the linked issue:

chymera@decohost ~ $ dandi validate-bids test -r
2022-05-02 05:41:54,190 [    INFO] Note: NumExpr detected 12 cores but "NUMEXPR_MAX_THREADS" not set, so enforcing safe limit of 8.
2022-05-02 05:41:54,191 [    INFO] NumExpr defaulting to 8 threads.
2022-05-02 05:41:55,452 [ WARNING] BIDSVersion 1.5.0 is less than the minimal working 1.7.0+012+dandi001. Falling back to 1.7.0+012+dandi001. To force the usage of earlier versions specify them explicitly when calling the validator.
2022-05-02 05:41:56,231 [    INFO] BIDS validation log written to /home/chymera/.cache/dandi-cli/log/bids-validator-report_20220502094156Z-17211.log
2022-05-02 05:41:56,231 [   ERROR] The `.*?/README$` regex pattern file required by BIDS was not found.
2022-05-02 05:41:56,231 [   ERROR] The `.*?/CHANGES$` regex pattern file required by BIDS was not found.
2022-05-02 05:41:56,231 [ WARNING] The `/home/chymera/test/sub-MITU02/ses-MR2/sub-MITU01_ses-MR_SSSPDmap.nii.gz` file was not matched by any regex schema entry.
Summary: 2 BIDS required filename patterns could not be found.
2022-05-02 05:41:56,231 [    INFO] Logs saved in /home/chymera/.cache/dandi-cli/log/20220502094153Z-17211.log

@TheChymera
Copy link
Contributor Author

Actually, let me set up some extra logic to put everything together in one message.

@TheChymera
Copy link
Contributor Author

Ok, it integrates both kinds now and also speaks proper english:

chymera@decohost ~ $ dandi validate-bids test -r
2022-05-02 06:12:22,296 [    INFO] Note: NumExpr detected 12 cores but "NUMEXPR_MAX_THREADS" not set, so enforcing safe limit of 8.
2022-05-02 06:12:22,296 [    INFO] NumExpr defaulting to 8 threads.
2022-05-02 06:12:23,543 [ WARNING] BIDSVersion 1.5.0 is less than the minimal working 1.7.0+012+dandi001. Falling back to 1.7.0+012+dandi001. To force the usage of earlier versions specify them explicitly when calling the validator.
2022-05-02 06:12:24,321 [    INFO] BIDS validation log written to /home/chymera/.cache/dandi-cli/log/bids-validator-report_20220502101224Z-18698.log
2022-05-02 06:12:24,321 [   ERROR] The `.*?/README$` regex pattern file required by BIDS was not found.
2022-05-02 06:12:24,321 [   ERROR] The `.*?/CHANGES$` regex pattern file required by BIDS was not found.
2022-05-02 06:12:24,321 [ WARNING] The `/home/chymera/test/sub-MITU02/ses-MR2/sub-MITU01_ses-MR_SSSPDmap.nii.gz` file was not matched by any regex schema entry.
Summary: 2 BIDS required filename patterns could not be found and 1 filename did not match any pattern known to BIDS.
2022-05-02 06:12:24,322 [    INFO] Logs saved in /home/chymera/.cache/dandi-cli/log/20220502101221Z-18698.log

@TheChymera TheChymera marked this pull request as ready for review May 2, 2022 14:56
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.

Thanks!
I left some comments without bundling into this review. Overall -- seems to achieve desired (for now) output, but code could benefit from a bit of more Python-ification - I left some hints.

edit: also filed independent #997 which could be done separately or along with this PR to ensure that CLI code at least has some testing.

dandi/cli/cmd_validate.py Outdated Show resolved Hide resolved
dandi/cli/cmd_validate.py Outdated Show resolved Hide resolved
@yarikoptic yarikoptic added the release Create a release when this pr is merged label May 4, 2022
@TheChymera
Copy link
Contributor Author

Errors don't look like they're related to the PR :-/

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.

a few more RFs I requested in individual comments. We are converging.

@TheChymera
Copy link
Contributor Author

@yarikoptic done, I don't think list comprehension for error_list = [] will improve legibility at all, since it uses different conditionals.

@yarikoptic
Copy link
Member

@yarikoptic done, I don't think list comprehension for error_list = [] will improve legibility at all, since it uses different conditionals.

right, it is not a "loopy thing", my bad. Let's proceed. Thanks!

@yarikoptic yarikoptic merged commit 10ca263 into master May 9, 2022
@yarikoptic yarikoptic deleted the validation_errors branch May 9, 2022 21:03
@github-actions
Copy link

github-actions bot commented May 9, 2022

🚀 PR was released in 0.39.6 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BIDS release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants