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

Change FSL autodetect behaviour #1459

Merged
merged 1 commit into from Sep 24, 2018

Conversation

@octomike
Copy link

commented Sep 17, 2018

In environments where multiple FSL versions can be loaded (HPC),
preferring the fsl5.0-X defaults leads to undesirable behaviour.

Users may explicitly activate some custom FSL either with environment
modules or manually with the default FSL setup procedure. If there is
a fsl package installed which prefixes their scripts with "fsl5.0-"
(i.e. neurodebian) mrtrix should not prefer that one over the explicit
user choice. This PR defensively changes the preference, but actually I would argue to completely drop the fsl5.0 convenience detection. If this is just to work with neurodebian then this assumption is missing the fact that neurodebian still requires users to run source /etc/fsl/5.0/fsl.sh to actually load and activate everything. Afterwards all binaries are in PATH and can and should be found with find_executable(name).

@Lestropie Lestropie added the scripts label Sep 17, 2018

@Lestropie

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

Thanks for the PR. The convenience detection was implemented simply to "get the scripts to work" for some users. I wasn't aware of any kind of precedence or hierarchy: I had instead assumed that the "fsl5.0-" prefix was for FSL5 installs that were intended to reside alongside FSL4.

If there's good justification for not auto-detecting such binaries (please link any relevant pages), I'm happy to make the change. Though I would be in preference of making the change on the dev branch rather than master, just in case it would result in a different version of FSL being invoked on some users' systems, which has a chance to alter outcomes: in between tag updates, we're trying to alter master with bug fixes only.

Also, if the presence of prefixed binaries but absence of non-prefixed binaries in the path is indicative of a "compulsory" setup script having not been run, alternative logic for fsl.exeName() (or appropriately renamed function) would be:

  1. If non-prefixed binary is in PATH, that should be used.

  2. If not, but the "fsl5.0-" prefixed binary is, then write an error message instructing that the FSL setup is incomplete / inappropriate, and that the appropriate setup script needs to be run.

  3. If neither is present, then write an error message saying that FSL is not at all installed.

?

@octomike

This comment has been minimized.

Copy link
Author

commented Sep 18, 2018

I had instead assumed that the "fsl5.0-" prefix was for FSL5 installs that were intended to reside alongside FSL4.

As far as I know the fsl5.0- prefix is very specific to the fabulous, but outdated Neurodebian (ND) packages. Their existence is described in this README.debian. It turns out it is a convenience layer to run a FSL program without setting FSLDIR and sourcing the setup script first. People probably don't know about that layer and when using the ND packages just assumed that they have to run the commands with the prefix, just like this person in the ND comments section.

If there's good justification for not auto-detecting such binaries (please link any relevant pages), I'm happy to make the change.

Okay, I will try to find other people falling over this. But I really think this is potentially very dangerous bug concerning reproducibility. In HPC systems [1, 2] people usually activate and pin their software with something called environment modules. So a job would look something like this:

#PBS -l mem=32gb,nodes=1:ppn=8
module load freesurfer/5.3.0
module load fsl/5.0.7
module load mrtrix3/rc3

dwipreproc foo bar baz
[...]

These environment modules mostly just change PATH, LD_LIBRARY_PATH and the sorts to point to a specific version. What happens right now is that users activate a specific FSL version and mrtrix3 is reverting that back iff the ND packages happen to be installed as well. This is probabaly not just a bug in our environment and I can imagine that some people might not even notice they're using the outdated FSL. In the worst case they would report their results with the wrong FSL version.

Though I would be in preference of making the change on the dev branch rather than master

That makes total sense, I'll change the target branch.

Also, if the presence of prefixed binaries but absence of non-prefixed binaries in the path is indicative of a "compulsory" setup script having not been run, alternative logic for fsl.exeName() (or appropriately renamed function) would be:

I was too fast with my judgement. The README.debian states that the prefixed binaries are a convenience so there is no need to output an error in the second case. However, if you want to keep the ND autodetection/convenience, I believe, as stated above, it is necessary to change the order to always prefer the default binaries currently in the PATH.

@octomike octomike changed the base branch from master to dev Sep 18, 2018

Change FSL autodetect behaviour
In environments where multiple FSL versions can be loaded (e.g. HPC)
preferring the fsl5.0-X defaults leads to undesirable behaviour.

Users may explicitly activate some custom FSL either with environment
modules or manually with the default FSL setup procedure. If there is
a fsl package installed which prefixes their scripts with "fsl5.0-"
(i.e. neurodebian) mrtrix should not prefer that one over the explicit
user choice.

@octomike octomike force-pushed the MPIB:fix_fsl_autodetect branch from e3d3934 to 83ac12a Sep 18, 2018

@Lestropie Lestropie self-requested a review Sep 19, 2018

@Lestropie

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

Okay, I will try to find other people falling over this.

No need to go constructing a comprehensive list; just wanted a link to anything you'd already come across / discussions that had happened that had led to the PR.

What happens right now is that users activate a specific FSL version and mrtrix3 is reverting that back iff the ND packages happen to be installed as well.

Yep, just that scenario in isolation is a sufficient justification to alter the logic.

But I really think this is potentially very dangerous bug concerning reproducibility.

If you're concerned about it, you're more than welcome to write a post on the community forum describing to people in what circumstances the issue may arise. That way you also get full credit for it.

The README.debian states that the prefixed binaries are a convenience so there is no need to output an error in the second case.

Okay, in that case let's just flip the precedence. At least there's some existing opportunity for feedback in that if the prefixed binaries are invoked, the stderr output generated by the script will reflect as such in its "Command: " strings.

@Lestropie
Copy link
Member

left a comment

I'm happy with this. Unfortunately TravisCI is failing due to a Sphinx 1.8.0 bug; hopefully fix is not too far away, others on Travis are having the same issue so 1.8.1 should get merged in pretty quickly once released.

@jdtournier

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

But I really think this is potentially very dangerous bug concerning reproducibility.

If you're concerned about it, you're more than welcome to write a post on the community forum describing to people in what circumstances the issue may arise. That way you also get full credit for it.

Sounds like a candidate for our new FAQ wiki section...? Although we haven't officially opened it up to users yet. Now might be the time?

@Lestropie

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

Sounds like a candidate for our new FAQ wiki section...?

Maybe less so than other things: Users may need to be alerted that this is already happening currently with their processing, whereas they won't get automatically alerted to the presence of a new wiki entry.

@Lestropie Lestropie merged commit c610f50 into MRtrix3:dev Sep 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jdtournier

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

they won't get automatically alerted to the presence of a new wiki entry

Well, actually I think they will, given that it would just be a regular post in a different category, with the ability for regular users to edit it. But it's just a thought, no big deal.

@Lestropie

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

Well, actually I think they will

Sorry was thinking of GitHub wiki rather than forum wiki.

$ git reset HEAD~1

@jdtournier

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

Yes, there's been a few changes of direction on the FAQ front... Latest suggestion is this one.

@octomike octomike deleted the MPIB:fix_fsl_autodetect branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.