-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix pickle and repr for Duration
.
#14174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
One or more of the following people are relevant to this code:
|
Duration
.
Pull Request Test Coverage Report for Build 14269579109Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, I'm just a bit surprised that we'd have to explicitly export each variant.
m.add( | ||
"Duration_ns", | ||
duration::Duration::type_object(m.py()).getattr("ns")?, | ||
)?; | ||
m.add( | ||
"Duration_us", | ||
duration::Duration::type_object(m.py()).getattr("us")?, | ||
)?; | ||
m.add( | ||
"Duration_ms", | ||
duration::Duration::type_object(m.py()).getattr("ms")?, | ||
)?; | ||
m.add( | ||
"Duration_s", | ||
duration::Duration::type_object(m.py()).getattr("s")?, | ||
)?; | ||
m.add( | ||
"Duration_dt", | ||
duration::Duration::type_object(m.py()).getattr("dt")?, | ||
)?; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised that this step is needed. But I guess it makes some sense considering that pickle
will try to access the class objects based on their import paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was too, though I think it makes sense that PyO3 doesn't expose these by default. Users should not be constructing Duration_xx
s directly, so I don't think the names have a good case for being exposed by the containing module, other than for serialization (which PyO3 doesn't explicitly support).
(cherry picked from commit b0bc6d8)
* Fix pickle and repr for Duration. (#14174) (cherry picked from commit b0bc6d8) * Update releasenotes/notes/fix-duration-props-0543fe1d5e6e2820.yaml * Update releasenotes/notes/fix-duration-props-0543fe1d5e6e2820.yaml --------- Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Summary
Adds missing Python pickle support for the new
Duration
type added in Qiskit 2.0.Details and comments
To get this working, I needed to manually register the PyO3 generated
Duration_xx
Python classes at the module level, sincepickle
looks for them during deserialization. I'm not sure if there's a better way, but I'm open to suggestions if you can think of something cleaner. Pickle doesn't have explicit support in PyO3, so we're lucky to have any working solution here.While fixing this, I also noticed that
__repr__
was defined inside animpl
block that wasn't decorated with#[pymethods]
, meaning it wasn't actually being used before. That's fixed here too.Fixes #14169