-
Notifications
You must be signed in to change notification settings - Fork 107
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
HED Validator integration into bids schema based validator. #1648
base: master
Are you sure you want to change the base?
Conversation
…ion. Add hed validator dep built from current master.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1648 +/- ##
==========================================
- Coverage 88.57% 85.68% -2.90%
==========================================
Files 39 91 +52
Lines 2372 3792 +1420
Branches 273 1220 +947
==========================================
+ Hits 2101 3249 +1148
- Misses 265 457 +192
- Partials 6 86 +80 ☔ View full report in Codecov by Sentry. |
@rwblair --- should we be nervous that this is PR is still a draft? Is there anything that we need to prepare for? Thx! VisLab |
…. add logger debug for response from hed validator
I know you have been swamped, but could you give us an update on this @nellh? Thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready to merge. I ran through some test cases and was able to match the failures to the legacy validator. ds004395 runs out of memory with the default heap size but does pass increasing the size to 8GB. There could be some memory optimization to be done because this doesn't happen without the HED patches but that dataset is an exception, every other test case passed with the default heap size.
@nellh ... thx for update. I am not sure what is going on with ds004395
-- I looked at it on openNeuro and it doesn't have any HED in it. The new
HED validator works directly from the list of .tsv files and their
associated sidecars as produced from the BIDS side. I wonder if we are
somehow copying the data inadvertently before determining whether to even
look at the files.
@alexander Jones ***@***.***> let's look at this more closely.
Version 3.13.3 should be released in the next day or so. It has some minor
bug fixes.
…On Thu, Jan 11, 2024 at 11:58 AM Nell Hardcastle ***@***.***> wrote:
***@***.**** approved this pull request.
I think this is ready to merge. I ran through some test cases and was able
to match the failures to the legacy validator. ds004395 runs out of memory
with the default heap size but does pass increasing the size to 8GB. There
could be some memory optimization to be done because this doesn't happen
without the HED patches but that dataset is an exception, every other test
case passed with the default heap size.
—
Reply to this email directly, view it on GitHub
<#1648 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJCJOWF6OFLORSN5CB55JTYOAR4BAVCNFSM6AAAAAAWJW4A2SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMJWGE3TEMJVHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I should finish profiling before running my mouth. |
…l hed-validator call commented out for testing memory usage
I'm not sure what's going on with the Deno build. The lines in the errors do not appear in the version of that file in our code base, and we don't use |
@rwblair Has there been any further progress on this? |
… schema/hedIntegration
…path to hed example being tested.
…-validator into schema/hedIntegration
@happy5214 Merged master to get it up to date. The out of memory issue is from keeping an object around with all the sideccar and tsv data in memory while the file tree is walked. I think I was mirroring how the old validator called the hed validator. I'm going to try to:
|
HED processes each tsv and its sidecars separately and does not need to have multiple event files at one time. @happy5214 will need to address the details of the other issues. Thanks for looking at this -- we'd like to get this resolved. |
I rewrote bids-validator/src/validators/hed.ts to work on a file per context basis in line with the 3rd option in my last post. This allows ds004395 to complete validation. I need to rewrite/add tests for this way of invoking HED validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx! In the longer term, we'll probably want to build the HED schema object from the dataset description before validation and pass it in as it doesn't change. We'll get back to you on this. @happy5214 comment?
To be honest, hearing that is a little annoying, because that's how it was originally implemented, and then we implemented a design philosophy to move as much of the code as possible to hed-validator. The schema-loading function is still exported ( |
For now its still passing the dataset_description to each validation call for each file. Not sure how inefficient this is CPU wise, but the memory issue is gone for now. |
That is fine for now. We will work on the underlying implementation.
…On Mon, Jun 17, 2024 at 11:27 AM Ross Blair ***@***.***> wrote:
For now its still passing the dataset_description to each validation call
for each file. Not sure how inefficient this is CPU wise, but the memory
issue is gone for now.
—
Reply to this email directly, view it on GitHub
<#1648 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJCJOUUFY6R34OZNAS7BE3ZH4E7TAVCNFSM6AAAAABI5ZOCD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZTHA2DGMZWHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
For #1639.
bids-validator/src/deps/hed-validator
dependency was generated by runningnpm run build
on latest commit for hed-javascript:hed-standard/hed-javascript@bba927b
Two main functions located in
bids-validator/src/validators/hed.ts
arehedAccumulator
andhedValidate
hedAccumulator
is called once per context like other checks (filename validation, schema checks) inbids-validator/src/validators/bids.ts
. It catches any events.tsv files and json files and builds up an object to be passed to the actual validator call.Instead of recreating the old data structures that were passed into
bids-validator/validators/events/hed.js
I tried to get everything in shape a bit closer to where the outgoing calls were made, not sure if I got it quite right. The contexts have the proper sidecar data in them already so no need to mess withpotentialSidecars
.hedValidate
is called once, after the context walk is complete.bids-validator/src/issues/list.ts
as needed.detectHed
function to skip calls when no HED data present.Currently am getting
"ERROR: [HED_SCHEMA_LOAD_FAILED] The supplied schema specification is invalid. Specification: no sche..."
when run onbids-examples/eeg_ds003645s_hed