-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
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 . |
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.
a few things that I spotted that might help
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.
might need an if on ASTRA
|
I'm a bit surprised that all the actions are running. we're excluding |
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. |
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.
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. |
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 |
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. |
yeah, but are the wrappers built if ASTRA isn't there? |
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! |
SIRF-SuperBuild/jupyterhub/Dockerfile Line 1 in fe6806f
|
SIRF-SuperBuild/jupyterhub/Dockerfile Line 50 in fe6806f
Fully3D . I don't care really, but just letting you know
|
SIRF-SuperBuild/jupyterhub/Dockerfile Lines 39 to 40 in fe6806f
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 I suppose this works for you, so no need to fix this now. |
|
|
|
|
Installed |
75e5c9f
to
6d057c1
Compare
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 |
d4f0793
to
410d7ac
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.
just a bit concerned about which strategy is best:
- copy SIRF installation dirs from
synerbi/sirf:latest
(CPU) image - copy SIRF installation dirs from
synerbi/sirf:service-gpu
(GPU) image - 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?
### 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 . | ||
|
||
``` |
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.
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?
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 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.
#### 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 . |
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.
I also removed this section @paskino because in the proposed PR it seems CIL is just installed via mamba
... is this correct?
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.
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.
0fa2a03
to
164445e
Compare
b1340a8
to
04e266d
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.
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!
surely this must close quite a few more issues than what's listed? |
I... just looked through the issues and added to the list. |
Major update indeed! Let's assign it to 3.6 and merge :-) |
ghcr.io/synerbi/sirf
latest
,latest-gpu
vM.m.p
M
,M.m
,M.m.p
,M-gpu
,M.m-gpu
,M.m.p-gpu
vM.m.p
edge
,edge-gpu
master
devel
,devel-gpu
master
withcmake -DDEVEL_BUILD=ON -DBUILD_CIL=ON
build --pull
optimisationccache
builds with https://github.com/actions/cacheOMP_NUM_THREADS = cpu_count//2
gadgetron
/jupyterhub/
->/docker/
docker/compose.sh
helper scriptDockerfile
&docker-compose*.yml
download_data.sh
make
=>ninja
builderipywidgets>=8
to fix widget display (at the cost of figure duplicationislicer
support foripywidgets>=8
TomographicImaging/CIL#1600)Dockerfile
)--user
,--group-add
)USER_ID
,UID
)USER_ID
: you could define this as a custom overrideUID
: current userGID
: not needed, vis. https://jupyter-docker-stacks.readthedocs.io/en/latest/using/common.html#user-related-configurations# CMake options
inDockerfile
)SIRF_DOWNLOAD_DATA_ARGS
This PR is currently used to run jupyterhub for the PSMRTBP2022 schoolrequirements.txt
requirements.txt
ifBUILD_CIL=OFF
orBUILD_CIL_LITE=OFF
, and switch to requirements installed fromconda-forge
in case we build CIL.to move to follow-up issues/PRs