GH-41098: [Python] Add copy keyword in Array.array for numpy 2.0+ compatibility #41071

jorisvandenbossche · 2024-04-08T15:27:14Z

Rationale for this change

Adapting for changes in numpy 2.0 as decribed at https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword and future changes to pass copy=True (numpy/numpy#26208)

What changes are included in this PR?

Add a copy=None to the signatures of our __array__ methods.

This does have impact on the user facing behaviour, though. Questioning that upstream at numpy/numpy#25941 (comment)

Are these changes tested?

Yes

Are there any user-facing changes?

No (compared to usage with numpy<2)

GitHub Issue: [Python] Compatibility with NumPy 2.0 #39532
GitHub Issue: [Python] Adapt to changes in the copy keyword for numpy 2.0+ compatibility #41098

…y 2.0+ compatibility

jorisvandenbossche · 2024-04-08T15:27:28Z

python/pyarrow/array.pxi

@@ -1543,7 +1543,8 @@ cdef class Array(_PandasConvertible):
    def _to_pandas(self, options, types_mapper=None, **kwargs):
        return _array_like_to_pandas(self, options, types_mapper=types_mapper)

-    def __array__(self, dtype=None):
+    def __array__(self, dtype=None, copy=None):
+        # TODO honor the copy keyword


Need to open an issue for that

github-actions · 2024-04-08T15:27:39Z

⚠️ GitHub issue #39532 has been automatically assigned in GitHub to PR creator.

jorisvandenbossche · 2024-04-09T10:56:26Z

@github-actions crossbow submit test-conda-python-3.11-pandas-upstream_devel test-conda-python-3.10-pandas-nightly

github-actions · 2024-04-09T10:58:45Z

Revision: 686865e

Submitted crossbow builds: ursacomputing/crossbow @ actions-3ef08a5f21

Task	Status
test-conda-python-3.10-pandas-nightly
test-conda-python-3.11-pandas-upstream_devel

pitrou · 2024-04-09T13:26:23Z

python/pyarrow/array.pxi

@@ -1543,7 +1543,20 @@ cdef class Array(_PandasConvertible):
    def _to_pandas(self, options, types_mapper=None, **kwargs):
        return _array_like_to_pandas(self, options, types_mapper=types_mapper)

-    def __array__(self, dtype=None):
+    def __array__(self, dtype=None, copy=None):
+        # TODO honor the copy=True case


Can we raise an exception for now? Also, can you open a GH issue for it?

Can we raise an exception for now?

I wouldn't do that I think, because then when numpy would start passing that down in let's say numpy 2.1, np.array(obj) would start erroring, while those never errored before (and for some cases this might incorrectly not return a copy (although still marked as read-only), many cases already do copy anyways)

Although to be honest, I don't really know what the strategy of numpy will be to enable this keyword. If they would just start to stop copying the result of __array__, that would cause such changes in many libraries with __array__s)

Though ideally, we just directly implement copy=True as well (it was just lest critical for numpy 2.0 as it is not yet used, and also a bit harder to implement)

and also a bit harder to implement

On second thought, it might be relatively easy to determine if to_numpy() returned a copy or not? I was first thinking we would have to mimic the logic based on the type ("if primitive and no nulls, then it is zero copy"), but we might be able to check if the resulting ndarray has a base pointing to a pyarrow object?

Although the simple logic of numeric+no nulls might be easier in practice.

pitrou · 2024-04-09T13:26:58Z

python/pyarrow/table.pxi

@@ -525,7 +525,8 @@ cdef class ChunkedArray(_PandasConvertible):

        return values

-    def __array__(self, dtype=None):
+    def __array__(self, dtype=None, copy=None):
+        # copy keyword can be ignored because to_numpy() already returns a copy


Well, copy=False should then raise an error, no?

Ah yes, that comment is from before I decided to handle the copy=False case for Array. Indeed we should just raise an error here that a no-copy is not possible.

Updated.

pitrou · 2024-04-09T13:28:50Z

python/pyarrow/table.pxi

@@ -1533,7 +1534,8 @@ cdef class _Tabular(_PandasConvertible):
        raise TypeError(f"Do not call {self.__class__.__name__}'s constructor directly, use "
                        f"one of the `{self.__class__.__name__}.from_*` functions instead.")

-    def __array__(self, dtype=None):
+    def __array__(self, dtype=None, copy=None):
+        # copy keyword can be ignored as this always already returns a copy


…y-compat-array

pitrou · 2024-04-15T12:32:44Z

python/pyarrow/array.pxi

        values = self.to_numpy(zero_copy_only=False)
+        if copy is True and is_primitive(self.type.id) and self.null_count == 0:
+            # to_numpy did not yet make a copy


Will this make a second copy for e.g. boolean?

No, boolean is not considered as primitive (although you might expect that). So a boolean array takes the code path below just returning the numpy values is no dtype was required, so no second copy.

No, sorry, boolean is of course "primitive", what I meant to say is that it is not "numeric", and the code above was meant to use is_numeric (I added that to libarrow.pxd for that purpose). Good catch ..

…y-compat-array

jorisvandenbossche · 2024-04-15T13:08:22Z

@github-actions crossbow submit test-conda-python-3.11-pandas-upstream_devel test-conda-python-3.10-pandas-nightly

github-actions · 2024-04-15T13:10:40Z

Revision: 4b630f6

Submitted crossbow builds: ursacomputing/crossbow @ actions-0fad67714d

Task	Status
test-conda-python-3.10-pandas-nightly
test-conda-python-3.11-pandas-upstream_devel

jorisvandenbossche · 2024-04-15T15:01:56Z

(the failure is the flaky join_asof test)

pitrou

Thanks @jorisvandenbossche !

… compatibility (#41071) ### Rationale for this change Adapting for changes in numpy 2.0 as decribed at https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword and future changes to pass copy=True (numpy/numpy#26208) ### What changes are included in this PR? Add a `copy=None` to the signatures of our `__array__` methods. This does have impact on the user facing behaviour, though. Questioning that upstream at numpy/numpy#25941 (comment) ### Are these changes tested? Yes ### Are there any user-facing changes? No (compared to usage with numpy<2) * GitHub Issue: #39532 * GitHub Issue: #41098 Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

conbench-apache-arrow · 2024-04-16T07:56:27Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 80258fe.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

…y 2.0+ compatibility (apache#41071) ### Rationale for this change Adapting for changes in numpy 2.0 as decribed at https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword and future changes to pass copy=True (numpy/numpy#26208) ### What changes are included in this PR? Add a `copy=None` to the signatures of our `__array__` methods. This does have impact on the user facing behaviour, though. Questioning that upstream at numpy/numpy#25941 (comment) ### Are these changes tested? Yes ### Are there any user-facing changes? No (compared to usage with numpy<2) * GitHub Issue: apache#39532 * GitHub Issue: apache#41098 Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

apacheGH-39532: [Python] Add copy keyword in Array.__array__ for nump…

2f1b8cc

…y 2.0+ compatibility

jorisvandenbossche commented Apr 8, 2024

View reviewed changes

github-actions bot added Component: Python awaiting committer review Awaiting committer review awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 8, 2024

jorisvandenbossche changed the title ~~GH-39532: [Python] Add copy keyword in Array.__array__ for numpy 2.0+ compatibility~~ GH-41098: [Python] Add copy keyword in Array.__array__ for numpy 2.0+ compatibility Apr 9, 2024

properly handle the case of copy=False

686865e

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 9, 2024

jorisvandenbossche marked this pull request as ready for review April 9, 2024 12:42

jorisvandenbossche requested a review from pitrou April 9, 2024 12:43

pitrou reviewed Apr 9, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 9, 2024

raise error for copy=False in ChunkedArray/RecordBatch/Table

2bd63a9

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 9, 2024

jorisvandenbossche added 3 commits April 11, 2024 12:10

Merge remote-tracking branch 'upstream/main' into apachegh-39532-nump…

5b87fd3

…y-compat-array

add support for copy=True as well

0e2b402

remove TODO comment

4e2bc7d

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 11, 2024

jorisvandenbossche requested a review from pitrou April 15, 2024 12:20

pitrou reviewed Apr 15, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 15, 2024

jorisvandenbossche added 2 commits April 15, 2024 14:50

fix to actually use is_numeric

d980cef

Merge remote-tracking branch 'upstream/main' into apachegh-39532-nump…

4b630f6

…y-compat-array

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 15, 2024

pitrou approved these changes Apr 15, 2024

View reviewed changes

pitrou merged commit 80258fe into apache:main Apr 15, 2024
12 checks passed

pitrou removed the awaiting change review Awaiting change review label Apr 15, 2024

pitrou mentioned this pull request Apr 15, 2024

[Python] Adapt to changes in the copy keyword for numpy 2.0+ compatibility #41098

Closed

github-actions bot added the awaiting committer review Awaiting committer review label Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41098: [Python] Add copy keyword in Array.array for numpy 2.0+ compatibility #41071

GH-41098: [Python] Add copy keyword in Array.array for numpy 2.0+ compatibility #41071

jorisvandenbossche commented Apr 8, 2024 •

edited by github-actions bot

Loading

jorisvandenbossche Apr 8, 2024

github-actions bot commented Apr 8, 2024

jorisvandenbossche commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

pitrou Apr 9, 2024

jorisvandenbossche Apr 9, 2024

jorisvandenbossche Apr 9, 2024

jorisvandenbossche Apr 9, 2024

pitrou Apr 9, 2024

jorisvandenbossche Apr 9, 2024

pitrou Apr 9, 2024

pitrou Apr 15, 2024

jorisvandenbossche Apr 15, 2024

jorisvandenbossche Apr 15, 2024

jorisvandenbossche commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

jorisvandenbossche commented Apr 15, 2024

pitrou left a comment

conbench-apache-arrow bot commented Apr 16, 2024

GH-41098: [Python] Add copy keyword in Array.__array__ for numpy 2.0+ compatibility #41071

GH-41098: [Python] Add copy keyword in Array.__array__ for numpy 2.0+ compatibility #41071

Conversation

jorisvandenbossche commented Apr 8, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

github-actions bot commented Apr 8, 2024

jorisvandenbossche commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

jorisvandenbossche commented Apr 15, 2024

pitrou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Apr 16, 2024

GH-41098: [Python] Add copy keyword in Array.array for numpy 2.0+ compatibility #41071

GH-41098: [Python] Add copy keyword in Array.array for numpy 2.0+ compatibility #41071

jorisvandenbossche commented Apr 8, 2024 •

edited by github-actions bot

Loading