Use OnceLock to cache a NormalOperation in Target
#13995
Conversation
- As we plan to use `PackedOperation` to add instructions to a `Target` in Rust. We should not require having a python instance exists within. The following commits fix that by ensuring that a python instruction is not required, and if so it can be built based on a `PackedOperation`'s internal data. - This should not have any visible effects on the current implementations, but can be proven to work via the added unit-tests.
|
One or more of the following people are relevant to this code:
|
OnceLock to cache a NormalOperation in TargetOnceLock to cache a NormalOperation in Target
Pull Request Test Coverage Report for Build 14625475896Warning: 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 |
|
|
||
| #[test] | ||
| fn add_standard_gate_without_python() -> PyResult<()> { | ||
| let _lock = IMPORT_LOCK |
There was a problem hiding this comment.
There was some discussion a while back about having qiskit get imported up front to avoid this sort of thing:
I didn't bother submitting a PR to do that since there's also the issue of PyObjects not being extractable to Rust pyclass structs due to ABI compat issues (i.e. in the Rust tests, our pyclass structs come from the rlib, but the structs that back PyObjects coming from the actual Qiskit code originate from the cdylib linked into the Python process).
My assumption was that folks probably would just not really use PyO3 Rust unit testing for anything complex. But, it looks like you have a use case here. Should I make a PR to avoid the deadlock issue?
There was a problem hiding this comment.
If it does solve the issue, I would love to have a PR that does that. It would be pretty helpful.
There was a problem hiding this comment.
Done in #14200.
I tried it with this PR removing the mutex and it seemed to work 🙂/
There was a problem hiding this comment.
I commented on #14200, but just to centralise: if we do this, I'd agree that #14200 is a better strategy. That said, I'm not convinced that we ought to use Python in this test. I'm not entirely sure what the use of Python is buying us - if we test the Rust objects from Rust, is there a really serious risk that "object created from Rust" might not produce the correct Python object later in a way that would not be caught by regular tests of the creation of Python-free StandardGate objects?
Even if this test can in theory work at this moment in time, I'm not crazy about doing more testing where _accelerate is imported from apython in Rust, because we know that that's not ABI compatible and feels like an accident waiting to happen.
There was a problem hiding this comment.
IMO, #14200 is a nice-to-have if we're going to keep PyO3 unit testing around for the time being. The main use-case I'd see for it is as a stop-gap measure for cases where our internal Rust code relies on importing parts of our data model that aren't yet native to Rust.
But in this particular case, I'd agree that we're not getting much by testing that the Rust-backed operations from the target are equivalent to gates constructed via Python. I'd rather expect any testing here to be poking at the Target Rust instance to make sure the instructions were added successfully (no Python involved), if you're to have any testing here at all.
- Remove `From` implementations in favor of `from_packed_operation` in both `TargetOperation`, and `NormalOperation`.
| Ok(self.op_object.bind(py).clone()) | ||
| Ok(self | ||
| .op_object | ||
| .get_or_init(|| self.create_py_op(py, None).unwrap()) |
There was a problem hiding this comment.
Should we store op_object as a PyResult<PyObject> instead so we can propagate the error if create_py_op fails?
There was a problem hiding this comment.
I think it might not be necessary, I could just extract the operation before setting it instead.
There was a problem hiding this comment.
After an internal conversation, we have gone with storing a PyResult. Fixed in cd8e63b.
Co-authored-by: Kevin Hartman <kevin@hart.mn>
Summary
The following commits replace the
op_objectattribute ofNormalOperationwith aOnceLockto allow the python version to be either built dynamically or cached from its original source.Details and comments
As we plan to use
PackedOperationto add instructions to aTargetin Rust. We should not require having a python instance to exist within. The following commits fix that by ensuring that a python instruction is not required, and if so it can be built based on aPackedOperation's internal data.This should not have any visible effects on the current implementations, but can be proven to work via the added unit-tests.