GH-39942: [Python] Make capsule name check more lenient #39977

paleolimbot · 2024-02-07T16:12:14Z

Rationale for this change

While #39969 fixed the immediate issue caused by the update of the capsule name used by reticulate whilst converting an R "external pointer", it will still result in an error if somebody is using an older version of the Arrow R package.

What changes are included in this PR?

The pyarrow Cython code was modified to accept capsules with the name NULL or "r_extptr".

Are these changes tested?

Not sure where the best place for this is, but:

CRAN arrow + released pyarrow + new reticulate (errors):

library(arrow, warn.conflicts = FALSE)
reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv")
packageVersion("arrow")
#> [1] '14.0.0.2'
packageVersion("reticulate")
#> [1] '1.35.0'
pa <- reticulate::import("pyarrow")
pa[["__version__"]]
#> [1] "15.0.0"

reticulate::r_to_py(arrow::int32())
#> PyCapsule_GetPointer called with incorrect name

CRAN arrow + pyarrow from this PR + old reticulate:

library(arrow, warn.conflicts = FALSE)
reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv")
packageVersion("arrow")
#> [1] '14.0.0.2'
packageVersion("reticulate")
#> [1] '1.34.0'
pa <- reticulate::import("pyarrow")
pa[["__version__"]]
#> [1] "16.0.0.dev92+geafcff7a5"

reticulate::r_to_py(arrow::int32())
#> DataType(int32)

CRAN arrow + pyarrow from this PR + new reticulate:

library(arrow, warn.conflicts = FALSE)
reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv")
packageVersion("arrow")
#> [1] '14.0.0.2'
packageVersion("reticulate")
#> [1] '1.35.0'
pa <- reticulate::import("pyarrow")
pa[["__version__"]]
#> [1] "16.0.0.dev92+geafcff7a5"

reticulate::r_to_py(arrow::int32())
#> DataType(int32)

Are there any user-facing changes?

No

Closes: [CI][R][Docs] Docs jobs failing due to R Markdown error #39942

github-actions · 2024-02-07T16:12:41Z

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

jorisvandenbossche

I am personally fine with not testing it on CI, as long as you verified locally it works (and add a comment in the code why "r_extptr" is also tested)

python/pyarrow/types.pxi

Co-authored-by: Joris Van den Bossche <[email protected]>

pitrou · 2024-02-08T16:24:32Z

python/pyarrow/types.pxi

@@ -114,7 +123,14 @@ cdef void* _as_c_pointer(v, allow_null=False) except *:
            "Arrow library", UserWarning, stacklevel=2)
        c_ptr = <void*> <uintptr_t > v
    elif PyCapsule_CheckExact(v):
-        c_ptr = PyCapsule_GetPointer(v, NULL)
+        capsule_name = PyCapsule_GetName(v)
+        if capsule_name == NULL or capsule_name == "r_extptr":


Did you check that capsule_name == "r_extptr" works as expected?

I still need to check!

Suggested change

if capsule_name == NULL or capsule_name == "r_extptr":

if capsule_name == NULL or capsule_name == b"r_extptr":

I suspect it should be compared with bytes when comparing directly (our frombytes helper converts from the bytes to string)

I wrapped this into another change, but good call! capsule_name == "r_extptr" always returns false but capsule_name == b"r_extptr" seems to do the trick 🤷

python/pyarrow/types.pxi

pitrou · 2024-02-08T16:25:42Z

python/pyarrow/types.pxi

+    PyCapsule_IsValid
+)
+
+from libc.string cimport strcmp


This can be removed now?

Co-authored-by: Antoine Pitrou <[email protected]>

conbench-apache-arrow · 2024-02-09T16:24:33Z

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

There were no benchmark performance regressions. 🎉

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

…#39977) ### Rationale for this change While apache#39969 fixed the immediate issue caused by the update of the capsule name used by reticulate whilst converting an R "external pointer", it will still result in an error if somebody is using an older version of the Arrow R package. ### What changes are included in this PR? The pyarrow Cython code was modified to accept capsules with the name NULL or "r_extptr". ### Are these changes tested? Not sure where the best place for this is, but: CRAN arrow + released pyarrow + new reticulate (errors): ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.35.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "15.0.0" reticulate::r_to_py(arrow::int32()) #> PyCapsule_GetPointer called with incorrect name ``` CRAN arrow + pyarrow from this PR + old reticulate: ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.34.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "16.0.0.dev92+geafcff7a5" reticulate::r_to_py(arrow::int32()) #> DataType(int32) ``` CRAN arrow + pyarrow from this PR + new reticulate: ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.35.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "16.0.0.dev92+geafcff7a5" reticulate::r_to_py(arrow::int32()) #> DataType(int32) ``` ### Are there any user-facing changes? No * Closes: apache#39942 Lead-authored-by: Dewey Dunnington <[email protected]> Co-authored-by: Dewey Dunnington <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

### Rationale for this change While #39969 fixed the immediate issue caused by the update of the capsule name used by reticulate whilst converting an R "external pointer", it will still result in an error if somebody is using an older version of the Arrow R package. ### What changes are included in this PR? The pyarrow Cython code was modified to accept capsules with the name NULL or "r_extptr". ### Are these changes tested? Not sure where the best place for this is, but: CRAN arrow + released pyarrow + new reticulate (errors): ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.35.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "15.0.0" reticulate::r_to_py(arrow::int32()) #> PyCapsule_GetPointer called with incorrect name ``` CRAN arrow + pyarrow from this PR + old reticulate: ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.34.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "16.0.0.dev92+geafcff7a5" reticulate::r_to_py(arrow::int32()) #> DataType(int32) ``` CRAN arrow + pyarrow from this PR + new reticulate: ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.35.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "16.0.0.dev92+geafcff7a5" reticulate::r_to_py(arrow::int32()) #> DataType(int32) ``` ### Are there any user-facing changes? No * Closes: #39942 Lead-authored-by: Dewey Dunnington <[email protected]> Co-authored-by: Dewey Dunnington <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

…#39977) ### Rationale for this change While apache#39969 fixed the immediate issue caused by the update of the capsule name used by reticulate whilst converting an R "external pointer", it will still result in an error if somebody is using an older version of the Arrow R package. ### What changes are included in this PR? The pyarrow Cython code was modified to accept capsules with the name NULL or "r_extptr". ### Are these changes tested? Not sure where the best place for this is, but: CRAN arrow + released pyarrow + new reticulate (errors): ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.35.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "15.0.0" reticulate::r_to_py(arrow::int32()) #> PyCapsule_GetPointer called with incorrect name ``` CRAN arrow + pyarrow from this PR + old reticulate: ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.34.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "16.0.0.dev92+geafcff7a5" reticulate::r_to_py(arrow::int32()) #> DataType(int32) ``` CRAN arrow + pyarrow from this PR + new reticulate: ``` r library(arrow, warn.conflicts = FALSE) reticulate::use_virtualenv("~/Desktop/rscratch/arrow/.venv") packageVersion("arrow") #> [1] '14.0.0.2' packageVersion("reticulate") #> [1] '1.35.0' pa <- reticulate::import("pyarrow") pa[["__version__"]] #> [1] "16.0.0.dev92+geafcff7a5" reticulate::r_to_py(arrow::int32()) #> DataType(int32) ``` ### Are there any user-facing changes? No * Closes: apache#39942 Lead-authored-by: Dewey Dunnington <[email protected]> Co-authored-by: Dewey Dunnington <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

paleolimbot added 3 commits February 7, 2024 11:47

more lenient check for capsule name

a51650a

better

aa57b72

fix check for r_extptr

eafcff7

github-actions bot added Component: Python awaiting committer review Awaiting committer review labels Feb 7, 2024

jorisvandenbossche reviewed Feb 8, 2024

View reviewed changes

python/pyarrow/types.pxi Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 8, 2024

paleolimbot marked this pull request as ready for review February 8, 2024 12:03

Update python/pyarrow/types.pxi

1a42c96

Co-authored-by: Joris Van den Bossche <[email protected]>

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 Feb 8, 2024

pitrou reviewed Feb 8, 2024

View reviewed changes

python/pyarrow/types.pxi Outdated Show resolved Hide resolved

pitrou reviewed Feb 8, 2024

View reviewed changes

Update python/pyarrow/types.pxi

2226486

Co-authored-by: Antoine Pitrou <[email protected]>

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 Feb 8, 2024

add comment

aa3b0f8

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 8, 2024

fix typo

7b721dd

jorisvandenbossche approved these changes Feb 9, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Feb 9, 2024

paleolimbot merged commit abf4fbf into apache:main Feb 9, 2024
11 of 12 checks passed

paleolimbot removed the awaiting merge Awaiting merge label Feb 9, 2024

paleolimbot deleted the python-capsule-name branch February 9, 2024 13:46

assignUser modified the milestone: 15.0.1 Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39942: [Python] Make capsule name check more lenient #39977

GH-39942: [Python] Make capsule name check more lenient #39977

paleolimbot commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 7, 2024

jorisvandenbossche left a comment

pitrou Feb 8, 2024

paleolimbot Feb 8, 2024

jorisvandenbossche Feb 8, 2024

paleolimbot Feb 8, 2024

pitrou Feb 8, 2024

conbench-apache-arrow bot commented Feb 9, 2024

	if capsule_name == NULL or capsule_name == "r_extptr":
	if capsule_name == NULL or capsule_name == b"r_extptr":

GH-39942: [Python] Make capsule name check more lenient #39977

GH-39942: [Python] Make capsule name check more lenient #39977

Conversation

paleolimbot commented Feb 7, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Feb 7, 2024

jorisvandenbossche left a comment

Choose a reason for hiding this comment

pitrou Feb 8, 2024

Choose a reason for hiding this comment

paleolimbot Feb 8, 2024

Choose a reason for hiding this comment

jorisvandenbossche Feb 8, 2024

Choose a reason for hiding this comment

paleolimbot Feb 8, 2024

Choose a reason for hiding this comment

pitrou Feb 8, 2024

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Feb 9, 2024

paleolimbot commented Feb 7, 2024 •

edited

Loading