Docker tests

[ckolbPTB]

Run tests, notebooks and binder in docker container

Closes #151

[fzimmermann89]

Nice!
Why do you install the packages using conda and a separate .yml instead of using pip and installing the dependencies from the pyproject.toml? Is this planned to change?

[github-actions]

Coverage Report

File	Stmts	Miss	Cover	Missing
src/mrpro/algorithms
_adam.py	17	1	94%	80
_lbfgs.py	17	1	94%	69
_remove_readout_os.py	27	2	93%	48, 50
src/mrpro/data
_AcqInfo.py	82	2	98%	97, 135
_Data.py	15	2	87%	58, 71
_DcfData.py	83	4	95%	35, 78, 103, 191
_IData.py	51	4	92%	110, 118, 124, 128
_IHeader.py	58	2	97%	77, 104
_KHeader.py	126	9	93%	34, 81, 119, 164, 175, 182–183, 186, 193
_KNoise.py	21	9	57%	51–64
_KTrajectory.py	63	2	97%	232, 246
_SpatialDimension.py	39	5	87%	76, 79, 86–88
_TrajectoryDescription.py	14	1	93%	36
src/mrpro/data/_kdata
_KData.py	118	12	90%	97–98, 206, 208, 230, 235–236, 280, 282, 286, 324, 338
_KDataRearrangeMixin.py	17	1	94%	22
_KDataSelectMixin.py	23	3	87%	23, 63, 79
_KDataSplitMixin.py	52	4	92%	26, 68, 98, 107
src/mrpro/data/traj_calculators
_KTrajectoryCalculator.py	26	3	88%	37, 63, 65
_KTrajectoryIsmrmrd.py	16	2	88%	56, 65
_KTrajectoryPulseq.py	33	1	97%	69
src/mrpro/operators
_CartesianSamplingOp.py	48	9	81%	53–54, 59–60, 65–66, 90, 93, 116
_ConstraintsOp.py	59	2	97%	35, 37
_LinearOperator.py	49	4	92%	37, 90, 106, 111
_Operator.py	45	1	98%	36
_ZeroPadOp.py	16	1	94%	42
src/mrpro/utils
_split_idx.py	10	2	80%	55, 59
_zero_pad_or_crop.py	31	6	81%	38, 42, 66, 69, 72, 75
TOTAL	1862	95	95%

Tests	Skipped	Failures	Errors	Time
204	0 💤	0 ❌	0 🔥	52.373s ⏱️

[ckolbPTB]

AIM: Setting up python and installing all dependencies takes ~100s in GitHub actions - this is what we have to beat

I managed to reduce the size of the container from 10GB to 1.8GB and simplify it which means it now only takes 35s to pull the image and start the container.

Next step: Figure out how to tell pytest to actually use the pre-installed stuff and not reinstall everything again...

[ckolbPTB]

pip-compile which we are using to create the requirements.txt ignores any pre-installed packages. I could not find a way to ensure it creates a requirements.txt for pytorch-cpu. Therefore, I am now simply installing mrpro after installing pytorch-cpu. I am also installing all the test requirements in the docker container.

Pytest is now running in the container, we have got two containers one for python3.12 and python3.11.

Somehow the test result is not picked up correctly. (I modified one test to fail). The badge is correct but the pipeline status is wrong. @schuenke I think you implemented this - could you have a look?

[fzimmermann89]

A question for my understanding: why do we need a requirements.txt at all?
Why cant we just requiere mrpro for binder and have pip also install all of mrpros dependencies?

[ckolbPTB]

A question for my understanding: why do we need a requirements.txt at all? Why cant we just requiere mrpro for binder and have pip also install all of mrpros dependencies?

You mean have a requirements.txt which only contains something like mrpro @ git+https://github.com/PTB-MR/mrpro? Afaik we can then only install the default packages but not the packages for the notebooks.

[fzimmermann89]

mrpro[notebook] @ git+https://github.com/PTB-MR/mrpro

in a requirements.txt results in a fresh python3.12 conda-forge environment in

Would install Jinja2-3.1.3 MarkupSafe-2.1.5 PyWavelets-1.5.0 PyYAML-6.0.1 Pygments-2.17.2 asttokens-2.4.1 attrs-23.2.0 build-1.0.3 certifi-2024.2.2 charset-normalizer-3.3.2 click-8.1.7 comm-0.2.1 contourpy-1.2.0 coverage-7.4.2 cycler-0.12.1 debugpy-1.8.1 decorator-5.1.1 einops-0.7.0 executing-2.0.1 fastjsonschema-2.19.1 filelock-3.13.1 fonttools-4.49.0 fsspec-2024.2.0 h5py-3.10.0 idna-3.6 ipykernel-6.29.2 ipython-8.22.1 ismrmrd-1.14.0 jedi-0.19.1 jsonschema-4.21.1 jsonschema-specifications-2023.12.1 jupyter_client-8.6.0 jupyter_core-5.7.1 jupytext-1.16.1 kiwisolver-1.4.5 llvmlite-0.42.0 markdown-it-py-3.0.0 matplotlib-3.8.3 matplotlib-inline-0.1.6 mdit-py-plugins-0.4.0 mdurl-0.1.2 mpmath-1.3.0 mrpro-0.0.1 nbformat-5.9.2 nest-asyncio-1.6.0 networkx-3.2.1 numba-0.59.0 numpy-1.26.4 nvidia-cublas-cu12-12.1.3.1 nvidia-cuda-cupti-cu12-12.1.105 nvidia-cuda-nvrtc-cu12-12.1.105 nvidia-cuda-runtime-cu12-12.1.105 nvidia-cudnn-cu12-8.9.2.26 nvidia-cufft-cu12-11.0.2.54 nvidia-curand-cu12-10.3.2.106 nvidia-cusolver-cu12-11.4.5.107 nvidia-cusparse-cu12-12.1.0.106 nvidia-nccl-cu12-2.19.3 nvidia-nvjitlink-cu12-12.3.101 nvidia-nvtx-cu12-12.1.105 packaging-23.2 parso-0.8.3 pexpect-4.9.0 pillow-10.2.0 pip-tools-7.4.0 platformdirs-4.2.0 prompt-toolkit-3.0.43 psutil-5.9.8 ptyprocess-0.7.0 pure-eval-0.2.2 pydicom-2.4.4 pyparsing-3.1.1 pyproject_hooks-1.0.0 pypulseq-1.4.0 python-dateutil-2.8.2 pyzmq-25.1.2 referencing-0.33.0 requests-2.31.0 rpds-py-0.18.0 scipy-1.12.0 sigpy-0.1.26 six-1.16.0 stack-data-0.6.3 sympy-1.12 toml-0.10.2 torch-2.2.1 torchkbnufft-1.4.0 tornado-6.4 tqdm-4.66.2 traitlets-5.14.1 typing_extensions-4.9.0 urllib3-2.2.1 wcwidth-0.2.13 wget-3.2 xsdata-22.12 zenodo-get-1.5.1

so all the notebook requirements are installed, without any pip compile etc..
The only "disadvantage" is, that the binder will always run with the latest downstream dependencies.

[ckolbPTB]

The only "disadvantage" is, that the binder will always run with the latest downstream dependencies.

pip-compile would have the same "problem".

The only advantage of pip-compile I see now is, that we can modify the requirements.txt such that it does not install the nvidia packages and only installs the cpu version of torch.

[fzimmermann89]

the advantage is pip-compile fixes the versions for binder to what they where when the tests ran last.

For binder

it will always install cuda if we install it from a requierements.txt, there is no way around it: pytorch from pypi defaults to cuda and will again bring in the cuda requirements if we don't install it with --no-deps. AFAIK binder installs dependencies of the requirements.txt. So even if we manually remove all cuda from the file, it will afaik we installed at binder.

For binder, it might therefore be better to provide a environment.yml which is also supported.
this can then include

- pytorch::pytorch
- pytorch::cpuonly
- pip:
   -mrpro[notebook] @ git+https://github.com/PTB-MR/mrpro

(untested)

to install only the requirements we specify in the pyproject.toml and cpu-torch.

For the rest

In test stuff, it should suffice to first install pytorch-cpu via pip and then mrpro.
In the docker we would install both during image creation

[ckolbPTB]

AFAIK binder installs dependencies of the requirements.txt. So even if we manually remove all cuda from the file, it will afaik we installed at binder.

In the requirements.txt we could specifiy to use the cpu version and then binder would also stick to it. But your suggestion with the yml file should also work.

[fzimmermann89]

ah, yes, in a binder/requierements.txt we can specify an --index-url for a requirement , even better!
I mixed it up with pyproject.toml where we cant specify an index...

[ckolbPTB]

Need to change Binder back to
...../mrpro.git/main?labpath=examples)
before merging

[ckolbPTB]

@fzimmermann89 I think the problem is related to this:
"github.Workspace and runner.Workspace don't point to container valid paths when executing inside a container job." actions/runner#2058

Maybe we can use GITHUB_WORKSPACE and RUNNER_WORKSPACE for run-notebook. This env variables should be correctly set.

[fzimmermann89]

@ckolbPTB do we have pip inside the docker?

[ckolbPTB]

@ckolbPTB do we have pip inside the docker?

yes, we use it in the github actions to install mrpro, e.g. python -m pip install .[notebook]

[fzimmermann89]

the issue was that in our container python3 does not point to the correctly set up python....
I "fixed" it by instead calling python in the notebook-run action...

[ckolbPTB]

the issue was that in our container python3 does not point to the correctly set up python....
I "fixed" it by instead calling python in the notebook-run action...

Thanks for finding that out. Yes, I only ensured that python points to the right python installation but not python3. Will fix this in the container.

I am struggeling to understand what you did to "fix" this and what the wrong python file had to do with a missing folder error message...

[fzimmermann89]

sooo.... there were MANY issues.

some of them with our .yaml, some of them with the run-notebook code...

I forked the run-notebook action, made the output dir configurable (defaulting to workspace). Before, it was writing some temp output to workspace, but the notebooks results only to temp.
I also updated all dependencies of run-notebook, switched it over to ncc, recompiled it.

THEN I noticed that it was calling python3 -m pip to install the dependencies. I changed it in the action to calling python instead.

This is why most of the debugging did not happen in this PR but in https://github.com/fzimmermann89/run-notebook/

[ckolbPTB]

Last issue (I think) is when to build the container images for the tests. I would suggest to build the container images and push them to dockerhub in a github action which runs whenever something is pushed to main. @fzimmermann89 @schuenke Does this make sense?

[fzimmermann89]

I didn't follow the news regarding docker registries but: Does docker hub allow enough pulls for us for free (at some point the introduced some limits) Does github packages have higher limits for public reps?

Rebuilding the images at push to main sounds good.

[ckolbPTB]

dockerhub offers 200 image pulls per 6 hours which should be enough. Nevertheless as they are moving more and more towards a paid service I will have a look into the Github container registry.

[fzimmermann89]

Nice!
If I understand this correctly, this is almost done:
-> It automatically creates and pushes docker images and all our actions use these.

@ckolbPTB Will you split the docker creation at some point into a docker.yaml file? Or does it have to be in the pytest one?
Can we do the docker image creation only on changes to main (and maybe PRs with changes to the dockerfiles)?

And in all other PRs always reuse the existing images?

[ckolbPTB]

Can we do the docker image creation only on changes to main

Yes, I will add this just before merging (if I don't forget) otherwise testing is difficult.

(and maybe PRs with changes to the dockerfiles)?

This is more challenging but I can have a look into it.

Will you split the docker creation at some point into a docker.yaml file? Or does it have to be in the pytest one?

If it is in one file then it is easy to tell the pytests to wait for the docker creation. At least when we push to main, will the tests wait for up-to-date dockerimages. Should anything break we will get an errormessage then. Otherwise we would only detect the errors in whichever PR runs the next tests.

[ckolbPTB]

@fzimmermann89 and @schuenke could you have another look at this and let me know if anything is missing/needs adapting?

The checks which are listed as "Expected" are defined in the repo-settings. I can only adapt the names to the new ones once this is merged into main.

[fzimmermann89]

TBH a lot of the github action stuff for me is voodoo and I can only do debugging by trial-and-error.

So as the action run through, there are only some minor comments I can do (see above).

Otherwise I would say we merge it after @schuenke gives his ok
Worst case is that we have to revert it later on if it causes issues.

[schuenke]

Looks good as far as I can tell 😀

As I mentioned before, if there are any remaining problems, we will probably only notice when the actions are being used.