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

Merging validation functions #1154

Closed
wants to merge 95 commits into from
Closed

Merging validation functions #1154

wants to merge 95 commits into from

Conversation

TheChymera
Copy link
Contributor

@TheChymera TheChymera commented Nov 10, 2022

Currently non-working. The main problem seems to be that the old (functional) design ran the validation from validate.py, whereas the new (object-oriented) design runs it as part of the asset instantiation. This makes a lot of things a bit confusing. Currently the immediate issue is that if we validate via BIDS object instantiation, we get the same error reported over and over. Probably has to do with the vanilla validation function using yield versus return... Will continue digging, though comments are appreciated @yarikoptic @jwodder

I'm currently testing this on DANDI:000108 :

(dev) chymera@decohost ~/.local/share/datalad $ dandi validate 000108
2022-11-09 18:57:11,195 [ WARNING] BIDSVersion `1.4.0` is less than the minimal working `schema`. Falling back to `schema`. To force the usage of earlier versions specify them explicitly when calling the validator.
2022-11-09 18:57:12,329 [   ERROR] The `README(?P<extension>|\.md|\.rst|\.txt)` regex pattern file required by BIDS was not found.
..
ValidationResult(id='BIDS.MANDATORY_FILE_MISSING_PLACEHOLDER', origin=ValidationOrigin(name='bidsschematools', version='0.6.0'), scope=<Scope.DATASET: 'dataset'>, asset_paths=None, within_asset_paths=None, dandiset_path=None, dataset_path=None, message='BIDS-required file is not present.', metadata=None, path=None, path_regex='README(?P<extension>|\\.md|\\.rst|\\.txt)', severity=<Severity.ERROR: 'ERROR'>)
ii
ValidationResult(id='BIDS.MANDATORY_FILE_MISSING_PLACEHOLDER', origin=ValidationOrigin(name='bidsschematools', version='0.6.0'), scope=<Scope.DATASET: 'dataset'>, asset_paths=None, within_asset_paths=None, dandiset_path=None, dataset_path=None, message='BIDS-required file is not present.', metadata=None, path=None, path_regex='README(?P<extension>|\\.md|\\.rst|\\.txt)', severity=<Severity.ERROR: 'ERROR'>)
ii
ValidationResult(id='BIDS.MATCH', origin=ValidationOrigin(name='bidsschematools', version='0.6.0'), scope=<Scope.FILE: 'file'>, asset_paths=None, within_asset_paths=None, dandiset_path=PosixPath('/home/chymera/.local/share/datalad/000108'), dataset_path=PosixPath('/home/chymera/.local/share/datalad/000108'), message=None, metadata={}, path=PosixPath('/home/chymera/.local/share/datalad/000108/dataset_description.json'), path_regex=None, severity=None)
kk
ValidationResult(id='BIDS.MATCH', origin=ValidationOrigin(name='bidsschematools', version='0.6.0'), scope=<Scope.FILE: 'file'>, asset_paths=None, within_asset_paths=None, dandiset_path=PosixPath('/home/chymera/.local/share/datalad/000108'), dataset_path=PosixPath('/home/chymera/.local/share/datalad/000108'), message=None, metadata={}, path=PosixPath('/home/chymera/.local/share/datalad/000108/dataset_description.json'), path_regex=None, severity=None)
kk
/home/chymera/.local/share/datalad/000108/dataset_description.json
kk1
kk2
kk3
kk4
..
Counter({'BIDS.MANDATORY_FILE_MISSING_PLACEHOLDER': 3250})
2022-11-09 18:57:12,835 [    INFO] Logs saved in /home/chymera/.cache/dandi-cli/log/20221109235709Z-11928.log
Error: No active exception to reraise

@TheChymera TheChymera marked this pull request as draft November 10, 2022 00:04
@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2022

This pull request introduces 1 alert when merging 7261a91 into c66fd18 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 88.29% // Head: 89.08% // Increases project coverage by +0.78% 🎉

Coverage data is based on head (9aae3bf) compared to base (c66fd18).
Patch coverage: 84.39% of modified lines in pull request are covered.

❗ Current head 9aae3bf differs from pull request most recent head b4e2e59. Consider uploading reports for the commit b4e2e59 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   88.29%   89.08%   +0.78%     
==========================================
  Files          73       76       +3     
  Lines        8800     9444     +644     
==========================================
+ Hits         7770     8413     +643     
- Misses       1030     1031       +1     
Flag Coverage Δ
unittests 89.08% <84.39%> (+0.78%) ⬆️

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

Impacted Files Coverage Δ
dandi/cli/cmd_validate.py 73.03% <0.00%> (-0.83%) ⬇️
dandi/tests/test_dandiapi.py 100.00% <ø> (ø)
dandi/support/pyout.py 90.00% <50.00%> (ø)
dandi/dandiapi.py 87.11% <59.37%> (-1.56%) ⬇️
dandi/pynwb_utils.py 82.68% <66.66%> (-0.80%) ⬇️
dandi/metadata.py 87.46% <68.00%> (+0.44%) ⬆️
dandi/cli/cmd_ls.py 76.88% <83.33%> (+1.31%) ⬆️
dandi/files/_private.py 98.11% <87.50%> (+0.07%) ⬆️
dandi/organize.py 83.54% <97.29%> (+0.28%) ⬆️
dandi/cli/cmd_organize.py 100.00% <100.00%> (ø)
... and 25 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dandi/files/bids.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2022

This pull request introduces 1 alert when merging 242be00 into c66fd18 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

jwodder and others added 20 commits November 11, 2022 11:10
Update `test_validate_nwb_path_grouping` test
Fix typing error under mypy 0.990
No longer mark `test_rename_type_mismatch` as xfailing
also improved test function docstrings
there must be a better way of putting longer links in docstrings...
Renamed failing test, added use case for NWBI warning.
with the appropriate error message if there is only one, and a period if
not.
Corrected reporting function logic to complete group message variable
Add CodeQL workflow for GitHub code scanning and fix few bugs it detected
@yarikoptic
Copy link
Member

@TheChymera what is the roadmap for this PR?

@TheChymera
Copy link
Contributor Author

TheChymera commented Dec 22, 2022

Merge conflicts after #1164 (which did part of what I was trying to do here but just for ls). After going through open validation issues probably required to proceed on them, so fixing this up.

@TheChymera
Copy link
Contributor Author

ok, so this blew up in my face :(


from .bases import GenericAsset, LocalFileAsset, NWBAsset
from .zarr import ZarrAsset
from ..metadata import add_common_metadata, prepare_metadata
from ..consts import ZARR_MIME_TYPE
from ..metadata import add_common_metadata

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [dandi.metadata](1) begins an import cycle.
@TheChymera TheChymera closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants