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

docker-stacks and jupyterhub #718

Merged
merged 98 commits into from
Feb 21, 2024
Merged

Conversation

paskino
Copy link
Contributor

@paskino paskino commented May 19, 2022

  • drop travis, update GHA docker CI
    • push to ghcr.io/synerbi/sirf
      docker tag SuperBuild branch/tag
      latest, latest-gpu latest tag vM.m.p
      M, M.m, M.m.p, M-gpu, M.m-gpu, M.m.p-gpu tag vM.m.p
      edge, edge-gpu master
      only build & test (no tag) CI (current commit)
      devel, devel-gpu master with cmake -DDEVEL_BUILD=ON -DBUILD_CIL=ON
      • build --pull optimisation
    • push to DockerHub
    • ccache builds with https://github.com/actions/cache
  • auto OMP_NUM_THREADS = cpu_count//2
  • use https://github.com/jupyter/docker-stacks submodule
    • update entrypoint to launch gadgetron
  • drop old docker images & scripts
    • merge /jupyterhub/ -> /docker/
  • add docker/compose.sh helper script
  • update Dockerfile & docker-compose*.yml
  • separate CPU & GPU images
  • ship CIL-Demos, CIL-Exercises, SIRF-contrib
    • run download_data.sh
  • replace make => ninja builder
    • auto num parallel threads
  • force ipywidgets>=8 to fix widget display (at the cost of figure duplication islicer support for ipywidgets>=8 TomographicImaging/CIL#1600)
  • rebase after update versions of many dependencies #862 (maybe expose more cmake options in Dockerfile)
  • update readmes
    • docker permissions (--user, --group-add)
    • docker compose override permissions (USER_ID, UID)
    • build options (# CMake options in Dockerfile)
    • adding user deps
      FROM synerbi/sirf:jupyter-gpu
      USER root
      RUN mamba install pytorch && fix-permissions "${CONDA_DIR}" /home/${NB_USER}
      USER ${NB_UID}
    • SIRF_DOWNLOAD_DATA_ARGS
  • update changelog
  • use current repo state when building rather than cloning from remote URL
    • caveat: uncommitted changes are ignored for safety


to move to follow-up issues/PRs

@paskino
Copy link
Contributor Author

paskino commented May 20, 2022

Also I am building with the following command

 docker build --build-arg BASE_IMAGE=nvidia/cuda:10.0-cudnn7-devel-ubuntu18.04 \
--build-arg PYTHON_INSTALL_DIR=/opt/conda \
--build-arg EXTRA_BUILD_FLAGS="-DBUILD_CIL=ON -DIPP_LIBRARY=/opt/conda/lib -DIPP_INCLUDE=/opt/conda/include -DSTIR_URL=https://github.com/paskino/STIR.git -DSTIR_TAG=bump_parallelproj" \
--build-arg SIRF_SB_URL="https://github.com/paskino/SIRF-SuperBuild.git" \
--build-arg SIRF_SB_TAG="jupyterhub_env" --build-arg NUM_PARALLEL_BUILDS=6 --target sirf .

Copy link
Member

@KrisThielemans KrisThielemans 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 things that I spotted that might help

jupyterhub/README.md Outdated Show resolved Hide resolved
jupyterhub/README.md Outdated Show resolved Hide resolved
jupyterhub/README.md Outdated Show resolved Hide resolved
jupyterhub/README.md Outdated Show resolved Hide resolved
jupyterhub/README.md Outdated Show resolved Hide resolved
@KrisThielemans
Copy link
Member

are we building ASTRA? Best for the intro notebooks.

By the way, I'm not sure that CIL dependencies are alright when building ASTRA (i.e. does it need to be guaranteed that ASTRA is built before CIL? i.e.

set(${proj}_DEPENDENCIES "")

might need an if on ASTRA

@KrisThielemans
Copy link
Member

I'm a bit surprised that all the actions are running. we're excluding jupyterhub/**. I thought for the docker-one could be because the white-space change in Dockerfile, but the others shouldn't run. oh well. Not important now, but if you don't know why this is happening, please create an issue for it after the dust settles.

@paskino
Copy link
Contributor Author

paskino commented Jan 23, 2023

does it need to be guaranteed that ASTRA is built before CIL?

CIL does not depend on ASTRA. We will use ASTRA if present.

For SIRF usage it is not required. @ckolbPTB introductory notebook does not require ASTRA. The gradient_descent one does, see SyneRBI/SIRF-Exercises#192.

@KrisThielemans
Copy link
Member

CIL does not depend on ASTRA. We will use ASTRA if present.

I agree, but if ASTRA is built after CIL, will it still be able to use it?

Why not build it on JupyterHub? too late I guess.

For SIRF usage it is not required.

sure. https://github.com/SyneRBI/SIRF-Exercises/blob/master/notebooks/Introductory/acquisition_model_mr_pet_ct.ipynb uses it as well, although at least now it no longer fails as you've seen.

@paskino
Copy link
Contributor Author

paskino commented Jan 23, 2023

I currently didn't add CIL to the jupyterhub image. I'll add it via conda if required. It is a 5 minute job. :D

@paskino
Copy link
Contributor Author

paskino commented Jan 23, 2023

if ASTRA is built after CIL, will it still be able to use it?

Yes, we only interact with ASTRA at the Python level so, as long as ASTRA and the python wrappers are built we can use it.

@KrisThielemans
Copy link
Member

KrisThielemans commented Jan 23, 2023

as long as ASTRA and the python wrappers are built we can use it.

yeah, but are the wrappers built if ASTRA isn't there?

@KrisThielemans
Copy link
Member

I currently didn't add CIL to the jupyterhub image. I'll add it via conda if required. It is a 5 minute job. :D

well, if there's no CIL at all, even the intro stuff will fail, as we now just test on ASTRA.

I don't understand though. I thought you're grabbing the normal docker image, which has CIL. looks like i don't understand the jupyterhub stuff at all after all!

@KrisThielemans
Copy link
Member

ARG BASE_IMAGE=ubuntu:18.04
is still 18.04

@KrisThielemans
Copy link
Member

ENV SIRF_EXERCISES_DATA_PATH "/mnt/materials/SIRF/Fully3D/SIRF/"
says Fully3D. I don't care really, but just letting you know

@KrisThielemans
Copy link
Member

COPY --from=synerbi/sirf:sirf-core /opt/SIRF-SuperBuild/INSTALL/ /opt/SIRF-SuperBuild/INSTALL
COPY --from=synerbi/sirf:sirf-core /opt/SIRF-SuperBuild/sources/SIRF/ /opt/SIRF-SuperBuild/sources/SIRF/

sirf-core? Doesn't make sense to me. core doesn't have an INSTALL? I suppose that's your local name when building as the one on dockerhub is sirf/core.

I guess it'd have to be release-service-gpu, except that is always going to point backwards. We currently don't seem to build devel-service-gpu (nor devel-gpu) on Travis, so we cannot pull from there.

I suppose this works for you, so no need to fix this now.

@KrisThielemans
Copy link
Member

deprecation isn't installed. Can't say I understand the installation of the dependencies anymore, but I'm sure @paskino does!

@KrisThielemans
Copy link
Member

nbstripout is not available, and there for the instructions fail. (I've updated the HackMD page)

@KrisThielemans
Copy link
Member

KrisThielemans commented Jan 25, 2023

nibabel docopt also missing

@KrisThielemans
Copy link
Member

parallelproj isn't installed, so STIR doesn't have it (and neither does SIRF of course)

@paskino
Copy link
Contributor Author

paskino commented Jan 25, 2023

deprecation isn't installed. Can't say I understand the installation of the dependencies anymore, but I'm sure @paskino does!

Installed

@casperdcl casperdcl changed the title Jupyterhub env docker-stacks and jupyterhub Nov 28, 2023
docker/.bashrc Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/requirements-service.yml Outdated Show resolved Hide resolved
jupyterhub/README.md Outdated Show resolved Hide resolved
version_config.cmake Outdated Show resolved Hide resolved
jupyterhub/Dockerfile Outdated Show resolved Hide resolved
jupyterhub/build_docker_stacks.sh Outdated Show resolved Hide resolved
@casperdcl
Copy link
Member

Some time ago, I also made so that CIL could be installed by conda, however maintaining this will be a problem because I see that we might need to have different requirements for the case we build CIL, we want CIL but not want to build it, etc.

So I propose that CIL can only be built by the SuperBuild.

Is this still the plan @paskino?

Also btw what about replacing all the current SIRF jupyter image stuff with docker-stacks (i.e. no need for paskino#2 nor paskino#3, and simplify/merge a lot of things in ./docker/*)?

Copy link
Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

just a bit concerned about which strategy is best:

  1. copy SIRF installation dirs from synerbi/sirf:latest (CPU) image
  2. copy SIRF installation dirs from synerbi/sirf:service-gpu (GPU) image
  3. just build SIRF properly in the proposed Dockerfile

This PR currently does option (1), but I think (2) is safer, and (3) is best. btw option (3) also means we could perhaps deprecate the old SIRF images in favour of this new docker-stacks way?

CHANGES.md Outdated Show resolved Hide resolved
docker/requirements.yml Outdated Show resolved Hide resolved
jupyterhub/Dockerfile Outdated Show resolved Hide resolved
Comment on lines -54 to -65
### Start building SIRF

Build the `sirf` target of the SIRF Dockerfile with the `nvidia/cuda:11.5.0-cudnn8-devel-ubuntu18.04` base image.

```
git clone [email protected]:SyneRBI/SIRF-SuperBuild.git
cd SIRF-SuperBuild/docker

# build standard SIRF docker
docker build --build-arg BASE_IMAGE=nvidia/cuda:10.2-cudnn8-devel-ubuntu18.04 --build-arg PYTHON_INSTALL_DIR=/opt/conda --target sirf .

```
Copy link
Member

Choose a reason for hiding this comment

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

I removed this section @paskino but I'm concerned it doesn't match.

The removed readme text implies you have to rebuild SIRF images using nvidia/cuda... but in the proposed Dockerfile we just COPY --from=synerbi/sirf:latest (i.e. the CPU SIRF image). Is this ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that when doing this I was building the GPU SIRF. And this doc was for me.

At the time I needed to build a specific branch/tag of SIRF(SuperBuild); these were the commands I used to build the SIRF image I required. Then I copied the files from such image to the docker-stack one.

Comment on lines -67 to -74
#### Building CIL

Please see [here](https://github.com/SyneRBI/SIRF-SuperBuild#building-ccpi-cil) for detailed info on the command below.


```

docker build --build-arg BASE_IMAGE=nvidia/cuda:10.2-cudnn8-devel-ubuntu18.04 --build-arg PYTHON_INSTALL_DIR=/opt/conda --build-arg EXTRA_BUILD_FLAGS="-DBUILD_CIL=ON -DIPP_LIBRARY=/opt/conda/lib -DIPP_INCLUDE=/opt/conda/include -DBUILD_ASTRA=ON" --target sirf .
Copy link
Member

Choose a reason for hiding this comment

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

I also removed this section @paskino because in the proposed PR it seems CIL is just installed via mamba... is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that having this is important for when one wants to build a particular branch/tag of CIL which is not available as conda package.

jupyterhub/Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

I haven't tried this, nor looked at it in detail, but overall impression is good, and my remaining worries have been resolved, so let's merge!

@KrisThielemans
Copy link
Member

surely this must close quite a few more issues than what's listed?

@casperdcl
Copy link
Member

surely this must close quite a few more issues than what's listed?

I... just looked through the issues and added to the list.
This PR no longer looks daunting.
The length of the issues it fixes looks daunting!

@KrisThielemans
Copy link
Member

Major update indeed! Let's assign it to 3.6 and merge :-)

@casperdcl casperdcl merged commit bfb5f9f into SyneRBI:master Feb 21, 2024
8 checks passed
This was referenced Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment