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

Also relax the PySequence check when extracting fixed-sized arrays. #2675

Merged
merged 1 commit into from Oct 13, 2022

Conversation

adamreichold
Copy link
Member

Closes #2674

@adamreichold adamreichold force-pushed the relax-extract-array branch 3 times, most recently from 7863dc6 to 4c48977 Compare October 11, 2022 16:22
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for handling this, and sorry for my slow reply. My son and I were both sick with colds over the weekend previous and I'm just about getting back up to speed now.

Just a couple of tiny suggestions 👍

if ffi::PySequence_Check(obj.as_ptr()) != 0 {
<PySequence as PyTryFrom>::try_from_unchecked(obj)
} else {
return Err(PyDowncastError::new(obj, "Sequence").into());
Copy link
Member

Choose a reason for hiding this comment

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

Willing to add a test for this error branch (ie. pass a non-iterable) to get the coverage to 100%? 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test case triggering this error path.

@@ -0,0 +1 @@
`impl FromPyObject for [T; N]` will accept anything passing `PySequence_Check`, e.g. NumPy arrays, in the same way that `impl FromPyObject for Vec<T>` was modified after [#2477](https://github.com/PyO3/pyo3/pull/2477).
Copy link
Member

Choose a reason for hiding this comment

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

Given this is a revert, I think it'd be quite handy to state what version the behaviour is reverting to rather than the offending PR. Users who want to see the PR can start with the link from the CHANGELOG to this PR and follow the discussion thread.

E.g. "Fix regression of impl FromPyObject for [T; N] no longer accepting types passing PySequence_Check, e.g. NumPy arrays, since 0.17.0."

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted the wording, please re-review.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great, thanks again! I guess we should put out 0.17.3 - anything else we should include?

@davidhewitt davidhewitt merged commit 61fd70c into main Oct 13, 2022
@adamreichold adamreichold deleted the relax-extract-array branch October 13, 2022 19:58
@adamreichold
Copy link
Member Author

Looks great, thanks again! I guess we should put out 0.17.3 - anything else we should include?

I don't think we have any other bug fixes ready that are blocked only on review. Everything else seems to be enhancements.

We do have two undiagnosed crash bugs in #2611 and https://github.com/PyO3/pyo3/discussions/26778 though.

Personally, I am not sure if this fix here alone warrants a point release but then again it is a regression. (Turning NumPy arrays into fixed-size Rust arrays seems a bit fringe as NumPy arrays are relatively inefficient for small arrays which is where fixed-size Rust arrays are usually applied.)

@davidhewitt
Copy link
Member

Ok. Let's hold off for the moment; Python 3.11.0 is due at the end of the month so maybe we can target 0.17.3 for then once we can confirm PyO3 officially supports it. (Unless we can land some bigger features worthy of calling it 0.18!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot extract Rust arrray from NumPy array
2 participants