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

Fixes to containers FSL dependency #2929

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Lestropie
Copy link
Member

Closes #2921.

See also MRtrix3/containers#26, which includes changes necessary to produce the minified FSL dependency that this change then downloads from OSF.

While the container minification process intended to capture EddyQC's eddy_quad, the fact that it was not being properly executed was missed because dwifslpreproc does not return non-zero if it fails to execute.

Unfortunately I couldn't easily resolve just this one thing. I've ended up having to update the FSL version as well. Which is not fantastic for a patch MRtrix update, since there's some chance of a change in behaviour out of our control.

Regarding #2921, if this change is deemed acceptable I can push it to DockerHub as latest, which would make it easy for people to access immediately but without necessitating an MRtrix patch update.

- Minified FSL download updated to FSL version 6.0.7.7.
- Add ${FSLDIR}/share/fsl/bin to PATH rather than ${FSLDIR}/bin.
- Container minification process now appropriately captures eddy_quad command, as well as the softlink selected by its shebang.
@Lestropie Lestropie added bug container Docker or Singularity containers labels Jun 19, 2024
@Lestropie Lestropie requested a review from a team June 19, 2024 13:00
@Lestropie Lestropie self-assigned this Jun 19, 2024
@Lestropie
Copy link
Member Author

@anaharrismatnez @jhuguetn: If you want to try mrtrix3/mrtrix3:eddyqcfix on DockerHub, that should allow dwifslpreproc to run eddy_quad.

@jhuguetn
Copy link

That's great, thanks @Lestropie! We will and will give you some feedback.
One single doubt after quickly reviewing the changes proposed in both ends (the image for building the minified FSL package and the main MRtrix3 image), in the former you used ANTs 2.5.2 while in the later the version of ANTs installed is the same as it was previously, release 2.3.4.
Could that have any undesired effect on the usage of the final MRtrix3 image that you might think of?

@Lestropie
Copy link
Member Author

The minification container is now pulling a newer version of ANTs because the existing version was failing to compile (don't recall the specifics, but every time I tried to change one thing to get minification to work something else broke). However that change was only made for the sake of allowing me to re-run the minification process for FSL. That updated ANTs version has not been uploaded to OSF, and the MRtrix3 container continues to download the same minified version of ANTs as the 3.0.4 version of the container. That's intentional; I'd rather put off an update to a dependency until a minor version update.

@anaharrismatnez
Copy link

Thanks @Lestropie, we were able to run dwifslpreproc with eddy_quad perfectly using mrtrix3/mrtrix3:eddyqcfix.
The only thing is that every time I run a command I get the following message any idea why it could be?
/bin/bash: /opt/fsl/lib/libtinfo.so.6: no version information available (required by /bin/bash)

@daljit46
Copy link
Member

Most likely, that warning is due to the issue mentioned in this post.

@Lestropie
Copy link
Member Author

Lestropie commented Jun 24, 2024

  • Resolve libtinfo.so warning before merging
    I saw this myself also, just forgot to list it here, and prioritised getting something functional for you.

@Lestropie Lestropie marked this pull request as draft June 24, 2024 01:11
Should prevent appearance of error:
/bin/bash: /opt/fsl/lib/libtinfo.so.6: no version information available (required by /bin/bash)
@Lestropie Lestropie marked this pull request as ready for review June 24, 2024 01:52
@Lestropie Lestropie added this to the 3.0.5 updates milestone Sep 16, 2024
@Lestropie
Copy link
Member Author

@MRtrix3/mrtrix3-devs I would like to get this merged to clarify #3039.

Here is a terminal dump showing rectification of #2921; the output files in the last line would not be present if eddy_quad was not executed successfully:

[12:56 PM]-[robertes@9520l-003953-l]-[~/src/master]- |master U:1 ?:5 ✗|
$ git checkout container_eddyqc_fix
M	testing/binaries/data
Switched to branch 'container_eddyqc_fix'
Your branch is up to date with 'origin/container_eddyqc_fix'.

[12:56 PM]-[robertes@9520l-003953-l]-[~/src/master]- |container_eddyqc_fix U:1 ?:5 ✗|
$ docker build . -t mrtrix3:eddyqcfix
[+] Building 2453.4s (28/28) FINISHED                                                                  docker:default
...
 => => naming to docker.io/library/mrtrix3:eddyqcfix                                                             0.0s

[01:37 PM]-[robertes@9520l-003953-l]-[~/src/master]- |container_eddyqc_fix U:2 ?:5 ✗|
$ cd ~/data/BATMAN/Temp/

[02:40 PM]-[robertes@9520l-003953-l]-[~/.../BATMAN/Temp]-
$ docker run -it --rm -v $(pwd):/data mrtrix3:eddyqcfix dwifslpreproc /data/dwi_den_unr.mif /data/testeddyqc.mif -pe_dir ap -rpe_none -eddyqc_all /data/testeddyqc/
dwifslpreproc: 
dwifslpreproc: Note that this script makes use of commands / algorithms that have relevant articles for citation; INCLUDING FROM EXTERNAL SOFTWARE PACKAGES. Please consult the help page (-help option) for more information.
dwifslpreproc: 
dwifslpreproc: Generated scratch directory: /dwifslpreproc-tmp-V5EAJ1/
Command:  mrconvert /data/dwi_den_unr.mif /dwifslpreproc-tmp-V5EAJ1/dwi.mif -json_export /dwifslpreproc-tmp-V5EAJ1/dwi.json
dwifslpreproc: Changing to scratch directory (/dwifslpreproc-tmp-V5EAJ1/)
Command:  dirstat dwi.mif -output asym
dwifslpreproc: [WARNING] sampling of b=1000 shell is moderately asymmetric; distortion correction may benefit from use of: -eddy_options " ... --slm=linear ... "
dwifslpreproc: [WARNING] sampling of b=3000 shell is moderately asymmetric; distortion correction may benefit from use of: -eddy_options " ... --slm=linear ... "
Command:  mrinfo dwi.mif -export_grad_mrtrix grad.b
Command:  dwi2mask dwi.mif - | maskfilter - dilate - | mrconvert - eddy_mask.nii -datatype float32 -strides -1,+2,+3
Command:  mrconvert dwi.mif eddy_in.nii -strides -1,+2,+3,+4 -export_grad_fsl bvecs bvals -export_pe_eddy eddy_config.txt eddy_indices.txt
Command:  eddy_cuda10.2 --imain=eddy_in.nii --mask=eddy_mask.nii --acqp=eddy_config.txt --index=eddy_indices.txt --bvecs=bvecs --bvals=bvals --out=dwi_post_eddy --verbose
dwifslpreproc: CUDA version of 'eddy' was not successful; attempting OpenMP version
Command:  eddy_cpu --imain=eddy_in.nii --mask=eddy_mask.nii --acqp=eddy_config.txt --index=eddy_indices.txt --bvecs=bvecs --bvals=bvals --out=dwi_post_eddy --verbose
Command:  eddy_quad dwi_post_eddy -idx eddy_indices.txt -par eddy_config.txt -b bvals -m eddy_mask.nii
Command:  mrconvert dwi_post_eddy.nii.gz result.mif -strides -2,-3,-4,1 -fslgrad dwi_post_eddy.eddy_rotated_bvecs bvals
Function: os.makedirs('/data/testeddyqc')
Function: shutil.copy('dwi_post_eddy.eddy_parameters', '/data/testeddyqc/eddy_parameters')
Function: shutil.copy('dwi_post_eddy.eddy_movement_rms', '/data/testeddyqc/eddy_movement_rms')
Function: shutil.copy('dwi_post_eddy.eddy_restricted_movement_rms', '/data/testeddyqc/eddy_restricted_movement_rms')
Function: shutil.copy('dwi_post_eddy.eddy_post_eddy_shell_alignment_parameters', '/data/testeddyqc/eddy_post_eddy_shell_alignment_parameters')
Function: shutil.copy('dwi_post_eddy.eddy_post_eddy_shell_PE_translation_parameters', '/data/testeddyqc/eddy_post_eddy_shell_PE_translation_parameters')
Function: shutil.copy('dwi_post_eddy.eddy_outlier_report', '/data/testeddyqc/eddy_outlier_report')
Function: shutil.copy('dwi_post_eddy.eddy_outlier_map', '/data/testeddyqc/eddy_outlier_map')
Function: shutil.copy('dwi_post_eddy.eddy_outlier_n_stdev_map', '/data/testeddyqc/eddy_outlier_n_stdev_map')
Function: shutil.copy('dwi_post_eddy.eddy_outlier_n_sqr_stdev_map', '/data/testeddyqc/eddy_outlier_n_sqr_stdev_map')
Function: shutil.copytree('dwi_post_eddy.qc', '/data/testeddyqc/quad')
Function: shutil.copy('eddy_mask.nii', '/data/testeddyqc/eddy_mask.nii')
Command:  mrconvert result.mif /data/testeddyqc.mif
dwifslpreproc: Changing back to original directory (/)
dwifslpreproc: Deleting scratch directory (/dwifslpreproc-tmp-V5EAJ1/)

[02:55 PM]-[robertes@9520l-003953-l]-[~/.../BATMAN/Temp]-
$ ls testeddyqc/quad/
avg_b0.png  avg_b1999.png  avg_b3000.png  avg_b999.png  qc.json  qc.pdf  ref_list.png  ref.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug container Docker or Singularity containers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dwifslpreproc: Cannot run eddy_quad subcommand via MRtrix3 Docker image
4 participants