C API for SparseObservable
#13694
C API for SparseObservable
#13694
Conversation
- avoid using &Vec - add unsafe to mutable indices/boundaries - rm upgrade reno
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Cryoris
added
the
Rust
- use Arc<> instead of Py<> in ArrayView - py_sparse.. -> sparse_
…a into py-sparse-observable
Pull Request Test Coverage Report for Build 13593781557Warning: 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 |
some of this might need to be updated once we add the Docs
Cryoris
added
the
Changelog: New Feature
|
One or more of the following people are relevant to this code:
|
There was a problem hiding this comment.
Thanks to both of you for all this work.
I wouldn't say this is a fully in-depth review - I didn't even attempt to build it locally myself yet - I just wanted to start getting some questions out there to move it along. I will pull it down locally in the coming days and try to do some local playing myself, which hopefully won't lead to too many further questions.
I wouldn't pretend to have much familiarity with CMake or really making large-scale C projects at all, so at least some of my review on those stuff is just going to be vibes and not knowledge - I'll ask some questions, but you guys probably have more experience from looking into it than I do, so they're just questions.
I wouldn't mind a bit of name bikeshedding on the rewrite rules, if anyone's up for it: I've always been partial to the CPython API conventions of <Project><Type>_<Function> for functions where <Function> is PascalCase, e.g. PyList_New (c.f. QkObs_New) - it's a little less noisy than full snake_case, which matters more when there's no module-based namespacing in C. If we're using obs as the prefix for SparseObservable-related functions, I like the idea of the class just being QkObs (and QkObsTerm) in a similar vein, though I'm also fine with QkSparseObservable. I don't feel super strongly about any of this, just loose preference and thought I'd bring it up.
For all unsafe blocks: it'd be good to get into the habit of having // SAFETY: comments on top of all of them: the more we write, the more likely it is that we forget some of the invariants, or inadvertently add to them and think it's obvious from context, etc. I think there's a clippy lint against it we might consider turning on - it means a bit more boilerplate to write for us, but my rough feeling atm is it's better to have it. Rust unsafe is even harder to write than C code, because there's more invariants necessary to satisfy the higher-level Rust objects.
- mainly SAFETY sections - improve formatting tools - location and import of header
There was a problem hiding this comment.
Overall this looks really good. Thanks a lot for all the work on this @Cryoris and @mrossinek.
My main comment after reading through this is that I think we need a bit more user documentation, but that's something we can do in a follow up. I left a few small comments inline, but nothing major on the code front.
| To compile the above, a Rust compiler is required to build the dynamic library and | ||
| ``cbindgen`` to build the header. Then the C script can be compiled by specifying the library's | ||
| location, e.g., using ``gcc``, we have: | ||
|
|
||
| .. code:: | ||
|
|
||
| make cheader # build's the library into ./target/release and the header into root | ||
| gcc <my_script.c> -lqiskit_cext -L./target/release -I./ # compile and link the library |
There was a problem hiding this comment.
Yeah, I think it should be in the public docs somewhere, but also we should put this in the cext crate readme. Right now we don't have a readme for the c api either.
|
Thanks for all the comments! We addressed almost all of them, the main ones that are still open I think are:
Plus some small pieces like updates to the CMakeLists, or checking if we can put the library into Exposing the C bindings from the same shared library as the Python extension is something we plan as follow up, right? Or would you prefer this to be in the same PR? |
This commit reworks the print obs and obsterm functions which just called printf internally and instead replaces them with new `_str()` and `free_*_str()` functions to both return a string and free a string representation of a sparse observable and term. This enables debugging but without forcing you to stdout. Managing the stdout buffers between 2 or 3 languages isn't really feasible, so having a string generator function lets the C user do whatever they need with the string repr.
I pushed up a small commit adding this: 603ae56 someone should check it to make sure it's sane.
I think lets do this as a small follow-up. We co-authored the commit already to do this already, and it's like 3 lines in |
As an implementation detail, `qiskit.h` includes `stdint.h` and `stdbool.h`, but we use their items explicitly, so best to be explicit. Fixup includes
There was a problem hiding this comment.
Thanks for this, Julien and Max!
A couple of other questions beyond those in the review:
- do we have any plan for / idea how to measure coverage of our C API? It would be good to know if we're exercising it all in tests.
- what plans do we have for linting the test suite?
- do we have plans for running the test suite under some combination of behaviour sanitisers? I don't know if we'd be able to run the Rust stuff under Miri's interpreter simultaneously from the C test suite, but it would be really good if we could. If not: perhaps we could look into writing tests in Rust that use the C API itself, so we can run them under Miri?
I think most of the comments I left here say they can be for follow ups.
I pushed up a few bits and bobs of fixes that I saw while I was at it too.
| // Complex number typedefs -- note these are memory aligned but | ||
| // not calling convention compatible. | ||
| typedef float complex QkComplex32; | ||
| typedef double complex QkComplex64; |
There was a problem hiding this comment.
So in theory we can have cbindgen generate the correct definitions of these two types if we pass parse.includes = ["num-complex"].
In practice, that's going to make the C API a right pain to use for complex numbers, because then you have to do everything in terms of the structs. I think we're probably right to do some sort of hackery like this, but we need to take absolute care that we never accept or return. one of these by value.
There was a problem hiding this comment.
We investigated this in the beginning, but it seemed less convenient than just the typedef (which I believe is also used in the C++ project)
There was a problem hiding this comment.
The trade-off for convenience is just a lot of discipline - it would be easy to accidentally introduce the type into a by-value argument or return, and that would break because we're lying about the true type here.
Still the right call, I think, but I don't love how we generate some of the file and hard code other bits. Nothing to do here, though.
| // SAFETY: Per documentation, the pointer is non-null and aligned. | ||
| let obs = unsafe { const_ptr_as_ref(obs) }; | ||
| let string: String = format!("{:?}", obs); | ||
| CString::new(string).unwrap().into_raw() |
There was a problem hiding this comment.
Using debug printing makes quite a mess of the complex numbers. We should probably refactor the Rust stuff a little so we can use the same machinery as the Python __repr__.
Can be a follow-up.
There was a problem hiding this comment.
Yeah let's fix this in a follow up 👍🏻
also rename QkSparseTerm to QkObsTerm
This modifies the Makefile to produce a full distribution of the standalone C-API objects routed at `dist/c`. The Makefile rules `cheader` and `clib` can be used, depending on whether one wants to generate the individual components, or `make c` to make the whole C distribution.
There was a problem hiding this comment.
Thanks for all the work on this everybody. I'm going to add it to the queue now - I think the general principles are all ok now, the build instructions present a sensible initial platform to build from, and everything else we can go from here.
We've got quite a few follow-ups and ways to improve from here, but I think the right thing to do is get it merged at this point.
Summary
This PR introduces a C API for the
SparseObservable. These changes are based on top of #13595, which separates the Python and Rust implementations. While there are still some open question (see below), this first implementation is functioning and can serve as basis for discussions. See e.g. the test file (test/c/test_sparse_observable.c) for example usage.Closes #13445.
Details and comments
Open points (that we know of, certainly there are more 😄):
PySparseObservableoffSparseObservable#13595How to add the docs?We have a working concept with doxygen & breathe, let's add as follow-upSparseObservablebecome something likeQkSparseObservable, or are the names sufficiently unique.