[ENH] New FreeSurfer's license mechanism #787

oesteban · 2017-10-24T19:05:40Z

Enables the online provision of a local license file

effigies · 2017-10-24T19:13:29Z

For fmriprep-docker, I would read the user's environment and automatically mount $FS_LICENSE if it exists.

effigies

Couple comments, nothing major.

effigies · 2017-10-24T19:34:58Z

wrapper/fmriprep_docker.py

@@ -245,11 +246,21 @@ def main():
    g_dev.add_argument('-p', '--patch-nipype', metavar='PATH',
                       type=os.path.abspath,
                       help='working nipype repository')
+    g_dev.add_argument('--fs-license', metavar='PATH', type=os.path.abspath,
+                       default=os.getenv('FS_LICENSE', None),
+                       help='folder containing FreeSurfer\'s license.txt file')


While it's a little bit neither here nor there, I think this is more of a "wrapper" option than a "dev" option.

effigies · 2017-10-24T19:35:37Z

.circle/tests.sh

@@ -39,14 +39,14 @@ case ${CIRCLE_NODE_INDEX} in
        | tee $HOME/docs/builddocs.log
    cat $HOME/docs/builddocs.log && if grep -q "ERROR" $HOME/docs/builddocs.log; then false; else true; fi
    fmriprep-docker -i poldracklab/fmriprep:latest --help
-    fmriprep-docker -i poldracklab/fmriprep:latest --config $HOME/nipype.cfg -w $HOME/ds054/scratch $HOME/data/ds054 $HOME/ds054/out participant --no-freesurfer --debug --write-graph --force-syn
+    fmriprep-docker --fs-license /tmp/fslicense -i poldracklab/fmriprep:latest --config $HOME/nipype.cfg -w $HOME/ds054/scratch $HOME/data/ds054 $HOME/ds054/out participant --no-freesurfer --debug --write-graph --force-syn


This is a --no-freesurfer run, so it's not really testing anything except that fmriprep-docker doesn't fail when it receives the flag.

Yep, I'm aware.

Can we call it --fs-license-file?

effigies · 2017-10-24T19:36:43Z

.circle/tests.sh

    # Place mock crash log and rebuild report
    UUID="$(date '+%Y%m%d-%H%M%S_')$(uuidgen)"
    mkdir -p $HOME/ds054/out/fmriprep/sub-100185/log/$UUID/
    cp fmriprep/data/tests/crash_files/*.txt $HOME/ds054/out/fmriprep/sub-100185/log/$UUID/
    # Expect one error
    set +e
-    fmriprep-docker -i poldracklab/fmriprep:latest --config $HOME/nipype.cfg -w $HOME/ds054/scratch $HOME/data/ds054 $HOME/ds054/out participant --no-freesurfer --debug --write-graph --force-syn --reports-only --run-uuid $UUID
+    fmriprep-docker --fs-license /tmp/fslicense -i poldracklab/fmriprep:latest --config $HOME/nipype.cfg -w $HOME/ds054/scratch $HOME/data/ds054 $HOME/ds054/out participant --no-freesurfer --debug --write-graph --force-syn --reports-only --run-uuid $UUID


Also --no-freesurfer.

effigies · 2017-10-25T14:21:47Z

Sorry, thought I was done commenting, but it would be helpful to check for the license file at the beginning, ourselves, rather than just letting ReconAll crash.

In run.py, you can add the following check (or similar):

default_license = op.join(os.getenv('FREESURFER_HOME', ''), 'license.txt')
license_file = os.getenv('FS_LICENSE', default_license)
if opts.freesurfer and not os.path.exists(license_file):
    raise RuntimeError(...)

chrisgorgo

I find it confusing that --fs-license-file is a command line argument for fmriprep-docker but not for native fmriprep. I think it would be better to have it in both places.

chrisgorgo · 2017-10-27T01:16:06Z

docs/installation.rst

+        poldracklab/fmriprep:latest \
+        /data /out/out \
+        participant \
+        --ignore fieldmaps


Shouldn't this be call include --fs-license-file?

chrisgorgo · 2017-10-27T01:16:39Z

docs/installation.rst

+
+It is also possible to run ``fmriprep-docker`` with FreeSurfer processing: ::
+
+    $ fmriprep-docker --fs-license $HOME/.licenses/freesurfer /path/to/data/dir /path/to/output/dir participant


--fs-license-file?

chrisgorgo · 2017-10-27T01:18:59Z

wrapper/fmriprep_docker.py

@@ -231,6 +232,9 @@ def main():
                        type=os.path.abspath,
                        help='Grid reference image for resampling BOLD files to volume template '
                             'space.')
+    g_wrap.add_argument('--fs-license-file', metavar='PATH', type=os.path.abspath,
+                        default=os.getenv('FS_LICENSE', None),
+                        help='folder containing FreeSurfer\'s license.txt file')


help="Path to FreeSurfer license key file. To obtain it you need to register (for free) at https://surfer.nmr.mgh.harvard.edu/registration.html"

chrisgorgo · 2017-10-27T01:21:01Z

circle.yml

@@ -20,6 +21,7 @@ dependencies:
    - mkdir -p $HOME/ds005/out && sudo setfacl -R -d -m group:ubuntu:rwx $HOME/ds005 && sudo setfacl -R -m group:ubuntu:rwx $HOME/ds005
    - mkdir -p $HOME/ds054 && sudo setfacl -d -m group:ubuntu:rwx $HOME/ds054 && sudo setfacl -m group:ubuntu:rwx $HOME/ds054
    - mkdir -p $HOME/docs && sudo setfacl -d -m group:ubuntu:rwx $HOME/docs && sudo setfacl -m group:ubuntu:rwx $HOME/docs
+    - mkdir -p /tmp/fslicense && printf "${FS_LICENSE_CONTENT}" > ${FS_LICENSE}


As discussed - this would be a more robust way to populate the license for CI https://github.com/BIDS-Apps/freesurfer/pull/36/files#diff-29944324a3cbf9f4bd0162dfe3975d88R25

…til it works, fix help of --fs-license-file

oesteban · 2017-11-06T19:57:56Z

Addressing all the latest comments from Chris.

I find it confusing that --fs-license-file is a command line argument for fmriprep-docker but not for native fmriprep. I think it would be better to have it in both places.

Since --fs-license-file is a host option in fmriprep-docker, I don't think we can easily extend it to a fmriprep command, that should take for granted the (correct) installation of FreeSurfer, as nipype would do. I would prefer to keep fmriprep command untouched.

effigies · 2017-11-06T20:58:33Z

I agree with @oesteban. It's a necessary feature for fmriprep-docker to be able to inject a license into the container, but fmriprep native does not bundle FreeSurfer. Alerting users to a missing license seems like a reasonable behavior at the native CLI level.

oesteban · 2017-11-06T23:15:52Z

Tests are passing, this is RTM

effigies

Couple [docs only] changes. And want to add a changelog entry?

I'd recommend holding off on merging this until ready to make a new release, though. This is going to cause every build to re-download FreeSurfer until another release is made with this PR included.

effigies · 2017-11-06T23:34:07Z

docs/installation.rst

+    $ fmriprep-docker --fs-license-file $HOME/.licenses/freesurfer/license.txt \
+        /path/to/data/dir /path/to/output/dir participant
+    RUNNING: docker run --rm -it -v /path/to/data/dir:/data:ro \
+        -v /path/to_output/dir:/out poldracklab/fmriprep:1.0.0 \


This command doesn't have a -v /home/user/.licenses/freesurfer/license.txt:/etc/licenses/freesurfer/license.txt line (or similar.

effigies · 2017-11-06T23:34:23Z

docs/installation.rst

+    $ export FS_LICENSE=$HOME/.licenses/freesurfer/license.txt
+    $ fmriprep-docker /path/to/data/dir /path/to/output/dir participant
+    RUNNING: docker run --rm -it -v /path/to/data/dir:/data:ro \
+        -v /path/to_output/dir:/out poldracklab/fmriprep:1.0.0 \


effigies · 2017-11-06T23:34:41Z

docs/usage.rst

@@ -29,7 +29,7 @@ Command-Line Arguments
   :prog: fmriprep
   :nodefault:
   :nodefaultconst:
-
+ 


Probably unintentional space?

effigies · 2017-11-07T00:51:06Z

This looks good to me, though, again, I think merging should wait until just before the next release.

@chrisfilo do you still feel that the cli/run.py should have a --fs-license-file option? (See #787 (comment) and #787 (comment).)

chrisgorgo · 2017-11-07T00:54:31Z

Yes - we need it for OpenNeuro integration that does not support passing files via environment variables.

oesteban · 2017-11-07T01:08:55Z

Why not mounting the file directly?

…

On Nov 6, 2017 16:54, "Chris Filo Gorgolewski" ***@***.***> wrote: Yes - we need it for OpenNeuro integration that does not support passing files via environment variables. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#787 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkhxnsjsjGnjDZa7Gw8mzzeTosWoLfJks5sz6pHgaJpZM4QE750> .

chrisgorgo · 2017-11-07T01:12:29Z

This feature would have to be added to OpenNeuro specifically for this app. Passing files via command line argument is already supported (as in FreeSurfer BIDS App).

effigies · 2017-11-07T01:30:39Z

After talking with Chris, I think he's right we can't get around this. OpenNeuro mounts the file arbitrarily, and then passes that location with a command-line flag. Given we can't tell them where to mount it, cli/run.py needs to be able to handle the flag. I think the easiest thing is to just take the input and os.environ['FS_LICENSE'] = <path>, so that it's automatically passed to all subprocesses. (Rather than wiring it through to all FreeSurfer utilities.)

It doesn't necessarily require any changes to the Dockerfile or fmriprep-docker to handle this case, though it's possible some simplifications will be possible once the underlying executable handles it.

effigies · 2017-11-08T17:50:05Z

@chrisfilo Good to go?

mfalkiewicz · 2017-11-16T12:44:29Z

Hi guys,
I am having a problem passing the license file to the docker container. Specifically:

$ ls -l /usr/local/freesurfer/license.txt

-rw-r--r-- 1 marcel marcel 34 wrz 5 16:16 /usr/local/freesurfer/license.txt

When I try to run docker (with 1.0.0rc10) I get an error message:

docker run -ti --rm -v /home/marcel/experiments/SDT3/work:/work -v /home/marcel/experiments/SDT3/SDT3_BIDS:/data:ro -v /home/marcel/experiments/SDT3/derivatives/fmriprep:/out poldracklab/fmriprep:latest -w /work --use-aroma --fs-license-file /usr/local/freesurfer/license.txt --omp-nthreads 12 --mem_mb 24000 /data /out participant

Traceback (most recent call last):
File "/usr/local/miniconda/bin/fmriprep", line 11, in
load_entry_point('fmriprep==1.0.0rc10', 'console_scripts', 'fmriprep')()
File "/usr/local/miniconda/lib/python3.6/site-packages/fmriprep/cli/run.py", line 206, in main
raise RuntimeError('ERROR: when --no-freesurfer is not set, a valid '
RuntimeError: ERROR: when --no-freesurfer is not set, a valid license file is required for FreeSurfer to run.

I also tried to mount a local directory inside the container and provide the path to it as --fs-license-file, but it still does not work properly. Any ideas?

effigies · 2017-11-16T13:51:53Z

Hi Marcel,

The simplest approach is to use fmriprep-docker (pip install --user --upgrade fmriprep-docker), which will mount the file correctly, automatically, using the following commandL

fmriprep-docker -w /home/marcel/experiments/SDT3/work --use-aroma --omp-nthreads 12 \
    --mem_mb 24000 /home/marcel/experiments/SDT3/SDT3_BIDS \
    /home/marcel/experiments/SDT3/derivatives/fmriprep participant

However, given your current setup, the easiest quick fix is to mount the license file directly into the FreeSurfer directory:

docker run -ti --rm -v /home/marcel/experiments/SDT3/work:/work \
    -v /home/marcel/experiments/SDT3/SDT3_BIDS:/data:ro \
    -v /home/marcel/experiments/SDT3/derivatives/fmriprep:/out \
    -v /usr/local/freesurfer/license.txt:/opt/freesurfer/license.txt:ro \
    poldracklab/fmriprep:latest -w /work --use-aroma --omp-nthreads 12 --mem_mb 24000 \
    /data /out participant

mfalkiewicz · 2017-11-16T13:55:22Z

Thank you very much @effigies, this works fine now!

oesteban added 2 commits October 20, 2017 14:37

update dockerfile with new release

b126376

[ENH] New FreeSurfer's license mechanism

5d1de69

Enables the online provision of a local license file

oesteban requested a review from chrisgorgo October 24, 2017 19:05

reuse systems $FS_LICENSE, improve docs, update circle

5e93382

effigies reviewed Oct 24, 2017

View reviewed changes

oesteban added 2 commits October 24, 2017 12:54

wrapper/fmriprep_docker.py

49827de

Chris' fix

9039f14

use files, various fixes

00a16a0

chrisgorgo reviewed Oct 27, 2017

View reviewed changes

Merge branch 'master' into enh/NewFreeSurfer

561abc7

oesteban added this to the 1.0.0 milestone Nov 2, 2017

oesteban added 3 commits November 6, 2017 10:51

Merge branch 'master' into enh/NewFreeSurfer

6e69e19

update circle's license writing, remove fmriprep-docker from usage un…

65d69cf

…til it works, fix help of --fs-license-file

update intallation.rst

532a61b

effigies reviewed Nov 6, 2017

View reviewed changes

oesteban added 2 commits November 6, 2017 15:40

final minor details [docs only]

4006eb8

update CHANGES [skip ci]

ba42278

effigies added 2 commits November 7, 2017 15:11

RF: Move license file check fully into cli/run.py

593b4c6

DOC: Fix license target

9f9d9ad

effigies force-pushed the enh/NewFreeSurfer branch from a030180 to 9f9d9ad Compare November 7, 2017 23:16

chrisgorgo approved these changes Nov 8, 2017

View reviewed changes

chrisgorgo merged commit 68efe57 into nipreps:master Nov 8, 2017

oesteban deleted the enh/NewFreeSurfer branch November 16, 2017 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] New FreeSurfer's license mechanism #787

[ENH] New FreeSurfer's license mechanism #787

oesteban commented Oct 24, 2017

effigies commented Oct 24, 2017

effigies left a comment

effigies Oct 24, 2017

effigies Oct 24, 2017

oesteban Oct 24, 2017

chrisgorgo Oct 24, 2017

effigies Oct 24, 2017

effigies commented Oct 25, 2017

chrisgorgo left a comment

chrisgorgo Oct 27, 2017

chrisgorgo Oct 27, 2017

chrisgorgo Oct 27, 2017

chrisgorgo Oct 27, 2017

oesteban commented Nov 6, 2017

effigies commented Nov 6, 2017

oesteban commented Nov 6, 2017

effigies left a comment

effigies Nov 6, 2017

effigies Nov 6, 2017

effigies Nov 6, 2017

effigies commented Nov 7, 2017

chrisgorgo commented Nov 7, 2017

oesteban commented Nov 7, 2017 via email

chrisgorgo commented Nov 7, 2017

effigies commented Nov 7, 2017

effigies commented Nov 8, 2017

mfalkiewicz commented Nov 16, 2017

effigies commented Nov 16, 2017

mfalkiewicz commented Nov 16, 2017


		It is also possible to run ``fmriprep-docker`` with FreeSurfer processing: ::

		$ fmriprep-docker --fs-license $HOME/.licenses/freesurfer /path/to/data/dir /path/to/output/dir participant

[ENH] New FreeSurfer's license mechanism #787

[ENH] New FreeSurfer's license mechanism #787

Conversation

oesteban commented Oct 24, 2017

effigies commented Oct 24, 2017

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Oct 25, 2017

chrisgorgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban commented Nov 6, 2017

effigies commented Nov 6, 2017

oesteban commented Nov 6, 2017

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Nov 7, 2017

chrisgorgo commented Nov 7, 2017

oesteban commented Nov 7, 2017 via email

chrisgorgo commented Nov 7, 2017

effigies commented Nov 7, 2017

effigies commented Nov 8, 2017

mfalkiewicz commented Nov 16, 2017

effigies commented Nov 16, 2017

mfalkiewicz commented Nov 16, 2017