[Feature]: avoid demanding users to `generateCore` and `generateExtension` manually

yarikoptic commented 1 year ago

What would you like to see added to MatNWB?

I have initially expressed that opinion/desire in https://github.com/NeurodataWithoutBorders/matnwb/issues/491#issuecomment-1429078308 where we are trying to ping point/resolve an issue forbidding opening an nwb file.

Documentation ATM in https://github.com/NeurodataWithoutBorders/matnwb#step-2-generate-the-api mandates users to call generateCore() to produce some extra files. On one hand it is ok as for instructions "how to install from source" which is the case in that description based on git clone of the repository.

But then

in the next Optional section https://github.com/NeurodataWithoutBorders/matnwb#optional-generate-matnwb-code-for-extensions instructions talk about generating namespaces using generateExtension. As a user, who is just given an .nwb file to open, it sounds unclear (do I have to or not to open the file) and uncertain how (where to get those namespaces files) etc. Ideally matnwb should be able to pull necessary namespaces/files and call generateExtension if that is necessary as determined while opening some .nwb file
Further section https://github.com/NeurodataWithoutBorders/matnwb#optional-generate-an-older-core-schema opens possibility to generate schemas for older (although example does not specify any specific version so not clear how -- update schema submodule to older version? just give a version in invocation somehow?)

Is your feature request related to a problem?

Unable to open .nwb file(s) as encountered in #491 etc

What solution would you like?

For the "core" schema, I guess it would be possible to just ship "pre-generated" ones from this repo but it would not work for extensions generally. So, I think matnwb should, as needed, generate* whatever is needed to open a file and store those namespaces in some versioned (by schema, extension, and/or matnwb) cached location, e.g. ~/.cache/matnwb/ and not demand user to do anything "manually".

Do you have any interest in helping implement the feature?

No.

Code of Conduct

[X] I agree to follow this project's Code of Conduct
[X] Have you ensured this change was not already requested?

lawrence-mbf commented 1 year ago

To clarify (from #491 discussion):

The scripts generateCore, generateExtension, and nwbRead should be clarified as to their uses:

For reading, nwbRead should be all you need. Class files are generated from the embedded spec by default without any extra input from the user.
For writing, it is advised to use generateCore and generateExtension to generate specific versions of the core schema as needed. You then export this environment with nwbExport.

Both of these use the same cache location <install location>/+types which is prone to being overwritten but this isn't a big deal unless you opt to use more advanced options (i.e. savedir and friends) with these scripts.

lawrence-mbf commented 1 year ago

Minor clarification on writes:

When you nwbExport (write) a NWB file, the entire cache location <install location/+types is exported as an embedded specification.

yarikoptic commented 1 year ago

how do you envision "system-wide" installation of matnwb on a shared user system (e.g. to accompany an installation of matlab with its toolbox), where some users might want, some - write, versions of nwb files can vary?

should generateCore be ran by admins upon initial installation?
should they generateExtension for some known extensions right away?

lawrence-mbf commented 1 year ago

The main issue with system-wide installations would inevitably be MATLAB Path wrangling since you need some way to switch between different generated classes eventually.

If we must insist on a "system-wide" installation then I would advise preventing writes inside the matnwb/+types directory and insist on savedir everywhere. Then any generateCore or generateExtension calls must be handled by each user.

I think at some point the class files were automatically saved in the working directory but I forget why we changed that. Would definitely be preferable in this case.

yarikoptic commented 1 year ago

For reading, nwbRead should be all you need. Class files are generated from the embedded spec by default without any extra input from the user.

ok, I have tried that on the first sample (listed failing first in https://github.com/dandi/dandisets-healthstatus) file and it failed to load:

(venv) (base) dandi@drogon:~/cronlib/dandisets-healthstatus$ matnwb=$PWD/matnwb-master ; git -C $matnwb describe --tags;  git -C $matnwb clean -dfx; MATLABPATH=$matnwb time matlab -nodesktop -batch "f='$f'; disp(util.getSchemaVersion(f)); nwb = nwbRead(f)" && echo "All good!" || echo "Exited with $?"; git -C $matnwb clean -dfx
v2.6.0.1-2-gda49922
2.0b
Unable to resolve the name 'types.core.TimeSeries'.

Error in io.parseGroup (line 85)
    parsed = eval([Type.typename '(kwargs{:})']);

Error in io.parseGroup (line 38)
    subg = io.parseGroup(filename, group, Blacklist);

Error in io.parseGroup (line 38)
    subg = io.parseGroup(filename, group, Blacklist);

Error in nwbRead (line 59)
nwb = io.parseGroup(filename, h5info(filename), Blacklist);

Command exited with non-zero status 1
14.28user 1.48system 1:27.68elapsed 17%CPU (0avgtext+0avgdata 955596maxresident)k
691032inputs+480outputs (732major+207636minor)pagefaults 0swaps
Exited with 1
(venv) (base) dandi@drogon:~/cronlib/dandisets-healthstatus$ echo $f
/mnt/backup/dandi/dandisets-healthstatus/dandisets-fuse/000003/sub-YutaMouse33/sub-YutaMouse33_ses-YutaMouse33-150218_behavior+ecephys.nwb

although seeing version 2.0b for that file gives me concern -- may be that outdated "beta" one is the reason?

meanwhile I have tried on f=/mnt/backup/dandi/dandisets-healthstatus/dandisets-fuse/000405/sub-s0/sub-s0_ses-Li-OF-052818.nwb and there it worked out fine.

Overall -- I think I will adjust the test to indeed just do plain nwbRead for now and see where we would get.

lawrence-mbf commented 1 year ago

Sorry for taking so long to respond.

although seeing version 2.0b for that file gives me concern -- may be that outdated "beta" one is the reason?

Yes, that was some schema history that MatNWB no longer supports.

ehennestad commented 2 weeks ago

@yarikoptic I think this issue is partly or fully addressed by PR #556.

With regards to this comment:

how do you envision "system-wide" installation of matnwb on a shared user system (e.g. to accompany an installation of matlab with its toolbox), where some users might want, some - write, versions of nwb files can vary?

should generateCore be ran by admins upon initial installation? should they generateExtension for some known extensions right away?

The above PR does not solve for that scenario. At the moment, on a shared used system where multiple users might want to write NWB files using different NWB versions, some amount of training/instruction on how to use generate* functions might be the best way to make that work.

I will close this issue, but if you still think there are unresolved elements here, please re-open the issue and we can continue the discussion.

(I know for example that the current way schema classes are generated can lead to issues when using nwbRead in a parallelised manner, as you have done in the dandi-healthstatus tests)

NeurodataWithoutBorders / matnwb