Add FreeSurfer 8.2.0 support (bids/freesurfer:8)#96
Open
karl-koschutnig wants to merge 4 commits into
Open
Conversation
FreeSurfer 8.2.0 ships only as a .deb (no generic tarball), which neurodocker's --freesurfer template doesn't support, so the fs8 install is a hand-written --run-bash step in generate_freesurfer_images.sh (same pattern already used there for miniconda/nodejs/bids-validator). Adds Dockerfile_fs8 / Singularity_fs8, documents `docker pull bids/freesurfer:8` in the README, and wires fs8 build/deploy into CircleCI (fs8 skips the test matrix, which compares group2 output against a FS6-only precomputed fixture with no FS8 equivalent). run.py: - Detect the FreeSurfer version from a new FS_VERSION env var instead of parsing build-stamp.txt (whose format isn't guaranteed across major versions). FS_VERSION is set for all three images. - Fix df.append() (removed in pandas 2.0) -> pd.concat(), and mean(1) -> mean(axis=1) (keyword-only in pandas 4.0); fs8 no longer pins ancient pandas as a result. - Keep the FS6 python2 prefix for aparcstats2table/asegstats2table, generalized to `fsversion >= 7` so FS7/FS8 (no python2) run them directly while FS6 still works. Also fixes two pre-existing issues surfaced while regenerating: the shared env block hardcoded FREESURFER_HOME=/opt/freesurfer (wrong for fs7, which installs to /opt/), now derived from the per-version install dir; and the pinned neurodocker miniconda template predates Anaconda's Terms-of-Service gate, so the conda step is hand-written with the required `conda tos accept` calls (affects all three versions). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI on this PR shows build_6 and build_7 failing (build_8 passed) with: LibMambaUnsatisfiableError: package pandas-1.5.3-... requires python >=3.10,<3.11.0a0, but none of the providers can be installed Miniconda's "latest" installer now ships Python 3.14, which can't be solved against a pin on pandas 1.5.3. Not caused by this PR's own changes, but exposed by it (apparently the first fresh CI build in a while) and directly fixable here: the only reason for that pin was run.py's df.append() call, removed in an earlier commit on this branch for pandas 2.0 compatibility -- unconditionally, so it applies to all three images already, not just fs8 (which already used unpinned pandas). Verified the fixed conda install command directly in a plain ubuntu:jammy container: resolves and installs pandas 3.0.3 + nibabel cleanly against Python 3.14. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI on this PR (bids-apps#96) showed test_7_1/test_7_2 failing with "recon-all: not found" (exit 127) -- a real regression introduced earlier on this branch, not a pre-existing issue. Root cause: the 7.4.1 tarball's entries are "./freesurfer/...", so `tar --strip-components 1` only strips the leading "./", not "freesurfer/". Extracting into INSTALL_DIR=/opt/ (the existing, correct value, unchanged since 2023's "fix fs7 install dir") lands the files one level down, at /opt/freesurfer -- INSTALL_DIR is the parent of the true FreeSurfer root for fs7, not the root itself. fs6 and fs8 don't have this indirection: their INSTALL_DIR already *is* the root. An earlier commit on this branch parameterized the shared --env block (FREESURFER_HOME, PATH, etc.) to use $INSTALL_DIR directly, reasoning from a hardcoded FREESURFER_HOME=/opt/freesurfer in that block "not matching" INSTALL_DIR=/opt/. That reasoning was wrong: the hardcoded value was correct (compensating for the tarball's nesting), and "fixing" it to use $INSTALL_DIR verbatim broke fs7's PATH -- confirmed only by CI, since the earlier fix was verified by diffing generated Dockerfile text, never by an actual `docker build` + `recon-all -version` of fs7. Fix: introduce FS_ROOT, distinct from INSTALL_DIR, for the shared env block. FS_ROOT == INSTALL_DIR for fs6/fs8 (no behavior change -- confirmed via `git diff` showing zero change to Dockerfile_fs6/fs8). For fs7, FS_ROOT=/opt/freesurfer while INSTALL_DIR stays /opt/ (the tar extraction target). Verified this time by actually building Dockerfile_fs7 and running it, not just reading the generated text: - `recon-all -version` -> freesurfer-linux-centos7_x86_64-7.4.1-... - full `autorecon1` run on ds114_test1/sub-01 -> "recon-all -s sub-01 finished without error", recon-all.done marker present. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Hi @chrisgorgo, I’m writing to inquire about the status of this repository. I’m particularly interested in using Freesurfer 8.2 as a bids-app on our high-performance cluster. I apologize for asking directly, but I’m eager to get started. Best regards, |
Contributor
|
Thanks for reviving those PRs! |
Author
|
@bpinsard, thanks for the quick reply. Is anyone else available to help move this forward? It’s a really convenient container for HPC use. I recently built my own but perhaps others would like to use it as a bids container too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add FreeSurfer 8.2.0 support (
bids/freesurfer:8)Summary
Adds FreeSurfer 8.2.0 as a new image alongside 6 and 7, published as
bids/freesurfer:8(not the default — the untagged default is unchanged).8.2.0 ships only as a
.deb/.rpm(no generic tarball), which neurodocker's--freesurfertemplate doesn't support, so fs8 installs via a hand-written--run-bashstep ingenerate_freesurfer_images.sh(same pattern already used there for miniconda/nodejs/bids-validator). AddsDockerfile_fs8/Singularity_fs8(ubuntu:jammy, like fs6/fs7), documents the:8tag in the README, and wires fs8 into CircleCI build/deploy.run.pyFS_VERSIONenv var instead of parsingbuild-stamp.txt(which silently misidentified anything ≥ 8 as FS6). Set in all three images.df.append()→pd.concat()(removed in 2.0) andmean(1)→mean(axis=1)(keyword-only in 4.0); fs8 drops the old pandas pin.group2python2 prefix kept for FS6 but generalized tofsversion >= 7, so FS7/FS8 runaparcstats2table/asegstats2tabledirectly.Incidental fixes (affect fs6/fs7 too)
Regenerating the Dockerfiles surfaced two pre-existing issues; both are why the fs6/fs7 files change here:
--minicondatemplate predates Anaconda's Terms-of-Service enforcement, so a fresh build of any version now fails onconda update. The conda step is hand-written with the requiredconda tos acceptcalls.FREESURFER_HOME— the shared env block hardcoded/opt/freesurfer, but fs7 installs to/opt/; now derived from each version's install dir.Testing
docker build -f Dockerfile_fs8succeeds;--version/--helpwork.--stages autorecon1on the repo'sds114_test1(no session) andds114_test2(with session) fixtures — both finish without error.group2stats export, all clean — confirmsrun.py's completion-marker/directory assumptions still hold on 8.2.0.Relation to #91 and #92
@bpinsard's #91 and #92 tackle overlapping ground and deserve credit/consideration alongside this:
2.1.1instead of hand-rolling a--run-bashworkaround, so it can keep using the plain declarative--minicondaflag. That's arguably the better long-term fix; I kept the pinned sha256 + hand-written conda step here mainly to keep this diff's blast radius small and because I hadn't seen [FIX] update neurodocker to fix conda tos accept issue #91 until after building on the older pin. Also independently fixed thedf.append()removal (via a list + singlepd.concat(), cleaner than the per-iterationpd.concat()here) and deploy'sDOCKER_TOKENgap (by restrictingdeployto tags-only, rather than leaving it to fail loudly on every push as this PR does).mri_synthstrip's ~24GB memory requirement doesn't fit CircleCI'sresource_class: large. This PR sidesteps that by deliberately leaving fs8 out of thetestjob/matrix (see below) — build and deploy don't need to runrecon-allat all, so the memory ceiling never comes up. Worth flagging in case that's not an acceptable tradeoff for this repo.Happy to rebase this on top of #91 if that lands first, adopt its neurodocker-version-bump approach here instead of the hand-rolled conda fix, or fold the two into one PR — whatever's least work for you.
Notes for maintainers
DOCKER_TOKEN/DOCKER_USERsecrets, which forked-PR builds don't get — expectdeploy_8to stay red on this PR regardless; see [FIX] update neurodocker to fix conda tos accept issue #91 for a cleaner fix to that).testmatrix — see add Dockerfile for fs8, bypasses neurodocker #92 above for why (CI memory limits), on top of there being no FS8 precomputed fixture for the existinggroup2-comparison test to use.