refactor: migrate from_feather to pyarrow.ipc API#4204
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
|
ianna
left a comment
There was a problem hiding this comment.
Hi @pranjan2023 . Thanks for your contribution!
I noticed that the new implementation changes the read behavior as well as moving to the non-deprecated IPC API.
Previously:
pyarrow.feather.read_table(path, columns=columns, ...)could apply column selection during the read. The new implementation always calls:
arrow_table = reader.read_all()and then selects the requested columns afterward.
Since reverting is not an option, could you please test whether this change reproduces the coverage hang locally and whether thread usage is involved?
coverage run -m pytest -vv -s \
tests/test_1620_layout_builders.py::test_data_frame_listoffset_integersThen try the same test with Arrow threading disabled:
ARROW_NUM_THREADS=1 \
coverage run -m pytest -vv -s \
tests/test_1620_layout_builders.py::test_data_frame_listoffset_integersIt would also be useful to run the Feather-related tests directly:
coverage run -m pytest -vv -s \
tests/test_*.py -k "feather or from_feather"If the single test passes, could you also try the full coverage job with durations enabled?
ARROW_NUM_THREADS=1 \
coverage run -m pytest -vv --durations=30The CI process exits with code 129 (SIGHUP), so I am trying to determine whether the new read_all() path or PyArrow thread usage is causing the job to hang or become significantly slower.
|
Hi, thanks for the detailed repro steps. I ran the requested tests locally and here's what I found. SetupSince Results
SummaryI wasn't able to reproduce the CI hang or observe any meaningful slowdown under any of the combinations I tried:
Based on these results, the I've attached the complete output from all four runs in awkward_feather_investigation.log. Environment
Since I can't reproduce the issue locally, it may be worth comparing the CI environment (PyArrow version, operating system, resource limits, or timeout configuration) against my setup. If there's additional CI-side information available, such as the exact job configuration or a stack trace/thread dump from the hung process, I'd be happy to investigate further. |
|
I don't yet know the root cause. I ran the full local test suite (including the specific test_1620_layout_builders.py test, with and without ARROW_NUM_THREADS=1, and with PyROOT installed) multiple times and couldn't reproduce a hang, crash, or segfault locally . So whatever's happening on the GPU runner may be CI/environment-specific (GPU driver, CUDA memory limits, runner resource constraints) rather than a bug in the changed code itself, but I can't confirm that without more visibility into the actual GPU job's logs. |
Thanks for looking into it. It would be useful to understand why it is happening. It is possible that the job just times out. The question is by how much we expect to delay the ending and if it is reasonable. Thanks! |
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR4204 |
This PR updates both
ak_to_featherandak_from_featherto use the non-deprecatedpyarrow.ipcAPI, replacing the deprecated Feather APIs and resolving the associated deprecation warnings.Changes
ak.to_featherpyarrow.ipc.RecordBatchFileWriter.ak.from_featherpyarrow.feather.FeatherReaderwithpyarrow.ipc.open_fileandIpcReadOptions.Together, these changes migrate Awkward's Feather I/O implementation to the supported IPC API while maintaining compatibility with the previous behavior.
Fixes #4199.