WIP: Expose QkObs borrowing from Python-space SparseObservable#15752
WIP: Expose QkObs borrowing from Python-space SparseObservable#15752jakelishman wants to merge 2 commits intoQiskit:mainfrom
QkObs borrowing from Python-space SparseObservable#15752Conversation
e514235 to
6e9f54d
Compare
eliarbel
left a comment
There was a problem hiding this comment.
I tested it with a toy Python c-extension (managed to avoid writing something like print!("{}", "%d", num_qubits, end="\n") 😉). I mostly tested the C API in a sequential code setting, in which it seems to be working as expected. The tests validated things like borrowing (and mutating) from Python, creating and returning to Python, locking for read/write, blocking on a locked object, failing on try-lock on a locked object etc. I plan to test this a bit further with Python multithreading, but I feel confident already that for the basic use-case the implementation is good.
Some general comments:
- We definitely need a way to test this, or at least the locking aspect in a more general way (that is, not necessarily thorough a particular API like
QkObs's). Maybe we should have something like a dummy C Extension module which which runs outside of the regular unit testing, like in the vein of the QPY compatibility testing? - Regarding the
try_lockAPIs, how about extending the regular lock functions with a parameter, e.g. something like:
pub unsafe extern "C" fn qk_obs_lock_read_from_python(
ob: *mut ::pyo3::ffi::PyObject,
lock: *mut *mut ObsGuard,
blocking: bool,
) -> *const SparseObservable {and return NULL if locking fails?
- We're missing code examples for the new Python-binding C API functions. I realize it's tricky because there are several approaches for building C extensions for Python, so I don't have a good answer.
In addition I've also left a few concrete inline comments and questions.
| } | ||
|
|
||
| /// A lifetime-erased read guard held against a given `RwLock`. | ||
| pub struct ReadGuard<T: 'static> { |
There was a problem hiding this comment.
To be in line with the enum and CGuard, maybe call this CReadGuard (and the write one CWriteGuard)?
There was a problem hiding this comment.
This shouldn't actually ever be accessible or even visible in the C header file (unless I've made a mistake) because it's a private-only implementation detail of the Rust backing. Fwiw, we use the C prefix on C-exposed things as a sort of dummy for cbindgen because it's purely syntactical and can't resolve namespaces, so we give C-exposed symbols uglier names, since we're going to rename them to QkName stuff anyway.
| address: *mut ::std::ffi::c_void, | ||
| ) -> ::std::ffi::c_int { | ||
| // SAFETY: per documentation, we are attached to a Python interpreter, `ob` points to a valid | ||
| // Python object and `address` points to anough space to write a pointer. |
There was a problem hiding this comment.
| // Python object and `address` points to anough space to write a pointer. | |
| // Python object and `address` points to enough space to write a pointer. |
| /// `lock` must be exactly the return value of one of the `QkObs` Python-space locking functions, | ||
| /// and must not have been released before. | ||
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn qk_obs_release_lock(lock: *mut ObsGuard) { |
There was a problem hiding this comment.
Why is this not part of the py module?
There was a problem hiding this comment.
I think I left it out here because it doesn't actually depend on access to the Python symbols or libraries in any way, so it didn't need protecting in the header file. It might not be useful without Python, but I don't think there's a harm in failing to declare it.
| /// and must not have been released before. | ||
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn qk_obs_release_lock(lock: *mut ObsGuard) { | ||
| // SAFETY: per doecumentation, `lock` points to valid owned data returned by a locking function. |
There was a problem hiding this comment.
| // SAFETY: per doecumentation, `lock` points to valid owned data returned by a locking function. | |
| // SAFETY: per documentation, `lock` points to valid owned data returned by a locking function. |
| /// The caller must be attached to a Python interpreter. Behavior is undefined if `ob` is | ||
| /// not a valid non-null pointer to a Python object. | ||
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn qk_obs_borrow_from_python( |
There was a problem hiding this comment.
I'm wondering if we need this and qk_obs_convert_from_python, given the qk_obs_*lock* functions.
If there is a use-case for having both variants, we should be clearer about semantics of using this along with the *lock* functions and vice versa. For example, using this function on a locked object makes it behave like a try-lock function which returns NULL on failure.
There was a problem hiding this comment.
You can certainly get the same functionality as this by using the locking functions, but I included these as a convenience for C APIs that do not expect to be called in threaded or aliasing contexts at all - it's still safe, it just raises a Python exception in situations that actually can be handled if you use the more complex settings.
The difference here roughly is that it's a non-blocking function, but rather than returning a "wait" status, it sets the Python exception state.
This is a minimal set of the Python-conversion methods for `SparseObervable`, which do not currently handle the locking structure. For symmetry with all other Python conversion methods, `qk_obs_to_python` is swapped to take ownership of the object. This avoids an unnecessary copy-out step. The old behaviour can still be achieved by transferring a manual clone's ownership to Python space. The borrow-from-Python methods do not handling the locking structure in this commit, but will follow in a later patch.
Borrowing `QkObs` from the Python pointer is particularly tricky because the Python-space object has internal shared ownership and runtime read/write locking. _At present_ the locks should never fail to acquire (as long as we aren't accidentally cloning a `PySparseObservable` anywhere) because there's no situation where Python space can hold multiple views onto the same backing observable, but we do expect that to become possible in the future. More importantly, we expect that to happen with `PyCircuitData` and `PyDAGCircuit` in order to expose control-flow blocks to Python space without having to copy them out all the time; control-flow blocks will all be shared ownership with their containing circuit and the Python-space views. Here, we introduce several locking primitives for use with `QkObs`, but intended to be re-used when `PyCircuitData` and `PyDAGCircuit` similarly involve internal shared ownership. --- WIP because I haven't written any tests yet, or otherwise verified in any way that this actually works! I also am not at all convinced about the `try_lock` APIs I've written - I want to come back to those and see if there's a better way to indicate the locking failure without: - using `ExitCode` - requiring C to initialise a non-null pointer `QkObsGuard *lock;` before calling one of the functions - forcing the caller to call `PyErr_Occurred()` If I have to go for any of them, I'm likely to go for the last one (instead of the first one, which this commit currently has).
6e9f54d to
d616f7e
Compare
|
Following discussion with Eli, I've opened #15821 to split off the simpler components of this into a separate PR that will be easier to merge for 2.4, and deferred this one to 2.5. |
Borrowing
QkObsfrom the Python pointer is particularly tricky because the Python-space object has internal shared ownership and runtime read/write locking. At present the locks should never fail to acquire (as long as we aren't accidentally cloning aPySparseObservableanywhere) because there's no situation where Python space can hold multiple views onto the same backing observable, but we do expect that to become possible in the future. More importantly, we expect that to happen withPyCircuitDataandPyDAGCircuitin order to expose control-flow blocks to Python space without having to copy them out all the time; control-flow blocks will all be shared ownership with their containing circuit and the Python-space views.Here, we introduce several locking primitives for use with
QkObs, but intended to be re-used whenPyCircuitDataandPyDAGCircuitsimilarly involve internal shared ownership. For consistency withDAGCiruitandTarget, I also included the simple-caseqk_obs_borrow_from_pythonin cases where the caller can know that the observable is unique (or is prepared to accept a Python exception in those situations).Summary
Details and comments
Built on #15337
WIP because I haven't written any tests yet, or otherwise verified in any way that this actually works!
I also am not at all convinced about the
try_lockAPIs I've written - I want to come back to those and see if there's a better way to indicate the locking failure without:ExitCodeQkObsGuard *lock;before calling one of the functionsPyErr_Occurred()If I have to go for any of them, I'm likely to go for the last one (instead of the first one, which this commit currently has).