Repository navigation
Conversation
Add __reduce__ for Action, Result, BasicAuth, FlightDescriptor, Ticket, Location, FlightEndpoint, SchemaResult, and FlightInfo so they round-trip through pickle (via existing serialize/deserialize, or URI for Location).
|
|
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Full Arrow/Flight builds were not run locally (no Flight-enabled pyarrow on this machine); relying on CI for the Python Flight job.
CI is a scarce resource, especially with the volume of PRs nowadays. The agent should be more than capable of building and testing; I'm not really willing to review if it hasn't done that much.
ump45nose
left a comment
ump45nose
left a comment
There was a problem hiding this comment.
@lidavidm Thanks for the pushback — fair point on CI load.
I built Arrow C++ with ARROW_FLIGHT=ON (bundled gRPC/protobuf/absl/Boost) and a Flight-enabled pyarrow against that install prefix, then ran the pickle coverage locally:
pytest pyarrow/tests/test_flight.py -k pickle→test_pickle_flight_typespassed- also re-ran the related serialize / Location / FlightEndpoint / FlightInfo cases (4 passed)
PR description updated to drop the “relying on CI” note. Happy to run a broader Flight subset if you want a specific list.
(AI-assisted reply.)
|
|
1 similar comment
|
|
github-actions
Bot
added
awaiting changes
| info2 = pickle.loads(pickle.dumps(info)) | ||
| assert info.schema == info2.schema | ||
| assert info.descriptor == info2.descriptor | ||
| assert info.total_bytes == info2.total_bytes | ||
| assert info.total_records == info2.total_records | ||
| assert info.ordered == info2.ordered | ||
| assert info.app_metadata == info2.app_metadata | ||
| assert info.endpoints == info2.endpoints |
There was a problem hiding this comment.
(Why can't we compare directly here?)
There was a problem hiding this comment.
No good reason: I had mirrored the existing serialize/deserialize round-trip test above, but FlightInfo.__eq__ compares the underlying C++ objects, so a direct comparison works. Simplified in 99a7880. (AI-assisted reply.)
Signed-off-by: Vincent <52391318+ump45nose@users.noreply.github.com>
github-actions
Bot
added
awaiting change review
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the Flight value-type pickle support.
No open issues 🎉
What changed in this PR
Adds pickle support for Python Flight value types, addressing GH-35150 so Flight metadata can move across process boundaries.
Changes:
- Implements
__reduce__using existing serialization methods for eight Flight types. - Makes
Locationpickleable by reconstructing it from its URI. - Adds pickle round-trip tests, including nested endpoints, timestamps, and application metadata.
| File | Description |
|---|---|
python/pyarrow/tests/test_flight.py |
Adds pickle round-trip coverage for all nine supported types. |
python/pyarrow/_flight.pyx |
Implements pickle support for Flight value types. |
Review effort: Balanced
Rationale for this change
Fixes #35150. Flight types cannot be pickled because they do not implement
__reduce__, which breaks use cases that move Flight metadata across process boundaries (for example Ray).What changes are included in this PR?
Add
__reduce__for the types that already exposeserialize/deserialize.Are these changes tested?
Added
test_pickle_flight_types.Are there any user-facing changes?
No breaking API changes. Pickling Flight value types that previously failed now works.
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: