You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2024/02/07 16:12:17 UTC
[PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
paleolimbot opened a new pull request, #39977:
URL: https://github.com/apache/arrow/pull/39977
### 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("../.venv")
#> Error in reticulate::use_virtualenv("../.venv"): Directory ../.venv is not a Python virtualenv
packageVersion("arrow")
#> [1] '14.0.0.2'
packageVersion("reticulate")
#> [1] '1.34.0'
pa <- reticulate::import("pyarrow")
pa[["__version__"]]
#> [1] "16.0.0.dev86+gc9f6e0432.d20240207"
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1483488182
##########
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":
Review Comment:
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 🤷
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1483259377
##########
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":
Review Comment:
Did you check that `capsule_name == "r_extptr"` works as expected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1483298455
##########
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":
Review Comment:
```suggestion
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1482925859
##########
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 strcmp(capsule_name, "r_extptr") == 0:
Review Comment:
I'll give it another pass!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1483260834
##########
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":
+ c_ptr = PyCapsule_GetPointer(v, capsule_name)
+ else:
+ capsule_name_str = capsule_name.decode("UTF-8")
+ raise ValueError(
+ f"Can't convert PyCapsule with name '{capsule_name_str}' to pointer address"
+ )
Review Comment:
utf8 is the default encoding when decoding.
```suggestion
capsule_name_str = capsule_name.decode()
raise ValueError(
f"Can't convert PyCapsule with name '{capsule_name_str}' to pointer address"
)
```
##########
python/pyarrow/types.pxi:
##########
@@ -15,7 +15,15 @@
# specific language governing permissions and limitations
# under the License.
-from cpython.pycapsule cimport PyCapsule_CheckExact, PyCapsule_GetPointer, PyCapsule_New, PyCapsule_IsValid
+from cpython.pycapsule cimport (
+ PyCapsule_CheckExact,
+ PyCapsule_GetPointer,
+ PyCapsule_GetName,
+ PyCapsule_New,
+ PyCapsule_IsValid
+)
+
+from libc.string cimport strcmp
Review Comment:
This can be removed now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #39977:
URL: https://github.com/apache/arrow/pull/39977
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1482619339
##########
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 strcmp(capsule_name, "r_extptr") == 0:
Review Comment:
```suggestion
if capsule_name == NULL or capsule_name == "r_extptr":
```
That doesn't work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39977:
URL: https://github.com/apache/arrow/pull/39977#issuecomment-1932387410
:warning: GitHub issue #39942 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1482913096
##########
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 strcmp(capsule_name, "r_extptr") == 0:
Review Comment:
("transparently" was maybe the wrong word choice ;))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1482912440
##########
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 strcmp(capsule_name, "r_extptr") == 0:
Review Comment:
We have a `frombytes` helper that you could also use here `frombytes(capsule_name) == "r_extptr`.
Cython generally converts between python and c objects transparently (in which case `capsule_name == b"r_extptr"` might work out of the box), although in the case of a C string (`char*`) it might be that an explicit assignment is needed from reading the docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1482864452
##########
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 strcmp(capsule_name, "r_extptr") == 0:
Review Comment:
In C that would always return false (because it would compare memory addresses and not string content). I would be surprised if Cython overrode that? Probably `bytes(capsule_name) == "r_extptr"` would work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #39977:
URL: https://github.com/apache/arrow/pull/39977#discussion_r1483267020
##########
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":
Review Comment:
I still need to check!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39942: [Python] Make capsule name check more lenient [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39977:
URL: https://github.com/apache/arrow/pull/39977#issuecomment-1936221255
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit abf4fbf924391149ba2717aa9b57090094271a5d.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/21414242640) has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org