ReproNim / neurodocker

Generate custom Docker and Singularity images, and minimize existing containers
https://www.repronim.org/neurodocker/
Apache License 2.0
325 stars 97 forks source link

FIX use --transform in freesurfer's template to generalize across freesurfer versions to strip leading `freesurfer/` folder #626

Closed mvdoc closed 2 weeks ago

mvdoc commented 1 month ago

I guess we'll have to check if the same problem occurs with new freesurfer versions.

Closes #625

yarikoptic commented 3 weeks ago

could you check newer ones or they aren't out? or may be --transform= which takes regex to change filepaths could be used for more flexibility?

mvdoc commented 3 weeks ago

Ah good ol' Yarik always teaches me something new! I switched to --transform and it works great

7.3.2

$ neurodocker generate docker --base-image ubuntu:20.04 --pkg-manager apt --freesurfer version=7.3.2 > test-dockerfile-7.3.2
$ docker build --file test-dockerfile-7.3.2 . --tag freesurfer-7.3.2
$ docker run --rm -it freesurfer-7.3.2:latest ls /opt/freesurfer-7.3.2 && cat /opt/freesurfer-7.3.2/b
uild-stamp.txt
ASegStatsLUT.txt               average          matlab
DefectLUT.txt                  bin              mni
FreeSurferColorLUT.txt         build-stamp.txt  mni-1.4
FreeSurferEnv.csh              diffusion        models
FreeSurferEnv.sh               docs             python
SegmentNoLUT.txt               etc              sessions
SetUpFreeSurfer.csh            freesurfer       sources.csh
SetUpFreeSurfer.sh             fsafd            sources.sh
Simple_surface_labels2009.txt  fsfast           subjects
SubCorticalMassLUT.txt         lib              tkmeditParcColorsCMA
WMParcStatsLUT.txt             luts             tktools

$ docker run --rm -it freesurfer-7.3.2:latest cat /opt/freesurfer-7.3.2/build-stamp.txt
freesurfer-linux-centos7_x86_64-7.3.2-20220804-6354275

7.4.1

$ neurodocker generate docker --base-image ubuntu:20.04 --pkg-manager apt --freesurfer version=7.4.1 > test-dockerfile-7.4.1
$ docker build --file test-dockerfile-7.4.1 . --tag freesurfer-7.4.1
$ docker run --rm -it freesurfer-7.4.1:latest ls /opt/freesurfer-7.4.1 
ASegStatsLUT.txt               average          matlab
DefectLUT.txt                  bin              mni
FreeSurferColorLUT.txt         build-stamp.txt  mni-1.4
FreeSurferEnv.csh              diffusion        models
FreeSurferEnv.sh               docs             python
SegmentNoLUT.txt               etc              sessions
SetUpFreeSurfer.csh            freesurfer       sources.csh
SetUpFreeSurfer.sh             fsafd            sources.sh
Simple_surface_labels2009.txt  fsfast           subjects
SubCorticalMassLUT.txt         lib              tkmeditParcColorsCMA
WMParcStatsLUT.txt             luts             tktools

$ docker run --rm -it freesurfer-7.4.1:latest cat /opt/freesurfer-7.4.1/build-stamp.txt
freesurfer-linux-centos7_x86_64-7.4.1-20230613-7eb8460
yarikoptic commented 2 weeks ago

Ah good ol' Yarik

That elderly learns something new each day too ;-) I eye balled failing tests and it is still only the two of test_build_image_from_registered on centos, so unrelated. Leaving a comment on the diff now... nah -- I think we are all good -- for better or for worse -- that --transform works great.

yarikoptic commented 2 weeks ago

interestingly, even in 7.3.2 there were some ./freesurfer entries!

$> tar -tvf freesurfer-linux-centos7_x86_64-7.3.2.tar | awk '{print $6;}' | sed -e 's,^\([^/]*/[^/]*\).*,\1,g' | uniq -c
      1 freesurfer/
     11 freesurfer/tktools
    784 freesurfer/fsfast
    474 freesurfer/mni-1.4
      1 freesurfer/FreeSurferEnv.csh
      1 freesurfer/FreeSurferEnv.sh
   1665 freesurfer/trctrain
      1 freesurfer/SetUpFreeSurfer.csh
    113 freesurfer/docs
     37 freesurfer/models
    450 freesurfer/lib
     94 freesurfer/diffusion
      1 freesurfer/SegmentNoLUT.txt
  42431 freesurfer/python
      1 freesurfer/build-stamp.txt
      4 freesurfer/fsafd
      1 freesurfer/sources.csh
      1 freesurfer/DefectLUT.txt
    478 freesurfer/mni
   1954 freesurfer/subjects
      1 freesurfer/FreeSurferColorLUT.txt
      1 freesurfer/ASegStatsLUT.txt
      1 freesurfer/SubCorticalMassLUT.txt
    795 freesurfer/bin
      1 freesurfer/Simple_surface_labels2009.txt
      1 freesurfer/tkmeditParcColorsCMA
    287 freesurfer/matlab
      1 freesurfer/WMParcStatsLUT.txt
      1 freesurfer/sources.sh
   7575 freesurfer/average
      2 freesurfer/etc
      1 freesurfer/SetUpFreeSurfer.sh
      5 freesurfer/luts
      2 freesurfer/sessions
    953 ./freesurfer
$> tar -tvf freesurfer-linux-centos7_x86_64-7.3.2.tar | awk '{print $6;}' | grep '^\./freesurfer' | head                        
./freesurfer/lib/qt/
./freesurfer/lib/qt/lib/
./freesurfer/lib/qt/lib/libQt5Network.so
./freesurfer/lib/qt/lib/libQt5Gamepad.so.5.12

I assume that when extracting without stripping it worked just fine. So could it be we might have fixed more than planned? do you have still those examples from prior version to see if lib/qt was extracted under freesurfer/lib/qt ?

mvdoc commented 2 weeks ago

I'm afraid you're correct, the transform now strips too much. Good catch.

With --strip-components=1:

$ docker run --rm -it freesurfer-7.3.2-strip:latest ls -la /opt/freesurfer-7.3.2/freesurfer/
total 12
drwxr-xr-x  3 root root 4096 Jul  6 20:17 .
drwxr-xr-x 20 root root 4096 Jul  6 20:17 ..
drwxr-xr-x  3 root root 4096 Jul  6 20:17 lib

But with the --transform it is now empty:

$ docker run --rm -it freesurfer-7.3.2:latest ls -la /opt/freesurfer-7.3.2/freesurfer/
total 8
drwxrwxr-x  2 root root 4096 Aug  4  2022 .
drwxr-xr-x 20 root root 4096 Jul  5 22:27 ..

However I don't know if those libs are necessary, since we already exclude lib/qt and lib/cuda from the extraction process. The only remaining libs are for tktools:

$ docker run --rm -it freesurfer-7.3.2-strip:latest ls -la /opt/freesurfer-7.3.2/freesurfer/lib/
total 12
drwxr-xr-x 3 root root 4096 Jul  6 20:17 .
drwxr-xr-x 3 root root 4096 Jul  6 20:17 ..
drwxrwxr-x 7 root root 4096 Mar 30  2020 tktools
yarikoptic commented 2 weeks ago

the transform now strips too much.

I would not say "now strips too much" but rather "now strips the correct amount" since without stripping there is no difference between freesurfer/ and ./freesurfer/, correct? And because you didn't use g modifier, it strips only leading freesurfer and not any other possible freesurfer subfolder, thus making it "correct". Or am I logic flawed?

yarikoptic commented 2 weeks ago

we only could have made it more precise may be anchoring like 's,^\(\./\)*freesurfer/,,' and more robust by adding version information, smth like -e 's,^\(\./\)*freesurfer\(-[^/]*\)/,,' in case if any release packages it that way. WDYT?