SyneRBI / SIRF-SuperBuild

SIRF CMake SuperBuild
http://www.ccpsynerbi.ac.uk
Apache License 2.0
15 stars 17 forks source link

remove obsolete version restriction for SciPy #835

Closed KrisThielemans closed 10 months ago

KrisThielemans commented 10 months ago

temporarily addresses #834

KrisThielemans commented 10 months ago

This built ok, but it seems we're not running any CIL tests on the docker images, see https://github.com/SyneRBI/SIRF-SuperBuild/actions/runs/5867753842/job/15909452350?pr=835#step:6:25667 that isn't great.

KrisThielemans commented 10 months ago

we're not running any CIL tests on the docker images

This is due to #836.

@paskino @casperdcl @gfardell is there a way to test a conda install of CIL?

casperdcl commented 10 months ago

The CIL conda recipe does include a test key, so I guess yes... but presumably SIRF only needs to test the points of integration with CIL, not actually run the full CIL test suite.

KrisThielemans commented 10 months ago

Well, arguably if you put full CIL on our docker image, maybe it should be tested anyway, although I presume that if a conda build it is found&installed, it can be presumed to work?

But certainly, cross-CIL-SIRF tests are the most important. I'm not sure where they are now. Might need @paskino.

gfardell commented 10 months ago

Well, arguably if you put full CIL on our docker image, maybe it should be tested anyway, although I presume that if a conda build it is found&installed, it can be presumed to work?

We build and test all the binaries we push, this broadly tests numpy and python versions combinations. But we don't have version caps on our dependencies, so when they are updated, conda starts resolving with the newer version and issues can appear due to backward incompatibilities. I think we've had this with SciPy and matplotlib in the past.

To run the tests you need the extra dependencies in the conda test requirements that @casperdcl pointed out, but the tests aren't packaged in to the conda build (as far as a recall!) so you'd have to get them from our repo.

KrisThielemans commented 10 months ago

@gfardell or anyone else, could you write a small script for CIL testing of an existing installation (i.e. clone, install dependencies, run tests, clean-up). Adding a call to that in the Dockerfile would be reassuring.

casperdcl commented 10 months ago

alternatively... CIL could test SIRF? https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#downstream-tests

paskino commented 10 months ago

Integration tests with SIRF and CIL are in the CIL test suite https://github.com/TomographicImaging/CIL/blob/master/Wrappers/Python/test/test_SIRF.py

They are not run in CIL CI as we don't have SIRF. They should be run when BUILD_CIL=ON but they have been commented out because they used to fail. I believe I fixed that recently.

https://github.com/SyneRBI/SIRF-SuperBuild/blob/50f8a96e45ef6235e46bd237b534a95d122058b1/SuperBuild/External_CIL.cmake#L98-L100

paskino commented 10 months ago

@casperdcl can we easily extract the test requirements of CIL from the meta.yaml?

paskino commented 10 months ago

@gfardell or anyone else, could you write a small script for CIL testing of an existing installation (i.e. clone, install dependencies, run tests, clean-up). Adding a call to that in the Dockerfile would be reassuring.

Isn't it preferrable to BUILD_CIL=ON also in Docker? This would simplify this and close #836 after #837

paskino commented 10 months ago

I just had a look at the Docker files. I don't think that CIL is installed at all on our docker images, see requirements.yaml and requirements-service.yml

It is installed on the jupyterhub image via conda.

KrisThielemans commented 10 months ago

I just had a look at the Docker files. I don't think that CIL is installed at all on our docker images, see requirements.yaml and requirements-service.yml

It is now installed via SIRF-Exercises/environment.yml, done in #821 at my request...

Isn't it preferrable to BUILD_CIL=ON also in Docker? This would simplify this and close #836 after #837

I think we have a problem with dependencies as we mix conda and hand-build stuff. If we do BUILD_CIL=ON, I believe that we will still install it via conda as well, due to the above.

It's of course a mess, which #821 probably made larger. The best way around it is to do everything via conda (but we cannot), or nothing... I suggest we merge this and either do

casperdcl commented 10 months ago

can we easily extract the test requirements of CIL from the meta.yaml?

yez :)

pip install yq
sed s/{{.*}}/./g CIL/recipe/meta.yaml | yq -r .test.requires[]