PTB-MR / mrpro

MR image reconstruction and processing.
https://ptb-mr.github.io/mrpro/
Apache License 2.0
17 stars 2 forks source link

Python 3.10 support #461

Closed fzimmermann89 closed 3 weeks ago

fzimmermann89 commented 3 weeks ago

Change our code to support python3.10 again --> colab!

Import all typing stuff from typing_extension which backports typing features and in current python versions, reexports from typing.

Replace tuple unpacking in typehints by Unpack. This look a bit less nice but might be an ok trade-off. We can revert this commit if we want to drop python3.10 support again

Add a docker container for python 3.10

Refactor docker files: installing from github via clone will created chicken and egg problem as main was not installable in python 3.10. docker containers are now split in steps that might be cached.

Biggest difference is, our dependencies in the docker container will only update if we touch the pyproject.toml. If we want to, we can also update them on updates to VERSION, for example.

Closes #456

fzimmermann89 commented 3 weeks ago

Should be merged after #460

github-actions[bot] commented 3 weeks ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms/csm
   inati.py24196%44
   walsh.py16194%34
src/mrpro/algorithms/dcf
   dcf_voronoi.py53492%15, 48–49, 76
src/mrpro/algorithms/optimizers
   adam.py20195%69
src/mrpro/algorithms/reconstruction
   DirectReconstruction.py281643%51–71, 85
   IterativeSENSEReconstruction.py422345%77–78, 88–98, 113–124, 138–149
   Reconstruction.py522454%42, 54–56, 80–87, 104–115
src/mrpro/data
   AcqInfo.py128298%174, 214
   CsmData.py29390%15, 82–84
   DcfData.py45882%18, 66, 78–83
   IData.py67987%119, 125, 129, 159–167
   IHeader.py75791%75, 109, 127–131
   KHeader.py1651790%25, 127–131, 158, 208, 219, 226–227, 230, 237, 276–287
   KNoise.py311552%39–52, 56–61
   KTrajectory.py69593%178–182
   MoveDataMixin.py1271489%15, 110, 126, 140–142, 203, 266, 280, 359, 379–380, 397–398
   QData.py39782%42, 65–73
   Rotation.py4482295%97, 278, 363, 365, 392, 447, 452, 455, 470, 487, 492, 635, 640, 643, 659, 663, 737, 739, 747–748, 988, 1070
   SpatialDimension.py47296%65, 104
   TrajectoryDescription.py14193%23
   acq_filters.py10190%47
src/mrpro/data/_kdata
   KData.py1051685%108–109, 118, 126, 180–181, 216, 221–222, 241–252
   KDataRemoveOsMixin.py29293%44, 46
   KDataSelectMixin.py21290%48, 64
   KDataSplitMixin.py49394%51, 81, 90
src/mrpro/data/traj_calculators
   KTrajectoryCalculator.py25292%23, 45
   KTrajectoryIsmrmrd.py13285%41, 50
   KTrajectoryPulseq.py29197%54
src/mrpro/operators
   CartesianSamplingOp.py50982%49–50, 55–56, 61–62, 88, 91, 114
   ConstraintsOp.py60297%46, 48
   EndomorphOperator.py65297%228, 234
   FiniteDifferenceOp.py27293%40, 105
   FourierOp.py77199%131
   Functional.py71593%20–22, 117, 119
   GridSamplingOp.py136993%72–73, 82–83, 90–91, 94, 96, 98
   LinearOperator.py1611293%55, 91, 190, 220, 261, 270, 278, 287, 295, 306, 404, 409
   MultiIdentityOp.py13285%43, 48
   Operator.py78297%25, 74
   ProximableFunctionalSeparableSum.py39392%50, 103, 110
   SliceProjectionOp.py173895%44, 61, 63, 69, 206, 227, 260, 300
   WaveletOp.py120596%152, 170, 205, 210, 233
   ZeroPadOp.py16194%30
src/mrpro/utils
   filters.py62297%44, 49
   modify_acq_info.py17194%12
   slice_profiles.py46687%20, 36, 113–116, 149
   sliding_window.py34197%34
   split_idx.py10280%43, 47
   summarize_tensorvalues.py11918%20–29
   typing.py14843%8–18
   zero_pad_or_crop.py31681%26, 30, 54, 57, 60, 63
TOTAL407330992% 

Tests Skipped Failures Errors Time
1872 0 :zzz: 0 :x: 0 :fire: 1m 41s :stopwatch:
github-actions[bot] commented 3 weeks ago

:books: Documentation

:file_folder: Download as zip :mag: View online

fzimmermann89 commented 3 weeks ago

@ckolbPTB did you have a docker file for python3.10? I think we need stupid if case in the install script: image

fzimmermann89 commented 3 weeks ago

I am getting hit by a chicken and egg problem: During build of the docker image, mrpro cannot be installed with python 3.10 as we currently install from github main (and there the min version is python 3.11) This results in missing requirements later on This results in the tests not passing. So I have to split this into two PRs.

One PR will enable 3.10 compatibility Second PR will enable tests for 3.10

...or we would have to fix the docker logic, but this would also be a second PR as I want to keep this one only python3.10 related to be able to just revert it later on

Any other suggestions? @ckolbPTB @schuenke @JoHa0811 ?

fzimmermann89 commented 3 weeks ago

This should be merged as two commits to make undo of the python3.10 changes easier in the future.

schuenke commented 3 weeks ago

A few questions out of curiosity:

  1. Any idea why https://github.com/PTB-MR/mrpro/actions/runs/11451086815 is still running after ~6h ?
  2. Any idea why for the build of py310 and py311 the "Cached" value is ~7%, but for py312 ~28%? see here
  3. In general, splitting the docker build is a good idea imo. However, it seems like it's not resulting in a faster build yet, no? Would this change with the next run?
ckolbPTB commented 3 weeks ago

Seems to me a lot of work for something which is outdated and we will have to revert soon again. Would it not make more sense to try to overcome the problem of python 3.10 in colab? We are not the only ones complaining about this. Are there no options to upgrade python or run one of our containers in colab (maybe something like this: https://github.com/drengskapur/docker-in-colab)?

fzimmermann89 commented 3 weeks ago

Seems to me a lot of work for something which is outdated and we will have to revert soon again. Would it not make more sense to try to overcome the problem of python 3.10 in colab? We are not the only ones complaining about this. Are there no options to upgrade python or run one of our containers in colab (maybe something like this: https://github.com/drengskapur/docker-in-colab)?

There is no good way to use notebooks in a docker at colab.

When we moved to python 3.11, we wanted to get "Self" to work and I did not know about typing_extensions...

fzimmermann89 commented 3 weeks ago

A few questions out of curiosity:

  1. Any idea why https://github.com/PTB-MR/mrpro/actions/runs/11451086815 is still running after ~6h ?

I have to manually kill them or wait till they time out. There is a bug in pip that installing packages from '/' does not work and freezes. In docklerfiles, it is common to COPY to '/', run and remove afterwards. Took me a few tries to realize this is the issue.

  1. Any idea why for the build of py310 and py311 the "Cached" value is ~7%, but for py312 ~28%? see here

No idea. One possible difference is how far previous runs for this task got.

  1. In general, splitting the docker build is a good idea imo. However, it seems like it's not resulting in a faster build yet, no? Would this change with the next run?

It should. Here the pyproject.toml changed, which it will not do most of the time..

fzimmermann89 commented 3 weeks ago

image

fzimmermann89 commented 3 weeks ago
  1. Any idea why for the build of py310 and py311 the "Cached" value is ~7%, but for py312 ~28%? see here

No idea. One possible difference is how far previous runs for this task got.

  1. In general, splitting the docker build is a good idea imo. However, it seems like it's not resulting in a faster build yet, no? Would this change with the next run?

It should. Here the pyproject.toml changed, which it will not do most of the time..

Lets try adding a scope for each docker image (py10,py11,py12): https://docs.docker.com/build/cache/backends/gha/#scope

Scope is a key used to identify the cache object. By default, it is set to buildkit. If you build multiple images, each build will >overwrite the cache of the previous, leaving only the final cache.

To preserve the cache for multiple builds, you can specify this scope attribute with a specific name. In the following example, the >cache is set to the image name, to ensure each image gets its own cache:

GitHub's cache access restrictions, still apply. Only the cache for the current branch, the base branch and the default branch is >accessible by a workflow.

fzimmermann89 commented 3 weeks ago

This now also changed when dependencies are updated to newest versions. We do no longer update on each push to main all dependencies. Our normal PRs should not fail because a dependency is updated, this was a bit annoying in the past.

Instead, dependencies are updated if either the pyproject.toml is changed, or the VERSION is changed.

Long term, it would be great to get a dependabot-like action checking for requirements going.