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

MNT: Refactor Docker build process #2982

Merged
merged 19 commits into from
Apr 12, 2023
Merged

Conversation

effigies
Copy link
Member

@effigies effigies commented Apr 5, 2023

Changes proposed in this pull request

  • Break Docker build into stages, with each significant (and often slow) download getting its own stage
  • Switch conda environment to being micromamba-managed
  • Create env.yml with the pinned conda environment, keeping this information outside of the Dockerfile
    • Due to FSL relying on portions of the scientific python stack, we continue to install that from conda
  • Use pip-compile to pin our dependency graph and install that after the conda environment

This means that env.yml and requirements.txt need to be updated in lock-step if the dependency affects env.yml. We can probably script this.

Documentation that should be reviewed

@effigies
Copy link
Member Author

effigies commented Apr 5, 2023

@mgxd @oesteban Here's a working proposal. ds005 is expected to fail due to ICA-AROMA and FSL 6.0.6.2.

@effigies
Copy link
Member Author

Test failures: nipreps/niworkflows#795.

@mgxd
Copy link
Collaborator

mgxd commented Apr 12, 2023

Looks like something is wrong with ds210's SDC

Traceback:
	Traceback (most recent call last):
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/nipype/interfaces/base/core.py", line 398, in run
	    runtime = self._run_interface(runtime)
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/niworkflows/interfaces/nibabel.py", line 467, in _run_interface
	    self._results["out_file"] = _advanced_clip(
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/niworkflows/interfaces/nibabel.py", line 705, in _advanced_clip
	    a_min = np.percentile(
	  File "<__array_function__ internals>", line 200, in percentile
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/numpy/lib/function_base.py", line 4205, in percentile
	    return _quantile_unchecked(
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/numpy/lib/function_base.py", line 4473, in _quantile_unchecked
	    return _ureduce(a,
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/numpy/lib/function_base.py", line 3752, in _ureduce
	    r = func(a, **kwargs)
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/numpy/lib/function_base.py", line 4639, in _quantile_ureduce_func
	    result = _quantile(arr,
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/numpy/lib/function_base.py", line 4745, in _quantile
	    take(arr, indices=-1, axis=DATA_AXIS)
	  File "<__array_function__ internals>", line 200, in take
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/numpy/core/fromnumeric.py", line 190, in take
	    return _wrapfunc(a, 'take', indices, axis=axis, out=out, mode=mode)
	  File "/opt/conda/envs/fmriprep/lib/python3.10/site-packages/numpy/core/fromnumeric.py", line 57, in _wrapfunc
	    return bound(*args, **kwds)
	IndexError: cannot do a non-empty take from an empty axes.

@effigies
Copy link
Member Author

I guess we can try bumping caches...

@effigies
Copy link
Member Author

@mgxd I canceled ds005 just because it takes so long. Otherwise this is passing. Note the codecov removal is due to https://community.codecov.com/t/codecov-yanked-from-pypi-all-versions/4259, and has no additional impact since we haven't been using that package.

Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions, but this is really nice!

One consideration (not needed for this PR) would be to create these build stages into their own dedicated repos in nipreps-containers, so other tools could easily grab what they need with a simple COPY --from=<nipreps/tool> ...

Closes #2441

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@@ -0,0 +1,337 @@
#
# This file is autogenerated by pip-compile with Python 3.10
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this handling the extras too? It should account for [telemetry,test]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay adding telemetry, but the function of the tool does not depend on the versions of test dependencies, so I'd rather not set hard pins.

Dockerfile Show resolved Hide resolved
Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should fix it, my bad!

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@effigies effigies merged commit 3c30ee5 into nipreps:master Apr 12, 2023
@effigies effigies deleted the mnt/micromamba branch April 12, 2023 19:41
@effigies effigies added this to the 23.1.0 milestone Apr 20, 2023
effigies added a commit that referenced this pull request Jun 12, 2023
23.1.0 (June 12, 2023)

New feature release in the 23.1.x series.

This release substantially reworks the resampling to fsLR grayordinate space,
better accounting for partial volumes and high variance voxels. If you are
resampling using ``--project-goodvoxels``, we strongly recommend upgrading.

Fieldmap handling is improved, with better preference given to single-band
references in both PEPolar and SyN-SDC schemes. Additionally, fMRIPrep will
no longer estimate fieldmaps that are not intended to be used to correct BOLD
series, reducing unneeded processing.

This release removes ICA-AROMA from the fMRIPrep workflow. To use ICA-AROMA,
set ``MNI152NLin6Asym:res-2`` as a target output space. MELODIC and ICA-AROMA
can be run on the resulting images in a separate pipeline. For further
information on the reasoning behind this change, see
`GitHub issue #2936 <https://github.com/nipreps/fmriprep/issues/2936>`__.

This release increments the versions of ANTs and FSL bundled in the Docker
image.

With thanks to Eilidh MacNicol, Basille Pinsard and Taylor Salo for contributions
in fMRIPrep and SDCflows.

* FIX: Raise RuntimeError at build if echos have mismatched shapes (#3028)
* FIX: Inconsistent fmapless estimation when ignoring fieldmaps (#2994)
* FIX: Dilate BOLD mask by 2 voxels to prevent over-aggressive masking degrading T2* map estimation (#2986)
* FIX: Estimate free memory with "available", not "free" (#2985)
* ENH: Add ``--me-t2s-fit-method`` parameter (#3030)
* ENH: Resample BOLD to fsLR directly, dropping fsaverage intermediate (#3011)
* ENH: Allow SBref+EPI PEPolar fieldmaps to correct BOLD series (#3008)
* ENH: Remove ICA-AROMA from workflow and docs (#2966)
* RF: Filter fieldmaps based on whether they will be used to correct a BOLD series (#3025)
* MNT: Update ANTs pin in Docker image (#3016)
* MNT: Update governance docs (#2992)
* MNT: Refactor Docker build process (#2982)
* MNT: Pin conda environment more strictly (#2853)
* MNT: Require niworkflows ~1.3.6 (#2740)
* CI: Use registry for layer caching (#3012)
* CI: Upgrade docker orb (#2865)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants