Skip to content

Do not traverse .ome.zarr and .ngff folders while looking for BIDS datasets#1071

Merged
yarikoptic merged 4 commits intomasterfrom
enh-reporting
Jul 19, 2022
Merged

Do not traverse .ome.zarr and .ngff folders while looking for BIDS datasets#1071
yarikoptic merged 4 commits intomasterfrom
enh-reporting

Conversation

@yarikoptic
Copy link
Member

Fixes #1069

TODO

  • check on real case if that helps enough

…tories

e.g. to prevent searching for files under .zarr/ folders with thousands for files
…dataset_description.json

Also added a log line so process is not just hanging while traversing but we know
that it might be going through folders. Ideally we should add some debug messages in find_files I guess
to alert user that we are not dead
@yarikoptic yarikoptic requested review from TheChymera and jwodder July 18, 2022 21:27
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (045e9ff) to head (716453b).
Report is 1013 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1071      +/-   ##
==========================================
+ Coverage   88.38%   88.40%   +0.02%     
==========================================
  Files          72       72              
  Lines        9261     9272      +11     
==========================================
+ Hits         8185     8197      +12     
+ Misses       1076     1075       -1     
Flag Coverage Δ
unittests 88.40% <100.00%> (+0.02%) ⬆️

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.

@yarikoptic
Copy link
Member Author

ok -- with this I fail (incorrect token) on our target use case within 3 seconds, so we manage to traverse enough fast enough, so effectively addresses the case. but may be we would tune bids detection more - see #1073

@yarikoptic yarikoptic added release Create a release when this pr is merged performance Improve performance of an existing feature labels Jul 18, 2022
@satra
Copy link
Member

satra commented Jul 18, 2022

@yarikoptic - does the check still run when --allow-any-path is used? as in is there a necessity of any bids-specific traversal other than dandiset.yaml detection?

and secondarily, if we don't use that flag and do --validation skip then does it care for anything else other than the specific directory contents and the presence of a dataset_description.json. i.e. it should not care about files in the other subject's directory.

(and the one ngff file in MITU01 is being removed).

@yarikoptic
Copy link
Member Author

@yarikoptic - does the check still run when --allow-any-path is used?

the patch is only for _bids_discover_and_validate( for discovery of dataset_description.json, no other operation should be affected AFAIK.

and secondarily, if we don't use that flag and do --validation skip then does it care for anything else other than the specific directory contents and the presence of a dataset_description.json. i.e. it should not care about files in the other subject's directory.

I don't think this patch should affect that behavior either, IMHO should not . If that behavior is wrong, we need a separate issue issue for that particular case - I am just trying to fix up #1069 here.

@yarikoptic
Copy link
Member Author

filed #1078 for a failed test, filed #1077 for a separate aspect identified by @jwodder , got blessing from @jwodder. So Let's proceed with this but I will delay release -- may be more PRs would get merged in short time.

@yarikoptic yarikoptic merged commit 1c94736 into master Jul 19, 2022
@yarikoptic yarikoptic deleted the enh-reporting branch July 19, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Improve performance of an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hanging dandi upload call

4 participants