Skip to content

Conversation

kevinhartman
Copy link
Contributor

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, since pickle 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 an impl block that wasn't decorated with #[pymethods], meaning it wasn't actually being used before. That's fixed here too.

Fixes #14169

@kevinhartman kevinhartman added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Apr 4, 2025
@kevinhartman kevinhartman added this to the 2.0.1 milestone Apr 4, 2025
@kevinhartman kevinhartman self-assigned this Apr 4, 2025
@kevinhartman kevinhartman requested a review from a team as a code owner April 4, 2025 16:11
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@kevinhartman kevinhartman changed the title Fix pickle and repr for [Duration]. Fix pickle and repr for Duration. Apr 4, 2025
@raynelfss raynelfss self-assigned this Apr 4, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 14269579109

Details

  • 21 of 26 (80.77%) changed or added relevant lines in 2 files are covered.
  • 31 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 88.061%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/lib.rs 15 20 75.0%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 94.79%
crates/qasm2/src/lex.rs 6 92.23%
crates/qasm2/src/parse.rs 24 96.22%
Totals Coverage Status
Change from base Build 14243929141: -0.02%
Covered Lines: 72913
Relevant Lines: 82798

💛 - Coveralls

Copy link
Contributor

@raynelfss raynelfss left a 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.

Comment on lines +177 to +197
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")?,
)?;

Copy link
Contributor

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.

Copy link
Contributor Author

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_xxs 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).

@kevinhartman kevinhartman added this pull request to the merge queue Apr 7, 2025
Merged via the queue into Qiskit:main with commit b0bc6d8 Apr 8, 2025
24 checks passed
mergify bot pushed a commit that referenced this pull request Apr 8, 2025
github-merge-queue bot pushed a commit that referenced this pull request Apr 8, 2025
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test test.python.compiler.test_transpiler.TestTranspile.test_delay_converts_expr_to_dt fails in Qiskit 2.0
4 participants