fix: update ak.to_cudf for cuDF >= 24.12 column API changes#3801
fix: update ak.to_cudf for cuDF >= 24.12 column API changes#3801X0708a wants to merge 26 commits intoscikit-hep:mainfrom
Conversation
|
I’ve opened a PR that updates ak.to_cudf to use stable cuDF column factory APIs Since I’m on macOS, the tests are skipped locally and validated via CI GPU runners. |
Codecov Report❌ Patch coverage is Additional details and impacted files
🚀 New features to boost your workflow:
|
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3801 |
|
@X0708a - thank you! please update the pr title: Available types:
|
ianna
left a comment
There was a problem hiding this comment.
@X0708a - looks great! Thanks. Please enable the
tests-cuda/test_3051_to_cuda.py::test_jagged XFAIL (cudf internals
changed since v25.12.00) [ 17%]
tests-cuda/test_3051_to_cuda.py::test_nested XFAIL (cudf internals
changed since v25.12.00)
ikrommyd
left a comment
There was a problem hiding this comment.
Thanks @X0708a. Can you please also look at the other layouts too? Search for def _to_cudf. The same method for the other content types probably need similar fixes that are just not caught by tests (to_cudf testing is limited). Feel free to include more tests too :)
ak.to_cudf for cudf >= 24.12
ak.to_cudf for cudf >= 24.12|
I’ve enabled test_jagged and test_nested as XFAIL with a documented reason, while keeping the existing CUDA gating via importorskip. |
|
If your PR fixes the issue, none of those tests should fail. So you need to remove the xfail decorators completely and ensure that the tests pass. That's the core of the issue. |
|
@X0708a if you are just debugging with CI, it will be very difficult to do this. It would be much better if you can test locally for your mental health 🤣. You can maybe just use google colab to test (which offers a free GPU) or any other GPU access point you can get your hands on. |
|
Haha 😄 yeah, I’ve been running things locally as well. |
|
CI failures are due to cuDF raising a RuntimeError during import when Numba is already patched via pynvjitlink. This occurs before pytest.importorskip can trigger. I am adding a guard to skip CUDA tests in this configuration. Core ak.to_cudf logic is unchanged. |
|
Hi, |
2c0ef08 to
43dddef
Compare
972bbe9 to
f68130f
Compare
|
Hi @ianna @ikrommyd |
|
No you hadn't enabled the tests. I have fixed that, merged main and also fixed |
0f95212 to
49adb05
Compare
|
Two unrelated CI issues are blocking - |
|
@X0708a you have literally just force pushed the branch erasing my commits but also deleting your whole progress. You have replaced your branch with the one trying to do the byteswap thingy. It's best for your own good to avoid force-pushing every time git tells you you can't push and understanding why that is before blindly doing |
|
While rebasing locally I force-pushed and overwrote the branch pointer ,sorry for that . |
No worries at all! These things happen when multiple people are touching the same branch. Let’s go with restoring the original branch. It keeps the discussion history in one place. |
|
Restored the branch to commit c289825 as discussed. Thanks. |
|
Your changes right now have nothing to do with cudf. You have byteswap changes here. |
|
I see that this branch has accumulated unrelated changes which is making the diff noisy. |
|
It would be best to recover the branch here. I have pushed the last state of your branch into my own fork to assist you https://github.com/ikrommyd/awkward/tree/aashirvad-last-state. Just force push that branch into your |
c289825 to
ba86708
Compare
|
Force-pushed fix-to-cudf-24-12 to match ikrommyd/aashirvad-last-state. Thanks for setting that up. |
|
Good so now you can continue the implementation from there. Now the tests are actually testing what they should. |
|
Pushed a focused ak.to_cudf fix for cuDF column-construction changes (from_pylibcudf requirement). |
|
I think there's a a couple more |
|
hey ,Pushed updates to harden ak.to_cudf across cuDF changes and expanded CUDA coverage. Removed risky mask handling fallback in NumpyArray._to_cudf and rebuilt masked columns. Generalized constructor compatibility handling for list/string/struct conversions . Added CUDA tests for additional layouts: IndexedArray, UnmaskedArray, EmptyArray, IndexedOptionArray, and ListArray. |
|
Your new tests should not have the pytest skipif mark if they are meant to pass :) |
ikrommyd
left a comment
There was a problem hiding this comment.
On #3814, cudf has been removed from the test config because we needed later numba-cuda meaning that this PR cannot be tested in CI until cudf supports the latest numba-cuda version. They have merged the support on their main branch and remove the pin so probably in the next cudf release. Putting "request changes" until then because this needs to wait.
cuDF >= 24.12 changed internal column constructor signatures, causing
ak.to_cudf to fail when calling NumericalColumn and StructColumn with
the deprecated
data=keyword.This PR updates ak.to_cudf to:
Note: Tests are skipped locally on macOS and validated via CI GPU runners.
@ianna
@ikrommyd