Skip to content
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

enh: docker support + basic usage doc #13

Closed
wants to merge 2 commits into from

Conversation

mgxd
Copy link

@mgxd mgxd commented Oct 24, 2018

Adds Dockerfile with support for NIFTI, CIFTI, GIFTI and example usage

Will close if #5 is merged, but it seems orphaned

@andersonwinkler
Copy link
Owner

There are two implementations of docker files for PALM, made by two different contributors. This by Joke is one. Then there is another. I can merge but then someone else would need offer support as I haven't tested either, nor do I use docker/singularity in general...

Dockerfile Outdated
WORKDIR fileio
RUN cd \@gifti/private \
&& mkoctfile --mex zstream.c \
&& mkoctfile --mex miniz.c \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mini.z is a single-file library that is included in zstream.c so it doesn't have to be compiled separately (and it does not contain a MEX interface).

@mgxd
Copy link
Author

mgxd commented Oct 25, 2018

Hi Anderson - I wanted to share the Dockerfile I used to help others trying to easily install/use this nice software. Sure, I can provide basic support if people run into issues with the image.

Ideally, we could also build this and host it on docker hub (where users would just be able to pull the image from the registry) - I'm currently hosting one at (mgxd/palm:a111).

@andersonwinkler
Copy link
Owner

Hi Mathias,

This is great, many thanks. May ask you a final favour before merging? We have now 3 docker files. They are similar but not the same. Any chance you could see what pieces from the other two may be absent from yours, then include them if you find these are needed?

The three are these:

I didn't want to undo their work, but at the same time, didn't want to have multiple Docker files that the users won't know which to use. What do you think?

Thanks!

Anderson

@mgxd
Copy link
Author

mgxd commented Oct 25, 2018

Hi Anderson - some differences I've observed between the Dockerfiles:

  • Louis' is more lightweight, but doesn't include hcp's workbench (so no CIFTI compatibility unless wb_command support is dropped as discussed in Limitations of CIFTI support? #12)
  • Joke's is a bit heavier since it includes FSL as well - I haven't had a need for it while running PALM but perhaps there are some options that depend on it? I'm fine with adding it but was trying to encapsulate an environment that fully supports PALM exclusively.

In the end, all these Dockerfiles achieve the same goal - providing users with a prebuilt recipe to easily use PALM.

@andersonwinkler
Copy link
Owner

andersonwinkler commented Oct 25, 2018

Thanks Mathias. FSL isn't needed. Connectome Workbench and FreeSurfer are only needed to read files that the user may have from these programs, but then I think it's fair to assume that the user will have these installed anyway in that case, so I think I'd drop these too.

As long as Octave works, along with signal and image toolboxes and their dependencies, plus we have what is needed to compile the .c files, we should be good. Thanks!

@mgxd
Copy link
Author

mgxd commented Oct 25, 2018

Ah, in that case we may want to consider include FreeSurfer as well - the goal of the container is to have all dependencies (and preferably use-cases) bundled within this execution environment.

@andersonwinkler
Copy link
Owner

However way you prefer and will facilitate the life for Docker users...

@mgxd
Copy link
Author

mgxd commented Oct 31, 2018

@andersonwinkler After thinking on this a little more, would it be possible now to phase out the need for FreeSurfer with the update in d3afc0b? (@gllmflndn does @-gifti now support r/w of FS surfs and curvs?) If so, it would be great for reducing the size of this container!

@gllmflndn
Copy link
Contributor

@mgxd reading FS files should be supported, see:
https://github.com/gllmflndn/gifti/blob/master/%40gifti/private/read_freesurfer_file.m
If you have files that cannot be read, please give me access to an example file so that I can look at it.
I haven't implemented writing FS files as I was primarily interested in importing data into @gifti but this shouldn't be difficult to add if needs be.

@mgxd
Copy link
Author

mgxd commented Oct 31, 2018

@gllmflndn - amazing! I believe these lines are the specific files currently written with FS - there is no necessity to replace but would be nice to knock out a (~ 6GB) dependency.

@gllmflndn
Copy link
Contributor

@mgxd I have added export to FS .curv and .surf in @gifti: see gllmflndn/gifti@018de5e
Are the other options, fs_load_nifti and fs_load_mgh, actually used in PALM?

@neurolabusc
Copy link

neurolabusc commented Nov 1, 2018

@gllmflndn - I note you added mz3 reading. Would it be possible to also include mz3 write - Matlab code is here. I am biased as it is the internal format of Surfice, but this is a compact, simple, and fast format. While some might argue this is yet another format, this format is structured just like OpenGL/Metal/DirectX/Vulkan indexed triangles. So the format is defined by the hardware providing very good performance. The full format specification is here which includes a Python reader (in the form of a Blender plugin). Thanks for your great work.

@gllmflndn
Copy link
Contributor

@neurolabusc - ok, it might require more testing but I've added it now. It will be in the next release of SPM12 and I will update @gifti at that time.

@andersonwinkler
Copy link
Owner

andersonwinkler commented Nov 1, 2018

This is great!

Are the other options, fs_load_nifti and fs_load_mgh, actually used in PALM?

Yes... particularly for .mgh/.mgz files. The outputs of the FreeSurfer's command mris_preproc are in .mgh format, and these are often the inputs to PALM for people using FS. For NIFTI, it is the next in line if @nifti cannot be used for some reason.

However, including ~6 GB additional files to a container just because of a few lines of code don't seem reasonable. If Guillaume could integrate .mgz/.mgh into @nifti (these are volume formats, one the gzipped version of the other) it would be awesome. Alternatively, I can write small functions to do that.

@gllmflndn
Copy link
Contributor

Weirdly enough, I seem to have implemented reading from .mgh files in @gifti:
https://github.com/gllmflndn/gifti/blob/master/%40gifti/private/read_freesurfer_file.m#L46-L108
I could update it to also handle .mgz and have optional memory-mapping instead of loading the entire data into memory.
While it seems to make a lot more sense in the context of @nifti than @gifti (or can it also be used to store surface textures?), I would prefer to keep @nifti related to NIfTI (and Analyze) only so moving it as a small standalone function would be preferable here.

@hajernakua
Copy link

hajernakua commented Jun 25, 2019

Hi @andersonwinkler + @mgxd, I'm using a singularity container of PALM pulled from here and when I try to run it, I keep getting these errors:

_X11 connection rejected because of wrong authentication.
octave: unable to open X11 DISPLAY
octave: disabling GUI features
X11 connection rejected because of wrong authentication.
X11 connection rejected because of wrong authentication.

_

&

_Error using palm_configrw (/opt/palm/palm_configrw.m:61)
fprintf: invalid stream number = -1

Error in palm_takeargs (/opt/palm/palm_takeargs.m:54->palm_configrw)
Error in palm_core (/opt/palm/palm_core.m:33->palm_takeargs)
Error in palm (/opt/palm/palm.m:81->palm_core)_

I'm uncertain if this is a problem with my inputs, or perhaps design or contrast matrices or with the container itself. I am using a design and contrast matrix that worked when I did not run it in a singularity container so I would imagine they should be okay. Thank you very much!

This is the PALM function:

palm_command1() {
singularity run
-H ${dir}
-B ${SUBJECT_FILES}/${exampleSubid}/MNINonLinear/fsaverage_LR32k:/subfiles
-B ${design_matrix1}:/design
-B ${contrast_matrix1}:/contrast
-B ${fname}_L.func.gii:/fname
${palm_container} -i /fname -d /design -t /contrast -o results_L_cort -T -tfce2D -s /subfiles/${exampleSubid}.L.midthickness.32k_fs_LR.surf.gii L_area.func.gii -logp -n 2000
}

@andersonwinkler
Copy link
Owner

Hi @hajernakua,

Can you try outside a container? It's hard to tell otherwise if the problem is with the container or with PALM itself.

Seems you don't have access to a graphical mode, but PALM doesn't need one. Perhaps if you edit the 'palm' script so as to include the option --no-gui?

All the best,

Anderson

@hajernakua
Copy link

hajernakua commented Jul 2, 2019

Thank you for the reply @andersonwinkler (@mgxd) ,

I moved all my data to a computing cluster which doesn't have PALM and that's why I would need to run it in a container. When I ran palm prior to moving my data, it worked fine, but I'm just adjusting the commands and scripts to this container.

I found previous error that was similar to mine, and made some script adjustments, and this is my first command:

palm_command1() {
singularity run
-H /scratch/a/arisvoin/nakuah/sing_container
-B ${dir}:/dir
-B ${SUBJECT_FILES}:/subfiles
${palm_container} -i /dir/allsubs_merged_L.func.gii -d /dir/DesignMatrix.csv -t /dir/ContrastMatrix.csv -o /dir/results_L_cort -T -tfce2D -s /subfiles/${exampleSubid}/MNINonLinear/fsaverage_LR32k/${exampleSubid}.L.midthickness.32k_fs_LR.surf.gii /dir/L_area.func.gii -logp -n 100
}

However, I'm still getting this error:

Running PALM alpha115 using Octave 4.0.3 with the following options:
-i /dir/allsubs_merged_L.func.gii
-d /dir/DesignMatrix.csv
-t /dir/ContrastMatrix.csv
-o /dir/results_L_cort
-T
-tfce2D
-s /subfiles/sub-1050004/MNINonLinear/fsaverage_LR32k/sub-1050004.L.midthickness.32k_fs_LR.surf.gii /dir/L_area.func.gii
-logp
-n 100
Found HCP Workbench executable in /opt/workbench/bin_linux64/wb_command
Loading surface 1/1: /subfiles/sub-1050004/MNINonLinear/fsaverage_LR32k/sub-1050004.L.midthickness.32k_fs_LR.surf.gii
_**Error using read_gifti_file (/opt/palm-alpha115/fileio/@gifti/private/read_gifti_file.m:17)
[GIFTI] Loading of XML file /subfiles/sub-1050004/MNINonLinear/fsaverage_LR32k/sub-1050004.L.midthickness.32k_fs_LR.surf.gii failed.

Error in gifti (/opt/palm-alpha115/fileio/@gifti/gifti.m:89->read_gifti_file)
Error in palm_miscread (/opt/palm-alpha115/palm_miscread.m:284->gifti)
Error in palm_takeargs (/opt/palm-alpha115/palm_takeargs.m:1565->palm_miscread)
Error in palm_core (/opt/palm-alpha115/palm_core.m:33->palm_takeargs)
Error in palm (/opt/palm-alpha115/palm.m:81->palm_core)**_

Would you know a way to help solve this problem?

@andersonwinkler
Copy link
Owner

Hi

Two possibilities:

  1. You may need to compile the .c files. See the "compile.sh" scripts in the "private" directories inside "fileio" (just search for "compile.sh" in the tree). The compilation needs only that you have the "mkoctfile" so it should be straightforward.

  2. If after doing the above the problem persists, then it could be due to this bug: https://savannah.gnu.org/bugs/?53856, which was fixed in Octave 4.4.1. Guillaume (@gllmflndn) may be able to tell better whether that is the case.

In any case, PALM is meant to depend only on Octave, so I really don't see much benefit in using a container. I wouldn't use and wouldn't recommend (and wouldn't recommend containers in general...).

Cheers,

Anderson

@gllmflndn
Copy link
Contributor

To get a more detailed error message, you could remove the try/catch here:
https://github.com/andersonwinkler/PALM/blob/master/fileio/%40gifti/private/read_gifti_file.m#L14-L18
I have now made that change in @gifti so this will be available in the next release.

The MEX files should have been compiled through the instructions within the Dockerfile so your issue could indeed be due to a fixed bug from an old Octave version, or that the GIfTI file could not be accessed at all (e.g. a mistake in the singularity bind points).

@mgxd mgxd closed this Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants