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

Packaging FastSurfer for Pypi and conda-forge #262

Open
mscheltienne opened this issue Feb 21, 2023 · 6 comments
Open

Packaging FastSurfer for Pypi and conda-forge #262

mscheltienne opened this issue Feb 21, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@mscheltienne
Copy link

Hello,

Would it be possible to package FastSurfer completely for Pypi (and eventually conda-forge).
The main folder FastSurferCNN already has the suited structure. The main entry-point run_fastsurfer.sh could be defined as an actual entry-point.

This packaging would make distribution of FastSurfer simpler for native installation, especially for people not familiar with Docker. At the moment, a native install requires to edit several files: requirements, python version used in run_fastsurfer.sh, ... and it also creates many uncommitted and non-ignored files in the cloned repository.

I could open a PR for this change if needed.

Mathieu

@mscheltienne mscheltienne added the enhancement New feature or request label Feb 21, 2023
@m-reuter
Copy link
Member

Note that FastSurfer includes the surface pipeline and we are heading towards a more modularised pipeline with multiple modules such as Cerebellum sub segmentation, Olfactory Bulb, Hypothalamus, Corpus Callosum etc. This will change directory structure, as these modules share many of the same functions.

Generally I think it makes sense to move the packaging forward, but maybe we should wait a little until the first modules appear. Also need to think on how to deal with FreeSurfer dependencies, which are large.

@m-reuter
Copy link
Member

Also note, that changing python versions and also dependencies in the requirements, will lead to an unsupported install, which is not tested. We have seen in the past, e.g. for NiBabel, that changing dependency versions can lead to error and silent failures, that can sometimes be hard to detect and localise.

@mscheltienne
Copy link
Author

Looking forward to a multi-module approach. I agree that thinking ahead of the directory and code structure is super important. However, adding module/moving files and function around should not impact the packaging itself.

changing python versions and also dependencies in the requirements, will lead to an unsupported install

Ok, but Python 3.8 is already quite old.. Many projects support up to the 4 last version of Python, which means that Python 3.8 is already the oldest supported version, e.g. for conda-forge. When setting up FastSurfer, the pinned version in the requirements file were incompatible with Python 3.10, that I am using (3.11 is not yet supported by PyTorch).
This kind of issues should be picked up by unit tests and CIs, CIs that can be run against the stable and the development version of your main dependencies, e.g. nibabel, to catch failures before the new releases.

I'm a bit new to the MRI-side and I'm more familiar with electrophysiological data (EEG, MEG, ..), but I can help with the packaging/CI side if you want.

@m-reuter
Copy link
Member

Thanks, we already do extensive testing. The problem is whenever you change a tiny bit, results will differ. It is then hard to say if this difference is still OKish or if it is too far off. Our test-pipeline consists of some quick tests, but for releases, we run hundreds of images from various datasets and look at accuracy, test-retest reliability and sensitivity to some known real effects (aging or disease). That is why releases often take a few months from when they are basically ready, as - this way - we find and remove already a lot of small bugs.

We plan to move more to CI, but currently we would need a server with GPUs for that. We'd need to test both GPU and CPU pipelines (on multiple image resolutions). Running the surface pipeline further takes 1 h of processing. Do you know a service that can provide this for free? Currently we only build the docker images via travis.

And it would be great to get your help with any of this.

@mscheltienne
Copy link
Author

The problem is whenever you change a tiny bit, results will differ.

Again, I'm new to this side of the brain but that sentence is a bit worrying. I understand that contrary to electrophysiological data, it is difficult to have quick tests run on small files, but I would expect that 2 runs on the same image should produce the same output, regardless of the hardware used to run it (C/GPU) (with a certain tolerance, e.g. the numpy defaults for np.allclose). Is that not the case?

If that is the case, the CPU pipeline could be run entirely on free CIs, ensuring that the behavior of the library stays constant, while the GPU pipeline could be run on a local machine in a self-hosted runner. It's not free, but it will be cheaper than using a remote server with GPU capabilities which will likely have to be configured as a self-hosted runner anyway since CIs provider are reluctant to provide GPU capabilities as they could be misused.

@m-reuter
Copy link
Member

Usually differences are tiny, you would have to try out as it depends on what you change. The pipelines for the brain however are highly complex. For example, inclusion of a single voxel into the GM volume will modify the brainmask, and can lead to inclusion of some bright pixels outside the brain. This again can affect the global intensity scaling slightly, which will lead to a less bright image and will shift surfaces almost globally. Since there is no real ground truth for surface placement, the new location is similarly OK than the old one, but in order to really tell, one would need to open up that case and take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants