Skip to content
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

Implement PartialEq for Param in rust using PyO3 #12697

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

raynelfss
Copy link
Contributor

@raynelfss raynelfss commented Jul 1, 2024

Summary

After #12459 merged, a limited infrastructure for Param in Rust was introduced. The following commits aim to extend its functionality a little to allow for comparison and further debugging which will be of great use for #12292 and #12585.

Details and comments

  • Implement PartialEq using Python::with_gil() to compare parameters through Python.
    - Add Display method for debugging using the Python __repr__ magic method. Not necessary

- Implement `PartialEq` using `Python::with_gil()` to compare parameters through Python.
- Add display method for debugging.
@raynelfss raynelfss requested a review from a team as a code owner July 1, 2024 15:14
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @kevinhartman
  • @mtreinish

@raynelfss raynelfss added enhancement Rust This PR or issue is related to Rust code in the repository labels Jul 1, 2024
@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9747009629

Warning: 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

  • 0 of 25 (0.0%) changed or added relevant lines in 1 file are covered.
  • 22 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.06%) to 89.781%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/operations.rs 0 25 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.2%
crates/qasm2/src/lex.rs 2 92.88%
crates/qasm2/src/parse.rs 18 96.23%
Totals Coverage Status
Change from base Build 9745765969: -0.06%
Covered Lines: 64392
Relevant Lines: 71721

💛 - Coveralls

@jakelishman
Copy link
Member

It's possibly better to use specific methods for these that take a Python token, rather than hooking into the Rust builtin traits - with_gil is slow and will be very surprisingly expensive for PartialEq in particular.

@raynelfss
Copy link
Contributor Author

It's possibly better to use specific methods for these that take a Python token, rather than hooking into the Rust builtin traits - with_gil is slow and will be very surprisingly expensive for PartialEq in particular.

Would there be a better way of performing this comparison that could skip the passing of a py token? A lot of the functionality of the Target and the EquivalenceLibrary require these comparisons in order to perform essential operations.

I understand that with the way things are built now, the circuit is the one taking ownership of the parameters of each instruction, but I don't know how I would translate this well enough.

@jakelishman
Copy link
Member

No, because fundamentally we must have a Python token to do this operation - you have to prove you hold the GIL to read or write Python state. Using Python::with_gil calls out to the interpreter to ensure the GIL is held, and will potentially fully serialise any threaded code that doesn't know that it's hiding.

You can define a custom equals method that takes the Python token as well, or change the data structures so that there's no need to access the Python heap at all, but as long as there's Py pointers, we need the Python token.

@raynelfss raynelfss changed the title Implement PartialEq and Display for Param in rust using PyO3 Implement PartialEq for Param in rust using PyO3 Jul 3, 2024
@raynelfss
Copy link
Contributor Author

After some consideration, @jakelishman, I decided to just use the PyObject::is() method as it doesn't require a gil reference . This is also with the assumption that Parameters aren't really comparable unless they're one and the same. Since whenever a new parameter is added, a new uuid is generated, even if they have the same symbol. So it is easier to just compare if both instances are pointing to the same object.

I also decided that the Display isn't really necessary here so I removed it. Let me know your thoughts.

@jakelishman
Copy link
Member

Using Python's is operator is still unfortunately not sufficient - it'll not hold for ParameterExpression, which is what this object holds.

As long as there are Python objects here, I really can't see a way through without explicitly requiring the GIL token, or converting one of the objects into some sort of "bound" version, which includes a Python GIL lifetime within it in order to use that in the equality.

But you don't have to use the PartialEq trait to fulfil a function that compares two things for equality. That just makes the == and != operators work, but having a method called equal that takes the Python token explicitly is also fine.

@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9779973622

Details

  • 0 of 7 (0.0%) changed or added relevant lines in 1 file are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.002%) to 89.827%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/operations.rs 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.35%
crates/qasm2/src/lex.rs 3 92.88%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 9779831858: -0.002%
Covered Lines: 65132
Relevant Lines: 72508

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants