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 to 0.23 #457

Merged
merged 8 commits into from
Nov 22, 2024
Merged

update pyo3 to 0.23 #457

merged 8 commits into from
Nov 22, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Oct 9, 2024

Relates #455

For personal testing a created a branch targeting PyO3 0.23-dev from main. I thought I'll leave that here. Once pyo3 0.23 releases, this can probably be split up to migrate.

@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 18, 2024

@davidhewitt I just bumped this to the 0.23 release version and run into the Sync restriction for #[pyclass]es on PySliceContainer. I believe it should be fine to implement Sync if the elements are also Sync (patch below) but I'm not 100% certain. Your opinion would be appreciated.

Sync Patch
diff --git a/src/dtype.rs b/src/dtype.rs
index a97edd43..ac96a277 100644
--- a/src/dtype.rs
+++ b/src/dtype.rs
@@ -492,7 +492,7 @@ impl Sealed for Bound<'_, PyArrayDescr> {}
 ///
 /// [enumerated-types]: https://numpy.org/doc/stable/reference/c-api/dtype.html#enumerated-types
 /// [data-models]: https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
-pub unsafe trait Element: Sized + Send {
+pub unsafe trait Element: Sized + Send + Sync {
     /// Flag that indicates whether this type is trivially copyable.
     ///
     /// It should be set to true for all trivially copyable types (like scalar types
diff --git a/src/slice_container.rs b/src/slice_container.rs
index b1326746..2ba31181 100644
--- a/src/slice_container.rs
diff --git a/src/dtype.rs b/src/dtype.rs
index a97edd43..ac96a277 100644
--- a/src/dtype.rs
+++ b/src/dtype.rs
@@ -492,7 +492,7 @@ impl Sealed for Bound<'_, PyArrayDescr> {}
 ///
 /// [enumerated-types]: https://numpy.org/doc/stable/reference/c-api/dtype.html#enumerated-types
 /// [data-models]: https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
-pub unsafe trait Element: Sized + Send {
+pub unsafe trait Element: Sized + Send + Sync {
     /// Flag that indicates whether this type is trivially copyable.
     ///
     /// It should be set to true for all trivially copyable types (like scalar types
diff --git a/src/slice_container.rs b/src/slice_container.rs
index b1326746..2ba31181 100644
--- a/src/slice_container.rs
+++ b/src/slice_container.rs
@@ -14,8 +14,9 @@ pub(crate) struct PySliceContainer {
 }
 
 unsafe impl Send for PySliceContainer {}
+unsafe impl Sync for PySliceContainer {}
 
-impl<T: Send> From<Box<[T]>> for PySliceContainer {
+impl<T: Send + Sync> From<Box<[T]>> for PySliceContainer {
     fn from(data: Box<[T]>) -> Self {
         unsafe fn drop_boxed_slice<T>(ptr: *mut u8, len: usize, _cap: usize) {
             let _ = Box::from_raw(ptr::slice_from_raw_parts_mut(ptr as *mut T, len));
@@ -39,7 +40,7 @@ impl<T: Send> From<Box<[T]>> for PySliceContainer {
     }
 }
 
-impl<T: Send> From<Vec<T>> for PySliceContainer {
+impl<T: Send + Sync> From<Vec<T>> for PySliceContainer {
     fn from(data: Vec<T>) -> Self {
         unsafe fn drop_vec<T>(ptr: *mut u8, len: usize, cap: usize) {
             let _ = Vec::from_raw_parts(ptr as *mut T, len, cap);
@@ -65,7 +66,7 @@ impl<T: Send> From<Vec<T>> for PySliceContainer {
 
 impl<A, D> From<ArrayBase<OwnedRepr<A>, D>> for PySliceContainer
 where
-    A: Send,
+    A: Send + Sync,
     D: Dimension,
 {
     fn from(data: ArrayBase<OwnedRepr<A>, D>) -> Self {

@juntyr
Copy link

juntyr commented Nov 18, 2024

@Icxolu Thank you for working on this PR - I just started looking into the PyO3 version bumps in my project and it's always awesome to see ongoing work <3

@davidhewitt
Copy link
Member

Thanks for pushing forward on this! It might be easier to review the change to unsafe impl Sync for PySliceContainer as a separate PR? That seems like it justifies more scrutiny than the large but mostly mechanical upgrades going on here.

For the Sync patch above, it seems to me that depends on what numpy's access rules are. I guess you do get a pointer into the underlying array memory to read elements in-place, so it seems correct that Element: Sync should be required.

@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 19, 2024

It might be easier to review the change to unsafe impl Sync for PySliceContainer as a separate PR? That seems like it justifies more scrutiny than the large but mostly mechanical upgrades going on here.

Good idea, opened #469 for that.

@Icxolu Icxolu force-pushed the pyo3-0.23 branch 2 times, most recently from da18a72 to d35c53a Compare November 20, 2024 19:02
@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 20, 2024

PyO3 dropped support for PyPy 3.7 and 3.8 in 0.23. I addressed this in #470

@Icxolu Icxolu marked this pull request as ready for review November 20, 2024 20:40
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, this looks good to me! I'll follow up with an addition to CI for free-threaded testing, and if that seems ok, let's ship!

@davidhewitt davidhewitt merged commit c386c8e into PyO3:main Nov 22, 2024
67 of 68 checks passed
@Icxolu Icxolu deleted the pyo3-0.23 branch November 22, 2024 18:08
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.

3 participants