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

python/adbc_driver_manager: use PyCapsule for handles to C structs #70

Closed
lidavidm opened this issue Aug 19, 2022 · 23 comments
Closed

python/adbc_driver_manager: use PyCapsule for handles to C structs #70

lidavidm opened this issue Aug 19, 2022 · 23 comments

Comments

@lidavidm
Copy link
Member

We should try to use the 'native' type of the C API. Apparently, this will also ease interoperability with R.

@pitrou
Copy link
Member

pitrou commented Aug 19, 2022

Yes, IIRC the official R-Python interop library supports shipping PyCapsule to R (cc @paleolimbot ).

@paleolimbot
Copy link
Member

@pitrou
Copy link
Member

pitrou commented Aug 19, 2022

Ah, that doesn't sound helpful actually, because reticulate is only considering the case where the "context" of the capsule (an embedded opaque pointer) was created by R. Here, the case AFAIU would be to pass a Python-created PyCapsule to R. Casting a Python-managed opaque pointer to a R SEXP is probably a bad idea (though who knows :-)).

@pitrou
Copy link
Member

pitrou commented Aug 19, 2022

I mean, the code reference you gave was very helpful :-)

@paleolimbot
Copy link
Member

I see..that's the other direction (R to Python). I assumed that would roundtrip but apparently it doesn't ( https://github.com/rstudio/reticulate/blob/74d139b1772d29dce24b22a828f2972ac97abacf/src/python.cpp#L755-L756 ).

Casting a Python-managed opaque pointer to a R SEXP is probably a bad idea (though who knows :-)).

I think in this case that's exactly what we want! It's easy to work around, though as long as we can get an address (similar to what the Arrow R package does except in this case the R external pointer will keep a Python object reference).

@pitrou
Copy link
Member

pitrou commented Aug 19, 2022

Well, py_to_r doesn't seem to handle capsules at all? What am I missing?

@paleolimbot
Copy link
Member

Nothing! reticulate doesn't handle them. In this case the R package would implement py_to_r.some.qualified.python.type.schema() and return an external pointer classed as nanoarrow_schema (for example). My point was that the semantics should be exactly the same as if the transformation was automatic (at least in this case).

@lidavidm lidavidm added this to the ADBC Libraries 0.8.0 milestone Oct 18, 2023
@lidavidm lidavidm changed the title [Python] Try using PyCapsule for handles to C structs python/adbc_driver_manager: use PyCapsule for handles to C structs Oct 30, 2023
@lidavidm
Copy link
Member Author

lidavidm commented Nov 1, 2023

Going to punt this since I think if we start returning PyCapsule, you won't be able to work with that unless you also have PyArrow 14, and I don't want to bring up the minimum PyArrow version so much right now

@pitrou
Copy link
Member

pitrou commented Nov 1, 2023

I'm not sure I understand the relationship with PyArrow here. What are the PyCapsules in this issue supposed to convey?

@lidavidm
Copy link
Member Author

lidavidm commented Nov 1, 2023

We return custom wrappers around C Data Interface objects, we want to return compliant PyCapsules (and Joris already prototyped that), but first I also want some way to make the PyCapsule also work with versions of PyArrow that can't import them (since they're opaque to Python code, right?)

@pitrou
Copy link
Member

pitrou commented Nov 2, 2023

since they're opaque to Python code, right?

Indeed, they are, except using ctypes or cffi.

@paleolimbot
Copy link
Member

I think the idiom to apply would be to implement __arrow_c_stream__() on the statement wrapper and have that be the one and only thing that returns a capsule?

@lidavidm
Copy link
Member Author

lidavidm commented Nov 2, 2023

But we have other methods (like get_table_schema) that need to return capsules.

@paleolimbot
Copy link
Member

But we have other methods (like get_table_schema) that need to return capsules.

I think there it would return an object that implements __arrow_c_schema__()? I think the intention is that a user would do:

pyarrow.schema(conn.get_table_schema())

@lidavidm
Copy link
Member Author

lidavidm commented Nov 2, 2023

That still requires you to have PyArrow 14, which is the main thing I wanted to avoid

@lidavidm
Copy link
Member Author

lidavidm commented Nov 2, 2023

I guess that means I'd keep the existing objects we have (that expose the raw address), and just have them implement the new dunder methods in addition

@lidavidm
Copy link
Member Author

lidavidm commented Nov 8, 2023

Well, given the PyArrow CVE I think the next release of ADBC will have to require 14.0.1 as a baseline.

@pitrou
Copy link
Member

pitrou commented Nov 8, 2023

There's the hotfix as well if you won't want to require 14.0.1.

@lidavidm
Copy link
Member Author

lidavidm commented Nov 8, 2023

Right - but I think it behooves another Arrow project to follow our own advice 🙂

@jorisvandenbossche
Copy link
Member

Started looking at this again, exploring how we can adopt the capsule protocol in ADBC python.

The low-level interface (_lib.pyx) has no dependency on pyarrow, and currently has ArrowSchemaHandle / ArrowArrayHandle / ArrowArrayStreamHandle classes that hold the C structs, and those classes are returned in the various objects.

I think in theory we could replace those current Handle classes with PyCapsules directly (and in the higher-level code, we can still extract the pointer from the PyCapsule when having to deal with a pyarrow version that doesn't support capsules directly).
However, those handle objects are currently exposed in the public API, so would we be OK with just removing them? (I don't know if there are external libraries that use those directly?)
We could also keep them, add add the dunder methods to them so they are importable without having to access the integer address. Which is what David mentioned above (#70 (comment)), I assume:

I guess that means I'd keep the existing objects we have (that expose the raw address), and just have them implement the new dunder methods in addition

One advantage of keeping some generic wrapper/handle class that has the dunders vs returning pycapsules directly, is that the return values of the low-level interface can then more easily be passed to a library that expects an object with the dunder defined instead of the capsules directly (i.e. how we currently implemented support for this in pyarrow, e.g. pyarrow.array(..) checks for __arrow_c_array__ attribute on the passed object, but doesn't accept capsules directly, xref apache/arrow#38010)


The DBAPI is much more tied to pyarrow, so I don't know if, on the short term, we want to enable getting arrow data without a dependency on pyarrow and only relying on the capsule protocol. That would require quite some changes.

Just getting an overview for myself:

  • Connection.adbc_get_info and adbc_get_table_types -> returns info as a dict or list, but under the hood consumes the stream with pyarrow and convert to pylist -> this doesn't expose pyarrow data, so on the short term this could continue to require pyarrow as a runtime dependency
  • Connection.adbc_get_objects returns a pyarrow.RecordBatchReader
  • Connection.adbc_get_table_schema returns a pyarrow.Schema
  • Cursor._bind / _prepare_execute / execute -> currently accepts pyarrow RecordBatch/Table as parameters, but this can be expanded to any object that has the array or array stream protocol
  • Cursor.execute / adbc_read_partition sets a _result object to _RowIteratorof a AdbcRecordBatchReader. This reader subclasses pyarrow.RecordBatchReader (to ensure adbc error messages are properly propagated)
  • Cursor.adbc_ingest accepts pyarrow RecordBatch, Table or RecordBatchReader as data, this can also be generalized to any object that supports the protocol
  • Cursor.adbc_execute_schema / adbc_prepare returns a pyarrow.Schema
  • Cursor.fetchallarrow / fetch_arrow_table return a pyarrow.Table
  • Cursor.fetch_record_batch returns a pyarrow.RecordBatchReader
  • Cursor.fetchone / fetchmany / fetchall use the _RowIterator, which uses the pyarrow RecordBatchReader to iterate over the data -> this only uses pyarrow under the hood for converting the data to Python tuples, and so on the short term can continue to do that with a pyarrow runtime dependency.

Changing the methods that currently return a pyarrow Schema/Table/RecordBatchReader to return a generic object implementing the dunders seems too much of a breaking change (and would also be much less convenient for pyarrow users, e.g. requiring a users to do pyarrow.schema(conn.get_table_schema()) instead of conn.get_table_schema()).
But would we want to add additional method variants of each of those that return something generic like that?

Of course, assuming you have pyarrow 14+ installed, by returning pyarrow objects which implement the dunders, we of course also automatically make the capsule protocol available in ADBC. So it's more a question to what extent we want to make it possible to use the DBAPI layer without pyarrow.


So in summary, short term I think I would do:

  • Add the dunder methods to the handle classes of the low-level interface, which already enables using the low-level interface without pyarrow and with the capsule protocol
  • In the places that accept data (eg ingest), generalize to accept objects that implement the dunders in addition to hardcoded support for pyarrow

And then longer term we can think about whether we also want to enable using the DBAPI in some form without a runtime dependency on pyarrow.

@pitrou
Copy link
Member

pitrou commented Nov 24, 2023

So in summary, short term I think I would do:

  • Add the dunder methods to the handle classes of the low-level interface, which already enables using the low-level interface without pyarrow and with the capsule protocol

  • In the places that accept data (eg ingest), generalize to accept objects that implement the dunders in addition to hardcoded support for pyarrow

+1

@lidavidm
Copy link
Member Author

Thanks! Yup, this is my plan (I started working on it over holiday but realized it was...holiday)

lidavidm added a commit that referenced this issue Dec 13, 2023
… Capsule interface (#1346)

Addresses #70

This PR adds the dunder methods to the Handle classes of the low-level
interface (which already enables using the low-level interface without
pyarrow and with the capsule protocol).

And secondly, in the places that accept data (eg ingest/bind), it now
also accepts objects that implement the dunders in addition to hardcoded
support for pyarrow.

---------

Co-authored-by: David Li <[email protected]>
@lidavidm lidavidm removed this from the ADBC Libraries 0.9.0 milestone Dec 19, 2023
@lidavidm
Copy link
Member Author

Completed in #1346

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

No branches or pull requests

4 participants