ExploreASL BIDS tool

[dariusvalevicius]

BIDS Subject version of ExploreASL, cuts down on many confusing input options and forces input and output types.

[prioux]

• Why is this needed when we already have the other explore ASL program, exactly?
• Why is the tool called "ExploreASL BIDS" with a space in the name?
• Why does the container always run as root instead of a non privileged user? That allows a bad actor to compromise the system its runs on, or a clumsy user to destroy the operating system
• Your python script always succeeds no matter what happens, so tasks will never fail in CBRAIN, even if users give total junk to to program.

[dariusvalevicius]

For points (1) and (2): This version disables the DICOM input option and is optimized for BIDS datasets, and basically cuts down on ~8-12 parameters the user otherwise has to set in the correct combination. This is because of xASL basically coming with its own dicom-to-bids module which is optional and difficult to configure. We'd prefer users to perform conversion offline or use a separate DICOM-to-BIDS tool if we provide one.

We could instead update the existing xASL tool to host only this BIDS version?

For (3), I'm not certain where this container relies on/runs root, could you pinpoint that for me?

For (4), I just updated the wrapper+container to raise an exception if xASL terminates with an error code of 1.

[prioux]

Ace-vh-2 home ~ > docker run -it dvalev/xasl_cbrain:1.2.0        
root@83e0ef603a1e:/# id
uid=0(root) gid=0(root) groups=0(root)

[prioux]

I looked at your changes. Why raise an exception and make the user face a ugly stack trace instead of returning a proper error code? Use the standard conventions, please, 0 fo success, 1 for warning, 2 for errors. You check these very things from the return of subprocess.call(), so the least you can do is pass it along to your own caller.

[dariusvalevicius]

I added a couple lines to the dockerfile to run the container as a user and improved the error piping - the exit code and error message of the MATLAB application should be printed in stdout and stderr on a crash.

[natacha-beck]

@prioux, @dariusvalevicius what is your point of view about this point:

We could instead update the existing xASL tool to host only this BIDS version?

I personally prefer an update of the xASL, the version we have in CBRAIN is restricted to a sub-group of tester. So if the idea is to only use the new version, I do not see the point to have multiple version if one will not be used.

[dariusvalevicius]

I think that should work. In general we can make a point of encouraging dicom-to-bids conversion before uploading data for processing, it's a little unnecessary that ExploreASL has this module at all when tools exist for this purpose.

[dariusvalevicius]

I believe I changed it to string because the "value disables" option will not work with integers (just tested it). It actually does validate and run properly with type set to "number", but I changed it to "string" in the above commit for consistency.

For the naming, I am leaving it alone for the moment since it is quite a hassle to rename as I understand? Maybe we can test it online first (Safa has been tasked with doing that) and then perform the renaming later.

[prioux]

Well, I assume this isn't ready yet and there are still tests to run?

[dariusvalevicius]

Natacha's last comment (which isn't showing up for me):

You should be able to remove the whole session about dataset_description since the BoutiquesBidsSingleSubjectMaker submodule will already add some field that allow the user to select specific dataset_description.json file and will handle the copy of it at the appropriate place.

Will work on it now, although I think this means the tool will not be runnable locally using bosh exec.

[dariusvalevicius]

@natacha-beck I removed the argument from the descriptor - the wrapper code should be robust to this and read the one produced by the CBRAIN module first.

However, again the inconvenience is that the tool is not runnable via bosh exec on a local machine, unless there is a way to specify the CBRAIN module arguments in an invocation file?

One fix would be to auto-generate a dataset_description if none is input, similar to the CBRAIN module, but inside the wrapper, just so I can test it locally. Or is this a bit redundant?

[prioux]

I don't understand your argument, Darius. If you use bosh on a local machine, you have control of the file system including the files used by the tool. You need a dataset_description? You just cp it in place, no?

[prioux]

Okay.

As a side note, this brings to mind what name we'd like to give this tool, going forward. I say we put 'BIDS' in the name, but not BIDS Subject. The reason being, as a standalone descriptor, you give the tool a BIDS datasets, and within CBRAIN it's transformed to work on a BIDS subject.