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

Update PyO3 and Rust Numpy to 0.18.0 #787

Merged
merged 5 commits into from
Feb 1, 2023
Merged

Conversation

mtreinish
Copy link
Member

The latest releases of PyO3 and rust numpy were just released. The changelogs for both can be found at:

https://pyo3.rs/v0.18.0/changelog

and

https://github.com/PyO3/rust-numpy/blob/main/CHANGELOG.md

Since the two releases are tightly coupled this commit opts to update them together instead of the normal dependabot workflow. Similarly some of our usage (mainly around signatures and argument ordering) for building a python interface using PyO3 has been deprecated. That usage is also updated in this PR to avoid compilation warnings, which will be treated as errors by clippy in CI.

The latest releases of PyO3 and rust numpy were just released. The
changelogs for both can be found at:

https://pyo3.rs/v0.18.0/changelog

and

https://github.com/PyO3/rust-numpy/blob/main/CHANGELOG.md

Since the two releases are tightly coupled this commit opts to update
them together instead of the normal dependabot workflow. Similarly some
of our usage (mainly around signatures and argument ordering) for
building a python interface using PyO3 has been deprecated. That usage
is also updated in this PR to avoid compilation warnings, which will be
treated as errors by clippy in CI.
@mtreinish mtreinish added the dependencies Pull requests that update a dependency file label Jan 20, 2023
@coveralls
Copy link

coveralls commented Jan 20, 2023

Pull Request Test Coverage Report for Build 4056352977

  • 193 of 207 (93.24%) changed or added relevant lines in 15 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 97.079%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/layout/mod.rs 18 20 90.0%
src/union.rs 2 4 50.0%
src/isomorphism/mod.rs 8 12 66.67%
src/traversal/mod.rs 24 30 80.0%
Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_bellman_ford.rs 2 98.88%
Totals Coverage Status
Change from base Build 4039142668: 0.2%
Covered Lines: 13726
Relevant Lines: 14139

💛 - Coveralls

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a not ordinary bump, so I will take some time to review it

src/layout/mod.rs Show resolved Hide resolved
@mtreinish
Copy link
Member Author

I think this is a not ordinary bump, so I will take some time to review it

Fwiw, almost all of the changes here are just using the new #[pyo3(signature)] interface instead of #[args] or #[pyfunction]. I tried to keep the diff minimal besides that, but do check it carefully because it would have been easy for me to make a mistake.

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is looking good, the only minor comments are on the traversal and layout modules

src/layout/mod.rs Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
Comment on lines +296 to +300
visitor: Option<PyBfsVisitor>,
) -> PyResult<()> {
if visitor.is_none() {
return Err(PyTypeError::new_err("Missing required argument visitor"));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only problematic part of the PR... Why can't digraph_bfs_search accept PyBfsVisitor? I think PyO3 should check that the argument is valid, not us

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its the new rules about argument ordering, we're no longer allowed to have a required argument without a default defined after one an optional one that has a default. Basically it's trying to block a python signature like:

digraph_bfs_search(graph, source=None, visitor)

which generally wouldn't be valid in python and is effectively what the previous signature for this function was.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically we had two options to fix this, either give visitor a default option or move visitor before source. I opted to give it a default here because moving visitor would be a breaking change because it would break existing callers of this function using positional arguments

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess we’ll have to deal with it

src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
src/traversal/mod.rs Outdated Show resolved Hide resolved
@mtreinish mtreinish mentioned this pull request Feb 1, 2023
Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mtreinish mtreinish added the automerge Queue a approved PR for merging label Feb 1, 2023
@mergify mergify bot merged commit a297e7f into Qiskit:main Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants