inducer / pyopencl

OpenCL integration for Python, plus shiny features
http://mathema.tician.de/software/pyopencl
Other
1.04k stars 237 forks source link

use scikit-build-core #755

Closed matthiasdiener closed 4 weeks ago

matthiasdiener commented 1 month ago

This appears to fix #753.

TODO:

Please squash

alexfikl commented 1 month ago

If we want to switch, why not meson-python instead? The numpy / scipy / matplotlib ecosystem seems to have gone that way and it's probably a good idea to follow..

matthiasdiener commented 1 month ago

If we want to switch, why not meson-python instead? The numpy / scipy / matplotlib ecosystem seems to have gone that way and it's probably a good idea to follow..

TIL, thanks! I think the main reason for using scikit-build-core is that it is what nanobind recommends, and it's used in their examples.

matthiasdiener commented 1 month ago

This seems to work fine and is ready for a first look @inducer. I'm not sure what the best way to replace/reimplement the configure.py functionality is.

alexfikl commented 1 month ago

This seems to work fine and is ready for a first look @inducer. I'm not sure what the best way to replace/reimplement the configure.py functionality is.

What functionality exactly? Generally the configuration can be done in CMakeLists.txt and you can pass it arguments using

pip install . --config-settings='cmake.args=-DSOME_DEFINE=ON;-DOTHER=OFF'
matthiasdiener commented 1 month ago

What functionality exactly? Generally the configuration can be done in CMakeLists.txt and you can pass it arguments using

More specifically, I meant an easy-ish way for the user to configure the build, similar to the configure.py / siteconf.py system in the current version.

pip install . --config-settings='cmake.args=-DSOME_DEFINE=ON;-DOTHER=OFF'

Yeah, that is an option for replacing configure.py. Additionally, I was thinking of providing a siteconf.cmake file similar to siteconf.py.

inducer commented 1 month ago

I think ditching the configure.py mechanism is the right approach at this point. As long as we document the cmake.args way (also maybe add an entry to the changelog), I think that's OK.

matthiasdiener commented 1 month ago

Here are the OpenCL config options that we have been discussing (all except nr. 4 should work with the current state of this PR):

  1. Get path from CONDA_PREFIX if set
  2. siteconf.cmake
  3. Environment variables
  4. Having some kind of configure.py-like frontend (that calls pip install with the right options?)
  5. pip install . --config-settings='cmake.args=-D....'
  6. Fallback: let's just have cmake try to find OpenCL

If I understood correctly, option 4 isn't really desired. Are there any other options we should add/remove? (e.g., perhaps sieconf.cmake isn't necessary?)

Besides the question above, this is ready for a second look (in particular, regarding the next steps).

matthiasdiener commented 1 month ago

This is ready for review @inducer/@alexfikl.

inducer commented 1 month ago

I've pushed the branch to Gitlab to check CI there (added a link to the description for easy access). While main currently doesn't pass on Gitlab either :cry:, there are some issues there that might merit a look, particularly the downstream failures.

matthiasdiener commented 1 month ago

I've pushed the branch to Gitlab to check CI there (added a link to the description for easy access). While main currently doesn't pass on Gitlab either 😢, there are some issues there that might merit a look, particularly the downstream failures.

I think the 'downstream' Gitlab tests should be fixed now ('examples'/'K40' tests fail with main on Gitlab too), but I don't see a way to retrigger the CI run on Gitlab.

matthiasdiener commented 1 month ago

Not sure what's going on with the boxtree test (failure seems to be spurious?), but gitlab tests seem to be fixed.

inducer commented 1 month ago

We can't really get a clean test run on Gitlab because of https://github.com/inducer/pytools/issues/227, so I'll put this on hold until we can.

inducer commented 1 month ago

Nvm, we can, now that the release is yanked.

inducer commented 4 weeks ago

LGTM. Thanks for making this happen. Let's do it!