Conversation
|
One or more of the following people are relevant to this code:
|
4b0dea6 to
1f1e4cd
Compare
mrossinek
left a comment
There was a problem hiding this comment.
At a cursory glance this all makes sense to me thus far 👍
Will give a more thorough look next week!
mrossinek
left a comment
There was a problem hiding this comment.
I just reviewed 843b4bf to isolate the changes made by this PR from the current base.
This all seems great, I'm just adding some minor suggestions/questions.
I assume this is blocked by a working solution to get merged and released into setuptools-rust?
6bab43a to
c484a13
Compare
Yeah, basically. It's not 100% hard blocked on that because we could release using the build dependency on my branch of (I think there is - I could patch the |
This adds a basic `qiskit.capi` module for Python-space interaction with the C API. The two initial functions included are `get_include` and `get_lib`, to get the package include directory and shared-object library containing the C-API exported symbols, respectively. Note that while the included header files _are_ complete information to build against the C API, the file returned from `get_lib` is not generally useful as part of a _build_ system, but can be used to interact with the C API live through a Python session. I hope to follow this patch with two more: * provide a second "mode" for the packaged header files that allows building safe Python extension modules against the C API. This would mean that all the C API function names will be `#define`'d to function pointers looked up in a static table stored inside a `PyCapsule` that is initialised as part of `_accelerate`'s initialisation. * generate a `ctypes` wrapper file for the C API as part of the `_accelerate` (`pyext`) build script, which would expose all of the C API functions directly to Python space, potentially allowing them to be jitted through Numba, or allowing direct tests of C API functionality (without the manually-written file currently in our test suite). Co-authored-by: Max Rossmannek <max@rossmannek.de>
c484a13 to
0f3ebb5
Compare
|
I think I'll have to revisit this and put in temporary infrastructure to do a That's ok, I think - we can work around it. |
We need PyO3/setuptools-rust#574 (or something akin to it) to handle the movement of the generated files from the build script to the output. This is not yet ready for merge, and is highly unlikely to be ready in time for the Qiskit 2.4.0 release (or at least 2.4.0rc1), so to avoid blocking ourselves, we vendor in a monkey-patching reimplementation of the code temporarily. The allowed range of `setuptools-rust` is _probably_ larger than the hard pin, but the monkey-patch is so invasive and v1.12.0 is sufficiently old (August 2025) that it's not worth the risk. There are no meaningful licensing concerns since the vendored code here was entirely written by me both here and in the `setuptools-rust` patch; there is no cross-contamination from that repository and no code inclusion from it. This commit should be reverted as soon as we can swap to a safe released version of `setuptools-rust`.
0f3ebb5 to
d3d2e54
Compare
|
It's unlikely that PyO3/setuptools-rust#574 will merge in time, so I've updated this PR to make a horrendous but self-contained monkeypatch to a known-good version of Doing that unblocks this chain of work - if the |
Cryoris
left a comment
There was a problem hiding this comment.
Essentially LGTM, I left some small comments below
|
|
||
| LIB_PATH = qiskit._accelerate.__file__ | ||
| LIB = ctypes.PyDLL(LIB_PATH) | ||
| LIB = ctypes.PyDLL(qiskit.capi.get_lib()) |
There was a problem hiding this comment.
Nice - we should remember to update the guides with this
There was a problem hiding this comment.
Well not really - you basically should never be doing this. I do have to update the guide, but with how to build an actually safe C extension, and that doesn't involve qiskit.capi.get_lib() at all.
There was a problem hiding this comment.
(Using ctypes is like the one exception where it makes sense to use the _accelerate object loaded live.)
There was a problem hiding this comment.
-> which is what the guide is doing: https://quantum.cloud.ibm.com/docs/en/guides/c-extension-for-python 🙂
There was a problem hiding this comment.
No it isn't, at least not related to get_lib() - the guide is describing how to make a ctypes binding to you own correctly-exposed module. It's the "Build" section below that one that discusses _accelerate, where this method would be relevant, but that's the place that's wildly unsafe and needs to be completely rewritten.
|
The wheel-build failures are unrelated and should be fixed by #15791. |
mrossinek
left a comment
There was a problem hiding this comment.
Nice, Julien caught a lot more typos than I did 😅
Do you already want to open an issue to track the removal of the horrendous monkeypatch in setup.py once the setuptools-rust feature lands?
|
Max: the "messsages" typo was only introduced after you reviewed haha. I can open an issue after this merges, assuming it merges in this form. I'm not likely to forget, though - it's already my PR I'm waiting on, and I wrote it specifically for this purpose! |
Cryoris
left a comment
There was a problem hiding this comment.
LGTM thanks for the explanations, too!
This new module was added in Qiskit/qiskit#15711
This new module was added in Qiskit/qiskit#15711
This adds a basic
qiskit.capimodule for Python-space interaction with the C API. The two initial functions included areget_includeandget_lib, to get the package include directory and shared-object library containing the C-API exported symbols, respectively.Note that while the included header files are complete information to build against the C API, the file returned from
get_libis not generally useful as part of a build system, but can be used to interact with the C API live through a Python session.I hope to follow this patch with two more:
provide a second "mode" for the packaged header files that allows building safe Python extension modules against the C API. This would mean that all the C API function names will be
#define'd to function pointers looked up in a static table stored inside aPyCapsulethat is initialised as part of_accelerate's initialisation.generate a
ctypeswrapper file for the C API as part of the_accelerate(pyext) build script, which would expose all of the C API functions directly to Python space, potentially allowing them to be jitted through Numba, or allowing direct tests of C API functionality (without the manually-written file currently in our test suite).Summary
Details and comments
Close #15609.
Currently built on
#15679 anda PR tosetuptools-rust(PyO3/setuptools-rust#574).