Bep031 review updates #7

rwblair · 2022-01-07T18:24:36Z

Few proposed changes to the bep031 PR.

For the error generated from missing the chunk matrix, I use the data file in the error instead of the json file returned by generateMergedSidecarDictWithPath. generateMergedSidecarDictWithPath only returned the last json file validated, where the value could be and could go in any inherited json file for the data file.

I tried to rewrite without using the new readBuffer function and only using readFile. Its possible, we can decode the string returned by readFile back into a buffer, but this makes our potential memory issues worse by making a new copy of the files contents.

@nellh what do you think about adding a flag on readFile to return a buffer? or would leaving readBuffer as is in place be better?

Leaving as WIP until @nellh gets a chance to review.

Let me know what yall think @etiennebergeron and @konantian.

nellh · 2022-01-07T21:34:50Z

@nellh what do you think about adding a flag on readFile to return a buffer? or would leaving readBuffer as is in place be better?

I think keeping it as readBuffer is better for now, this kind of readFile call I think is something we'll replace soon but with one use of it here - keeping them separate makes it clear which behavior is expected when refactoring it later.

nellh

Ran through this, tried breaking some of tests and that worked as expected. 👍

konantian · 2022-01-08T00:07:56Z

I do have concerns about the memory usage if the dataset is too large, but currently have no good ideas on how to optimize this yet. I suggest leaving readBuffer as it is, if the performance will be a big issue in the future, we can do more optimization on it then.

mariehbourget · 2022-01-10T15:49:30Z

Hi @rwblair and @nellh, thank you for making corrections and reviewing this PR!

I tried testing it but I get an ENOBUF error with both our examples datasets (micr_spim and micr_sem) on this branch (that I don't have on bep031).
I tried installing a fresh clone but have the same error.

(base) C:\Users\bourg\bids-validator_np>node bids-validator/bin/bids-validator --config.ignore=44 --config.ignore=99 C:\Users\bourg\bids-examples\micr_sem
<ref *1> Error: spawnSync C:\Users\bourg\bids-validator_np\node_modules\esbuild\esbuild.exe ENOBUFS
    at Object.spawnSync (node:internal/child_process:1083:20)
    at spawnSync (node:child_process:812:24)
    at Object.execFileSync (node:child_process:858:15)
    at runServiceSync (C:\Users\bourg\bids-validator_np\node_modules\esbuild\lib\main.js:1802:30)
    at buildSync (C:\Users\bourg\bids-validator_np\node_modules\esbuild\lib\main.js:1628:3)
    at _bundle (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\esbuild.ts:92:19)
    at transpile (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\esbuild.ts:120:12)
    at Module.mod._compile (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\hook.ts:53:40)
    at Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Object.Module._extensions.<computed> [as .js] (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\hook.ts:56:7) {
  errno: -4060,
  code: 'ENOBUFS',
  syscall: 'spawnSync C:\\Users\\bourg\\bids-validator_np\\node_modules\\esbuild\\esbuild.exe',
  path: 'C:\\Users\\bourg\\bids-validator_np\\node_modules\\esbuild\\esbuild.exe',
  spawnargs: [ '--service=0.11.23' ],
  error: [Circular *1],
  status: null,
  signal: 'SIGTERM',
  output: [
    null,
    <Buffer 07 00 00 00 30 2e 31 31 2e 32 33 f9 9e 08 01 01 00 00 00 06 03 00 00 00 06 00 00 00 65 72 72 6f 72 73 05 00 00 00 00 0b 00 00 00 6f 75 74 70 75 74 46 ... 16842702 more bytes>,
    <Buffer >
  ],
  pid: 13732,
  stdout: <Buffer 07 00 00 00 30 2e 31 31 2e 32 33 f9 9e 08 01 01 00 00 00 06 03 00 00 00 06 00 00 00 65 72 72 6f 72 73 05 00 00 00 00 0b 00 00 00 6f 75 74 70 75 74 46 ... 16842702 more bytes>,
  stderr: <Buffer >
}

nellh · 2022-01-11T04:43:00Z

Hi @rwblair and @nellh, thank you for making corrections and reviewing this PR!

I tried testing it but I get an ENOBUF error with both our examples datasets (micr_spim and micr_sem) on this branch (that I don't have on bep031). I tried installing a fresh clone but have the same error.

(base) C:\Users\bourg\bids-validator_np>node bids-validator/bin/bids-validator --config.ignore=44 --config.ignore=99 C:\Users\bourg\bids-examples\micr_sem
<ref *1> Error: spawnSync C:\Users\bourg\bids-validator_np\node_modules\esbuild\esbuild.exe ENOBUFS
    at Object.spawnSync (node:internal/child_process:1083:20)
    at spawnSync (node:child_process:812:24)
    at Object.execFileSync (node:child_process:858:15)
    at runServiceSync (C:\Users\bourg\bids-validator_np\node_modules\esbuild\lib\main.js:1802:30)
    at buildSync (C:\Users\bourg\bids-validator_np\node_modules\esbuild\lib\main.js:1628:3)
    at _bundle (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\esbuild.ts:92:19)
    at transpile (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\esbuild.ts:120:12)
    at Module.mod._compile (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\hook.ts:53:40)
    at Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Object.Module._extensions.<computed> [as .js] (C:\Users\bourg\bids-validator_np\node_modules\esbuild-runner\src\hook.ts:56:7) {
  errno: -4060,
  code: 'ENOBUFS',
  syscall: 'spawnSync C:\\Users\\bourg\\bids-validator_np\\node_modules\\esbuild\\esbuild.exe',
  path: 'C:\\Users\\bourg\\bids-validator_np\\node_modules\\esbuild\\esbuild.exe',
  spawnargs: [ '--service=0.11.23' ],
  error: [Circular *1],
  status: null,
  signal: 'SIGTERM',
  output: [
    null,
    <Buffer 07 00 00 00 30 2e 31 31 2e 32 33 f9 9e 08 01 01 00 00 00 06 03 00 00 00 06 00 00 00 65 72 72 6f 72 73 05 00 00 00 00 0b 00 00 00 6f 75 74 70 75 74 46 ... 16842702 more bytes>,
    <Buffer >
  ],
  pid: 13732,
  stdout: <Buffer 07 00 00 00 30 2e 31 31 2e 32 33 f9 9e 08 01 01 00 00 00 06 03 00 00 00 06 00 00 00 65 72 72 6f 72 73 05 00 00 00 00 0b 00 00 00 6f 75 74 70 75 74 46 ... 16842702 more bytes>,
  stderr: <Buffer >
}

This is the issue discussed in https://github.com/bids-standard/bids-validator/issues/1399 does the ESBUILD_MAX_BUFFER=33554432 workaround solve it for you?

konantian · 2022-01-11T06:08:56Z

Based on my test, if we put relocate two dependencies exifreader and xml2js from bids-validator/bids-validator/package.json to bids-validator/package.json, there will be no ENOBUF error anymore. However, when I run our validator against micr_SEM I got this error, but everything was fine with micr_SPIM

`
Also, I am not able to run our validator from the browsers as well.

mariehbourget

Thank you @nellh and @konantian.
I was able to run the validator without the ENOBUF error using either of your suggestions.

set ESBUILD_MAX_BUFFER=33554432
or
moving exifreader and xml2js from bids-validator/bids-validator/package.json to bids-validator/package.json

So I tested the branch, everything is working great with the micr_SPIM dataset, except the filename embedded in the ChunkTransformationMatrix warning.
Similarly, the error we get with the micr_SEM dataset seems also related to this error, but I don't understand why and why only for PNG files.

I have no experience in Javascript (only python) so I hope my comments make sense. Thank you for your help!

mariehbourget · 2022-01-11T15:00:36Z

bids-validator/validators/microscopy/checkJSONAndField.js

+        file: {
+          path: file,
+        },
+        code: 223,


Here, if "chunkTransformationMatrix" is missing, the path of the "file" does not appear, I get ".undefined" instead.
I don't understand why because if the JSON file is missing altogether (error 225 above), the path in the error is correct.
Ex:

It also seems to be related to the issue with the micr_SEM dataset. This function returns the issues [ undefined, undefined ] causing the "Unhandled rejection". (but only for PNG files?)

Good catch, I forgot that the result of the json check function was being directly concatenated to the issues list. If there were no problems it returned undefined by default. I added a base return with an empty list to handle this.

rwblair · 2022-01-11T22:52:27Z

The ENOBUF is related to esbuild-runner some how. If the exifreader and xml dependencies are in bids-validator/bids-validator/package.json, and the validator is called with bids-validator/bids-validator/ being the current working directory I don't get the error.

If while in bids-validator/bids-validator/ I call npm run build and then edit bids-validator/bids-validator/bin/bids-validator to only call the code in its catch statement, by passing esbuild-runner import, I can call the validator from any directory with no error.

So at least thats only an error for development, not sure how best to resolve that issue though. Would be nice to keep the dependency in just the validators package.json and be able to call the cli without having to rebuild every time (though esbuild is so fast maybe the call to the cli could just kick off the build process if it detects its in a dev environment?)

I'm not able to build the website currently with npm run web-build:

ModuleNotFoundError: Module not found: Error: Can't resolve '/home/rwblair/projects/bids-validator/bids-validator-web/node_modules/@babel/runtime/helpers/esm/classCallCheck' in '/home/rwblair/projects/bids-validator/bids-validator-web/components'


> Build error occurred
Error: > Build failed because of webpack errors
    at /home/rwblair/projects/bids-validator/node_modules/next/dist/build/index.js:397:19
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async Span.traceAsyncFn (/home/rwblair/projects/bids-validator/node_modules/next/dist/telemetry/trace/trace.js:60:20)
    at async Object.build [as default] (/home/rwblair/projects/bids-validator/node_modules/next/dist/build/index.js:77:25)

I'll update here when I find a fix for this as well.

nellh · 2022-01-11T23:06:20Z

Putting the dependencies in the root package will incidentally work since they exist in node_modules but it will fail when bundled (esbuild or webpack) because they are not included in the output and not present at runtime. The ENOBUFS error is caused when esbuild's output buffer for the full bundle (bids-validator and all deps) is exceeding the buffer size.

These dependencies aren't terribly large (about 150kB minified together), I think we can temporarily raise the buffer limit and try to solve this separately.

konantian · 2022-01-12T07:02:24Z

@rwblair and @nellh Thanks for your solutions and explanations, it works for me as well. I am able to run bids-validator without any errors on micr_SEM and micr_SPIM now.

mariehbourget

Thank you @rwblair and @nellh!
I tested the new version without errors on both our bids-examples datasets by running it from the bids-validator/bids-validator/ directory.

Let me know if you prefer we address the website and ENOBUF errors here, or in another PR.
Thanks!

…sbuild-runner

…eing wonky.

rwblair · 2022-01-12T21:01:37Z

@mariehbourget ENOBUF and web issue should fixed from my past couple of commits.

Feel free to merge and any further issues I can push directly to the main PR.

rwblair added 10 commits January 6, 2022 12:22

add tests for checkJSONAndField

4daf782

add tests for checkSample.js

8e23cba

add tests for microscopy validate.js and test data

7d1e4ad

add param docs to readFile function

fb5da0a

generate error if magic number is incorrect on tiff file

e7ebc94

move new dependencies into validator package.json

b920d2d

have micro json check use old sidecar generation function

7588c0e

Merge branch 'bep031' of github.com:neuropoly/bids-validator into HEAD

0b7fd4f

rearrange logic for samples.tsv validation. Add tests.

9e3cabe

updated package-lock to reflect package.json changes.

5fdb675

nellh approved these changes Jan 7, 2022

View reviewed changes

rwblair changed the title ~~[WIP] Bep031 review updates~~ Bep031 review updates Jan 7, 2022

mariehbourget reviewed Jan 11, 2022

View reviewed changes

have microscopy json check return empty list if no other issues found

5862244

mariehbourget approved these changes Jan 12, 2022

View reviewed changes

rwblair added 2 commits January 12, 2022 14:23

set esbuild buffer size env in bin/bids-validator when we are using e…

877e8c7

…sbuild-runner

added babel runtime to web package.json, maybe it was just my setup b…

d5ad033

…eing wonky.

mariehbourget merged commit bd78f58 into neuropoly:bep031 Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bep031 review updates #7

Bep031 review updates #7

rwblair commented Jan 7, 2022

nellh commented Jan 7, 2022

nellh left a comment

konantian commented Jan 8, 2022

mariehbourget commented Jan 10, 2022 •

edited

Loading

nellh commented Jan 11, 2022

konantian commented Jan 11, 2022

mariehbourget left a comment

mariehbourget Jan 11, 2022

mariehbourget Jan 11, 2022

rwblair Jan 11, 2022

rwblair commented Jan 11, 2022

nellh commented Jan 11, 2022 •

edited

Loading

konantian commented Jan 12, 2022

mariehbourget left a comment •

edited

Loading

rwblair commented Jan 12, 2022

Bep031 review updates #7

Bep031 review updates #7

Conversation

rwblair commented Jan 7, 2022

nellh commented Jan 7, 2022

nellh left a comment

Choose a reason for hiding this comment

konantian commented Jan 8, 2022

mariehbourget commented Jan 10, 2022 • edited Loading

nellh commented Jan 11, 2022

konantian commented Jan 11, 2022

mariehbourget left a comment

Choose a reason for hiding this comment

mariehbourget Jan 11, 2022

Choose a reason for hiding this comment

mariehbourget Jan 11, 2022

Choose a reason for hiding this comment

rwblair Jan 11, 2022

Choose a reason for hiding this comment

rwblair commented Jan 11, 2022

nellh commented Jan 11, 2022 • edited Loading

konantian commented Jan 12, 2022

mariehbourget left a comment • edited Loading

Choose a reason for hiding this comment

rwblair commented Jan 12, 2022

mariehbourget commented Jan 10, 2022 •

edited

Loading

nellh commented Jan 11, 2022 •

edited

Loading

mariehbourget left a comment •

edited

Loading