Fix setuptools packaging - #860
Conversation
…um, and ship config data files
There was a problem hiding this comment.
🟡 Changes recommended
The pydocstyle directory expression matches none of the intended package directories.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes incomplete setuptools packaging as groundwork for pip/conda distribution.
Changes:
- Enables recursive discovery of all five package trees.
- Includes required runtime configuration and lookup data.
- Updates pydocstyle directory matching.
File summaries
| File | Description |
|---|---|
pyproject.toml |
Configures package discovery, package data, and pydocstyle scope. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ackage dirs instead of listing packages
There was a problem hiding this comment.
🟡 Changes recommended
The wheel still omits recon-surf.sh, which an installed Python module opens at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
…n-surf.sh at parser build time
There was a problem hiding this comment.
🟢 Approval recommended
The configuration correctly packages namespace subpackages and required runtime resources without affecting enabled CI checks.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
pyproject.tomldeclared the installed packages as an explicit list:Two things were wrong with it.
1.
CorpusCallosumwas never added. It is a proper package (__init__.pyplus seven subpackages), added after this list was written.2. The list is not recursive. setuptools treats these as literal package names and does not descend into them, so no subpackage was installed either. A
pip install .into a clean target gave:On top of that, no non-
.pyfile was installed at all (0 data files), so even the modules that did land could not findcheckpoint_paths.yaml, the network configs or the color lookup tables.The installed distribution was therefore unable to import its own modules. This went unnoticed because nothing ever imports from site-packages: every entry point puts the source tree first on
PYTHONPATH(run_fastsurfer.sh:576,recon_surf/long_prepare_template.sh:236, the Docker image'sENV PYTHONPATH=/fastsurfer, and the macOS package'spostinstall). In practicepip installonly served to resolve the dependency list.Changes
Discovery is namespace-aware, so the directories that have no
__init__.pyare still picked up (FastSurferCNN/config,CerebNet/config/dataset,CorpusCallosum/config,recon_surfitself andrecon_surf/utils) — nothing regresses versus the old explicit list. Theincludepatterns keepdoc,test,env,buildandcheckpointsout.Also: pydocstyle checked nothing at all
While registering
CorpusCallosumin[tool.pydocstyle], it turned out the existing setting was inert:match-diris a single regex matched against each directory name during the walk, not a comma-separated list. A directory is calledFastSurferCNNorutils, neverFastSurferCNN...,^CerebNet..., so the commas are literal characters and the embedded^anchors can never match. No directory matched at any level, the walk stopped at the repo root (which has no.pyfiles), andpydocstyle .reported 0 files checked. Measured with pydocstyle 6.3.0.An allow-list alternation would not fix it either, since the same pattern is applied at every level —
^(FastSurferCNN|CerebNet|...)would pruneutils/,models/,config/beneath them. So the packages are selected by excluding the non-package directories instead:Traversal now covers exactly the 26 package directories — all five roots plus every subpackage, including
CorpusCallosum/*andrecon_surf/utils.This surfaces 116 files with 513 existing docstring findings (previously 0/0). That is the pre-existing docstring backlog becoming visible, not new breakage, and it is why the pydocstyle step is commented out in
.github/workflows/code-style.yml:34-35. CI is unaffected by this PR. The point of the change is that when the docstring cleanup is picked up later,pydocstyle .will actually report the work instead of silently returning clean.Verification
pip install --no-deps --targetinto a scratch directory:CorpusCallosumand its seven subpackages.checkpoint_paths.yaml, theFastSurferVINN_*/HypVINN_*/CerebNet_*configs,FastSurfer_ColorLUT.tsv,FreeSurferColorLUT.txt,HypVINN_ColorLUT.txt,CerebNet2FreeSurfer.json,fsaverage_target.json, the threeDKTatlaslookup.txt.FastSurferCNN.utils.checkpoint,CorpusCallosum.utils.checkpoint,HypVINN.models.networks,CerebNet.models.networksandrecon_surf.image_ioall import, andFASTSURFER_ROOTresolves inside the installed tree.Impact
pyproject.tomlonly for dependency resolution (uv pip compile --extra container ... | uv pip sync,tools/Docker/Dockerfile:159-172) and thenCOPY . /fastsurfer/withPYTHONPATH=/fastsurfer. setuptools is never invoked, so these settings are inert there.postinstallrunspip install "$FASTSURFER_HOME[qc]"). Its venv now gets a complete copy instead of a partial one. No behavioral change, sincerun_fastsurfer.shprepends$FASTSURFER_HOMEtoPYTHONPATHeither way.Groundwork for #262 / #618 (pip/conda distribution), but not a solution to them — those are blocked on the FreeSurfer dependency, not on this.