Build project with Scikit-build-core
Description
Depends on https://github.com/TomographicImaging/CIL/pull/2117 Remaining items to address:
- [x] Convert linked library to a c-extension with
nanobind. - [x] Build c-extension using
scikit-build-core - [x] Remove ctypes scaffolding used in python files for the linked library, since
nanobindrenders that unnecessary.- [x] Removes ctypes scaffolding directly(but leaving extra variables in place)
- [ ] Remove temporary variables from ctypes scaffolding
- [ ] Manage dependencies using
pyproject.toml- [ ] Add
tigreandtomophantomoptional dependency(Sincetigreisn't on PyPI, I deferred figuring out how to reference it via a git repo) - [x] Use PyPI external libraries for
ippandopenmp. - [x] Fix cmake files to use these libraries correctly.
- [ ] Add
- [x] Add CI for uv
- [x] Try and switch back to running tests with unittests, or figure out how to get pytest working with unittest skips correctly
- [ ] Autogenerate
version.pyagain, or decide whether to update versions directly thoughpyproject.toml - [ ] Return original behavior for the data directory used for tests(instead of using an environment variable)
- [ ] Fix conda build, as that was somewhat gutted.
- [ ] Add back in optional building for
IPPif desired - [ ] Add instructions to developer guide
This is the next step in https://github.com/TomographicImaging/CIL/issues/1875.
I got a bit over excited and decided to get a decent scaffold up for this before waiting for approval.
I attempted to not move anything around; I believe this could look much cleaner in terms of project structure if we were to do that(using uv's workspace feature in particular), but I decided not to do that for now to minimize the diff(not that it's a small diff).
I attempted to make the commits here somewhat logical, but I didn't do a great job of it, so I'll summarize what I did.
First, I set up the dependencies of the project in pyproject.toml and set up the project to be built with uv, with the goal of being able to use uv build.
Next, I converted the cpp code to a C-extension by using nanobind and taking out the DLL exports.
Next, I hacked on the CMake files until I was able to get the C-extension to build. I did this without regard to keeping anything in for the conda build step, so I will probably need to return that if you want to keep that build path, but this was easier to get something functional :).
This broke a bunch of the code since there was some scaffolding for interfacing with the DLL exports in python, and nanobind doesn't seem to preserve pointer arguments for c-types, so next, I changed all the pointers to the nanobind::ndarray type, and then removed all the ctype scaffolding.
Running pytest, I've been able to get all the tests to pass, though I had to do some local overrides for the data paths to do so. After doing that, I've gotten most of the tests to pass. Nearly all of the failed tests are from the fact that I haven't set up any of the plugins yet(I don't actually own an NVIDIA GPU so I can't test tigre at all anyway). A bunch of tests were skipped for reasons I'm unsure of which maybe someone can give insight on.
Here's a summary of the tests right now:
What remains to be done is for big steps are:
- Setting up the tests to work with this project structure, particularly with the data path.
- Setting up the plug in support
There were 2 tests that failed due to having slightly wrong numbers:
I'm unsure what is causing this, but the numbers are not very far off from what they are supposed to be.
Example Usage
If you'd like to try using this, I think you can just install uv and run uv build to build a wheel, and uv sync to create a virtual environment, as long as the right libraries are installed. There's still work to be done but I wanted to get this in front of someone for feedback before continuing(I didn't even mean to get this far before doing that, but I guess i was too excited about it).
Changes
Testing you performed
Please add any demo scripts to https://github.com/TomographicImaging/CIL-Demos/tree/main/misc
Related issues/links
Checklist
- [ ] I have performed a self-review of my code
- [ ] I have added docstrings in line with the guidance in the developer guide
- [ ] I have updated the relevant documentation
- [ ] I have implemented unit tests that cover any new or modified functionality
- [ ] CHANGELOG.md has been updated with any functionality change
- [ ] Request review from all relevant developers
- [ ] Change pull request label to 'Waiting for review'
Contribution Notes
Please read and adhere to the developer guide and local patterns and conventions.
- [ ] The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License
- [ ] I confirm that the contribution does not violate any intellectual property rights of third parties
--->
Also, there is some simplification that can be done in terms of variables that I didn't do when removing the ctypes conversion, mostly because I wanted to make sure not to break anything unintentionally.
This looks great @purepani we'll take a look at it in our next developer meeting.
The unit tests will skip if you don't have the optional dependancies rather then fail, so you'll see many skipped due to not having a GPU etc.
Thanks! I don't quite have an nvidia GPU set up that I can use for this; would it be possible to test with CI? Obviously some CI would have to be set up since this is a new build system but I don't mind doing that to for testing!
There's something that stops uv build from working outright in machines that aren't mine(particularly in CI), so I need to solve that before proceeding
I have fixed the issue mentioned earlier, did some cleanup, and added a new CI file. I made some changes to the test files to get them to actually run on my machine(particularly, relative imports for the test files). For whatever reason, I wasn't able to get that to work with the unittest commands used in the current CI, so I used pytest for the uv CI. I can try to change it back later, but I wanted to just get it to a point that we can at least run the tests. I think due to one or both of those changes, some of the gpu tests fail instead of skip with some variation of error
| FAILED Wrappers/Python/test/test_PluginsTigre_interpolated.py::TestCommon_ProjectionOperator::test_backward_projector - AttributeError: 'TestCommon_ProjectionOperator' object has no attribute 'ag'
so trying to change it back is probably necessarily.
I also made the data directory for tests use an environment variable for similar reasons.
And have also manually included the version.py file instead of auto-generating it.
I added a checklist at the top post of things left to do. Let me know if we can run the tests in build_uv.yml!
Oh, I just noticed it looks like the 2 tests that were failing earlier due to precision are fine now! I have no idea what commit fixed that, but I guess no longer need to worry about that! Failing tests are now down to 34 for non-gpu tests, and they're all tests that should only be gpu tests. I'll see if i can get unittest working correctly.
I fixed the tests by removing the relative imports. I realized there was no reason to actually have the relative imports except for working with pytest(which for some reason I though you used until a few days ago). I would now expect all the tests to fully pass.
Had a dumb typo for the gpu tests; could they be run again?
It looks like the ASTRA tests work correctly. I removed the FORCE_GPU variable from CI, because it would otherwise stop the tests for ASTRA from running since I don't have tigre set up yet.
Now some technical questions: At the moment, neither TIGRE or TomoPhantom are able to be added to pyproject.toml immediately in the same way astra is(through optional dependencies).
The following paths are possible:
A. work with TIGRE and TomoPhantom to get their pyproject.toml to a state where I can just add them into pyproject. They wouldn't need to upload to PyPI, though that would be ideal. For TIGRE, this looks easy, but TomoPhantom looks a bit more involved. If we do this then pip install cil[tigre] and pip install cil[tomophantom] would work in the same way that it currently works with ASTRA.
B. Tell users to manually install tigre and tomophantom, building from source, and doing the same in CI.
C. Drop tigre and tomophantom.
C is almost certainly not an option(I would assume), and B seems pretty undesirable, so I'll get to working on A, unless there are better ideas.
Actually, I might try and split this into 2 PRs: one to convert to a c-extension, and then another to allow for building as a wheel for python packaging.
It'll be a bit more work for me, but I think it makes more sense to do it that way since those 2 steps are somewhat orthogonal(and will probably be easier for you to review individually).
Ok, it looks like it would be way easier to just replace the full build system with scikit-build-core trying to figure out how to install c-extensions manually within cmake.
I've gotten it down to 2 failed tests and 15 skipped tests for the GPU. I'm not quite sure why the failed tests are failing. At the moment, TIGRE is being compiled manually from this PR, since otherwise it failed to install. I've also started to clean up some of the commits(though obviously haven't gotten through everything yet), to hopefully make it easier to review.