-
Notifications
You must be signed in to change notification settings - Fork 144
bump python packages, use UV instead of conda, update docker build #766
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
base: dev
Are you sure you want to change the base?
Conversation
c658130 to
d5dadad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes FastSurfer's build infrastructure and dependency management by migrating from conda to UV as the package manager and updating all Python packages.
Changes:
- Complete migration from conda to UV for Python environment management
- Removal of all conda-specific files and configuration
- Docker build refactoring to use UV instead of conda, with Ubuntu 24.04 as base
- Python version references updated from
python3.10topython3throughout scripts - Addition of xvfb-run support for headless OpenGL rendering in corpus callosum QC
- Package version updates in pyproject.toml including torch 2.7.*, torchvision 0.22.1, and new dependencies
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/build/link_fs.sh | Convert string to array syntax, change error to warning |
| tools/build/install_fs_pruned.sh | Remove login shell, add error handling for parallel execution, convert strings to arrays |
| tools/Docker/install_env.py | File deleted - conda-specific environment installer |
| tools/Docker/build.py | Update shebang, replace conda with venv references, add new build args, update version support |
| tools/Docker/entrypoint.sh | Update comment from conda to venv |
| tools/Docker/conda_pack.sh | File deleted - conda-specific packaging script |
| tools/Docker/Dockerfile | Major refactor: UV instead of conda, Ubuntu 24.04, new build stages, add xvfb dependencies |
| tools/Docker/README.md | Update documentation for UV, new version numbers |
| run_fastsurfer.sh | Update python reference, add xvfb-run support for OpenGL |
| recon_surf scripts | Update python version references |
| pyproject.toml | Update dependencies, add Python 3.13 support, pin torch/torchvision versions |
| env/*.yml | Conda environment files deleted |
| doc/overview/*.md | Update documentation from conda to UV |
| Tutorial files | Update from conda to UV instructions |
| FastSurferCNN/version.py | Minor formatting changes, update git status flags |
| FastSurferCNN/utils/checkpoint.py | Improved error handling for checkpoint downloads |
| FastSurferCNN/data_loader/conform.py | Fix type annotation |
| CorpusCallosum/shape/mesh.py | Improve OpenGL/whippersnappy error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
37e500d to
d208b9e
Compare
a68b22e to
ef53e04
Compare
Update pyproject dependencies. Update docker build script.
install conda pack into a separate environment add labels to the different images pass values for the labels in the images through the build script make sure linking fspython to /venv/bin/python works Some formatting cleanup Resolve upx bugs
change the virtual environment and python install command from conda to uv Remove conda-related files, as our primary install mechanism is now uv (instead of conda, with the dependencies defined in pyproject). Update version and build scripts to work with the new Dockerfile.
…king it more flexible).
Add a check/error message if we are running headless and wrap the fastsurfer-cc call in xvfb-run.
…ftlink so it uses the correct venv and search paths
… requests, so prefer b2share from juelich
26066ee to
c4aaa3a
Compare
…cc.py, if the QC thickness image does not get rendered (due to whippersnappy, opengl, glfw, or x-server connection/framebuffer missing.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
8e1d060 to
9cf0609
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 39 out of 41 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dry_run : bool, optional | ||
| Whether to actually trigger the build, or just print the command to the console | ||
| (default: False => actually build). | ||
| Path o the working directory to perform the build operation (None: inherit). |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "o" should be "to".
The comment reads "Path o the working directory" but should read "Path to the working directory".
| echo "WARNING: The --qc_snap option of the corpus callosum module requires OpenGL support, but we could not" | ||
| echo " create OpenGL handles. For Linux headless systems, you may install xvfb-run to provide a virtual display." | ||
| fi | ||
| echo " FastSurfer will not fail due to the inavailability of OpenGL, but some QC snapshots (rendered thickness" |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in error message: "inavailability" should be "unavailability".
| import torch | ||
|
|
||
| p = Path(torch.__file__).parent / "lib" | ||
| (p / "libnvrtc.so").symlink_to(p / p.glob("libnvrtc-*.so.11.2")) |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug in glob usage: The code p.glob("libnvrtc-*.so.11.2") returns a generator, but it's being passed directly to symlink_to() which expects a string or Path object. This will likely fail at runtime. The code should use next(p.glob("libnvrtc-*.so.11.2")) to get the first match from the generator, or use list(p.glob(...))[0].
| # dl aria2c if that exists, else wget or curl | ||
| if [[ -n "$(which aria2c)" ]] ; then dl=(aria2c -cx 16 -s 16 --check-certificate=false -o "$freesurfer_dl" "$fslink") | ||
| elif [[ -n "$(which wget)" ]] ; then dl=(wget --no-check-certificate -qO- "$fslink" -O "$freesurfer_dl") | ||
| else dl=(curl -L --insecure "$fslink" -o "$freesurfer_dl") | ||
| fi |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FreeSurfer archive is downloaded with TLS certificate validation explicitly disabled using aria2c --check-certificate=false, wget --no-check-certificate, or curl --insecure, and then extracted and used to provide binaries in the build/runtime image. This allows a network attacker (e.g., on the same LAN or controlling DNS) to perform a man‑in‑the‑middle attack on the download and supply a malicious FreeSurfer tarball that will be unpacked and executed during image build, compromising the build and downstream images. To mitigate this, enforce strict HTTPS certificate verification for all download methods and add an integrity check (e.g., pinned checksum or signature verification) for the FreeSurfer archive before extraction.
This PR includes:
Todo:
finish surface pipeline testingcurrently there is an error where Freesurfer'srca-config(a python script) cannot importyamlbut yaml is installed and available in the distributed python environment