Add safe Python-extension module C API headers#15762
Conversation
|
One or more of the following people are relevant to this code:
|
2a1471a to
62752e0
Compare
Pull Request Test Coverage Report for Build 22786547376Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| // We rely on `qiskit.h` to `#include <Python.h>` on our behalf, since it needs to be included | ||
| // before all other includes. (Though really, a user should also have included it themselves, | ||
| // surely, if they're delcaring a Python extension module.) |
There was a problem hiding this comment.
I assume Python.h has an include guard, so why not just include it here anyways?
There was a problem hiding this comment.
Because it has to be included before all other includes - if we put it here, it must be redundant because this isn't a top-level import.
| if (!_Qk_API_QI) | ||
| return -1; | ||
|
|
||
| // TODO: any validity checks on the version of the Qiskit API? |
There was a problem hiding this comment.
Could you explain what kind of checks you're thinking of? Since the vtables and functions are all autogenerated they should be consistent, right? Does this refer to coherence checks in case the generation goes wrong?
There was a problem hiding this comment.
Oh, I forgot this was still here. The check I have in mind is a runtime version check; the compiled extension knows what header it's compiled with (it's the version info in there) but at runtime, it's got no way of knowing what version the library is. We're supposed to keep our ABI/API safe and forwards compatible, but just for forwards thinking, I'm thinking we should maybe have a version handshake during the import, so we can insert backwards-compatibility shims if we need to, or more basically just warn if there are known incompatibilities.
There was a problem hiding this comment.
I might want to squeeze in a qk_version() function into the release just so we can put it at slot number 0 and have it available in the future, but there's no need to do anything with it in the header file in 2.4.0rc1, I think.
There was a problem hiding this comment.
#15831 is the qk_api_version function, though I haven't made the header files for 2.4 check it here. I still would like to get it in to give us the option in the future, though.
62752e0 to
d6c1640
Compare
|
I've just rebased the PR here to make the diff clean, but I need to come back later this evening and write the commit message and the release note. |
With the vtables now compiled into the `_accelerate` object and stored in suitable `PyCapsule`s, the last step to exposing the complete ability to compile Python extension modules is providing header-file support for actually using the result. This support is modelled on NumPy. We generate alternate versions of the function declarations as part of the `pyext` build script, which are loaded (instead of the standard function prototypes) when the `QISKIT_PYTHON_EXTENSION` macro is set prior to the inclusion of `qiskit.h`. These declarations are all pre-processor macros that resolve to compile-time constant offset lookups into the vtables stored in the `PyCapsule`s, except we cache the internal pointer of each `PyCapsule` into a compilation-unit-local `static`. If we didn't have this cache, _all_ function calls would have Python-API overhead and require an attached Python thread state (holding the GIL). The cache population is done by a new header-only function `qk_import` defined in (the non-stub version of) `funcs_py.h`, which then must be called _before_ any C API function. This will almost invariably be done inside the `PyInit_*` module-initialisation function of the extension. The cache mechanism introduced in this commit is local to a single translation unit. It is possible to extend this to allow sharing it between different translation units, but since this necessarily requires exposing a non-`static` symbol out of a library, we will have to take care to do it with a mechanism that allows the user to override the names used.
d6c1640 to
6ecaa68
Compare
|
Ok, the release note and commit message are now all written, so this should be good to review. |
mrossinek
left a comment
There was a problem hiding this comment.
I found mostly typos. Other than that, I think this looks good 👍
| } | ||
| ir::Type::Path(p) => acc.push_str(p.export_name()), | ||
| ir::Type::Primitive(ty) => acc.push_str(ty.to_repr_c(config)), | ||
| ir::Type::Array(..) => todo!("array types not yet handled"), |
There was a problem hiding this comment.
How likely are these to come up in the future? Do you want a (or more) tracking issue(s) for this as well as the other not-handled cases below?
There was a problem hiding this comment.
Really pretty unlikely I think - minimum-size arrays as function arguments in C are super uncommon. I put in the panic so that if we ever do try it, it's marked as needing extra implementation.
raynelfss
left a comment
There was a problem hiding this comment.
I again couldn't find anything concerning here. I assume this isn't being used yet as we haven't designed any Python extensions yet but the logic seems to make sense.
From what I could read, based on usage of an extension definition, this module will first import qiskit and use the vtable to generate the functions header file.
That said, I only had one question about a check
| ty, | ||
| array_length, | ||
| } = arg; | ||
| assert!(array_length.is_none(), "array arguments not handled"); |
There was a problem hiding this comment.
Is this supposed to be array_length.is_some() or am I reading this wrong?
There was a problem hiding this comment.
The assertion is that the argument isn't an array - if it is, there's a problem because we explicitly don't handle that case. I didn't want the potential bug to pass silently.
It's not used within the Qiskit/qiskit repository itself yet, but I will follow up adding integration tests involving it. There's just more repo admin work to do about that because I'll most likely need/want a more powerful/less wildly over-opinionated command runner than
Well, the header file is generated in the build script of |
raynelfss
left a comment
There was a problem hiding this comment.
Thank you for quickly addressing my comments and explaining how things work here. I don't have much more to add, so feel free to merge this after @alexanderivrii takes a look
With the vtables now compiled into the
_accelerateobject and stored in suitablePyCapsules, the last step to exposing the complete ability to compile Python extension modules is providing header-file support for actually using the result. This support is modelled on NumPy.We generate alternate versions of the function declarations as part of the
pyextbuild script, which are loaded (instead of the standard function prototypes) when theQISKIT_PYTHON_EXTENSIONmacro is set prior to the inclusion ofqiskit.h. These declarations are all pre-processor macros that resolve to compile-time constant offset lookups into the vtables stored in thePyCapsules, except we cache the internal pointer of eachPyCapsuleinto a compilation-unit-localstatic. If we didn't have this cache, all function calls would have Python-API overhead and require an attached Python thread state (holding the GIL).The cache population is done by a new header-only function
qk_importdefined in (the non-stub version of)funcs_py.h, which then must be called before any C API function. This will almost invariably be done inside thePyInit_*module-initialisation function of the extension.The cache mechanism introduced in this commit is local to a single translation unit. It is possible to extend this to allow sharing it between different translation units, but since this necessarily requires exposing a non-
staticsymbol out of a library, we will have to take care to do it with a mechanism that allows the user to override the names used.Depends on:
TargetandDAGCircuit#15337QkObsborrowing from Python-spaceSparseObservable#15752qiskit.capimodule #15711pyext#15761Close #15572