feat: add ak.from_cudf via Arrow device interface - #3949
Conversation
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3949 |
|
Thanks for contributing!
I'm not sure how useful this is in the awkward context, where the whole point is to have nested/var types. I would say that the natural representation of a cudf Series for numpy-like operations is cupy. It is hard to follow the nested recursive schema with the new column API? Can the existing arrow conversion code be applied? Ref: https://github.com/intake/akimbo/blob/main/src/akimbo/ak_from_cudf.py
Other transformers already depend on pyarrow, can we not use that? |
|
hey,
agreed , I kept the scope here minimal to first sort migration from Series._from_column
ak.from_arrow already handles full Arrow schemas but using series.to_arrow() would introduce a device-to-host copy, losing zero-copy GPU path
nanoarrow was chosen cause again it can consume the arrow_c_device_array capsule without copying although pyarrow operates on host buffers |
I didn't mean literally using that code, but the pattern in that code, recursing into the schema structure. That's what my code in At the time, there were sufficient differences between the "columns" structures and CPU arrow, that the from_arrow code could not be reused. If those differences have reduced, then the code could be reused (by copy&modify or somehow abstracting out the buffers). If not, then nanoarrow won't help for any complex case, and so is not really useful.
Essentially cupy for flat types, no? We already depend on that for GPUs. |
|
Good point. My hesitation is that it relies on column.base_data, base_children, and base_mask, which look internal and may shift as part of rapidsai/cudf#18726. I'll rework the PR around that study pattern, dropping nanoarrow and adding full nested/list/struct/null support. |
Yes, that is exactly the situation. With the internal restructuring in cudf, these accessor attributes and methods will find permanent places to live, so we should be able to implement the conversion in the same manner as before but for the new layout. I don't think too much has changed, except that from the perspective of |
|
Great, thanks for confirming. I'll update this PR to follow the |
|
Sorry for the slow response here.
To be explicit, those attributes are all gone now.
Yes, this should work now. Concretely, an if-else cascade (or a value-based singledispatch-like approach) based on the value of pylibcudf_column.type().id() should allow you to traverse arrow structures in the same way you would expect for CPU arrow. The main edge cases you'll run into are unspecified corners of the Arrow spec itself such as whether empty list columns have one offset or none, or whether null masks can be unallocated altogether for empty columns, etc. |
|
Thanks for the update — good to know those attributes are gone, and helpful to have the concrete direction. I'll implement the traversal based on plc_column.type().id(), following the same if-else cascade as from_arrow but reading buffers from pylibcudf directly via CuPy. I'll handle the Arrow edge cases (empty list offsets, unallocated null masks) as I go and note anything that needs clarification from the cuDF side. Will push an updated version. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
|
The failing test test_2616_use_pyarrow_for_strings.py::test_split_whitespace is unrelated to this PR — none of my changes touch the string operations path. It doesn't reproduce locally with multiple pyarrow versions. |
this is a known flaky test |
|
any updates ? |
|
I'm sorry, I didn't realise there were new changes here! A quick glance suggests it is going well - I see a bunch of decent looking tests. Perhaps we should have even more coverage of deep/nested types and something across the various times units and such. I remember that in arrow, there is a difference between nullable, but mask=None versus nullable but happens to have no nulls - I don't know if that's the case for cudf, but we should explicitly test it. |
|
Thanks I’ll add tests accordingly |
|
@ianna , since you are thinking about extending GPU capabilities, do you have anything o add to a more concerted push for cuDF integration? I am thinking that the most concrete upside is being able to read/write parquet directly with GPU memory. However, some things are more cleanly implemented in cudf (i.e., counterparts of arrow.compute) which might be handy; I am thinking string and time/date manipulations. |
vyasr
left a comment
There was a problem hiding this comment.
Just realized I never committed this comment from weeks ago, sorry...
There is a similar situtation in cudf as well, but it is subtly different. In cudf you can distinguish between whether a null mask is present at all vs whether the null count is zero. The null count is always zero if there is no mask, but you could also have a mask with all valid elements. In our representation "nullable" is an indication of whether a column's null mask has been allocated at all. |
ianna
left a comment
There was a problem hiding this comment.
@aashirvad08 - thanks for working on this! I will let @martindurant to have the final word on it 👍
martindurant
left a comment
There was a problem hiding this comment.
The shape of everything here looks good. I had some confusion about the split of functionality between _connect.cudf and ak_from_cudf modules , and several smaller questions.
| - "src/awkward/_connect/cudf.py" | ||
| - "src/awkward/operations/ak_from_cudf.py" |
| series (cudf.Series): The cuDF Series to convert into a low-level | ||
| Awkward layout. |
There was a problem hiding this comment.
We may also wish to accept dataframes, which are equivalent to a series with top-level record structure. The only difference is possible arrow metadata on the columns. akimbo does this somewhere.
(can be left for the future)
There was a problem hiding this comment.
Already did cudf.DataFrame builds a top-level RecordArray preserving column order
| num_children = getattr(col, "num_children", None) | ||
| if callable(num_children): | ||
| return num_children() | ||
| elif num_children is not None: | ||
| return num_children | ||
| else: | ||
| return len(col.children()) |
There was a problem hiding this comment.
This also seems to crop up a lot
| except AttributeError as err: | ||
| raise RuntimeError( | ||
| "cudf.Series.to_pylibcudf() is required by ak.from_cudf. " | ||
| "Please use cudf >= 25.02." |
There was a problem hiding this comment.
Don't we already check on import?
There was a problem hiding this comment.
this check is specifically for Series.to_pylibcudf() being version-gated
| def _dataframe_to_layout(dataframe): | ||
| fields = list(dataframe.columns) | ||
| contents = [ | ||
| _column_to_layout(_to_pylibcudf_column(dataframe[name])) for name in fields | ||
| ] | ||
| return RecordArray(contents, fields, length=len(dataframe)) |
There was a problem hiding this comment.
OK, sorry, you do handle dataframes
| return out | ||
|
|
||
|
|
||
| def _column_to_layout(col): |
There was a problem hiding this comment.
What's the difference between work done here and in _conect.cudf.recurse ?
There was a problem hiding this comment.
you're right ,I’ll consolidate this into _connect/cudf.py so there is only one path
|
Please ping me when I should review again |
|
yep ill pin when I push changes |
|
Would you mind running pytest with coverage on the new module? We don't need to cover it all, but it would be good to record where we're up to. |
|
sure, will do |
|
I am hoping to get some time this week on my GPU to try this out together with akimbo - thank you for waiting! |
|
sounds good , let me know if u need any help from my side |
|
I am finding that there have been some upstream changes in [[py]lib]cudf 26.06.01 ... I have some running patches, but this may take some work. |
|
See aashirvad08#1 for some changes that appear necessary (not coded nicely, just to demonstrate). I see that the existing to_cudf is version guarded because pylibcudf was still expected when that came out. I can look at it - that side will be needed too for anything in |
|
thanks for putting this together , ill go through it and push the updated version |
|
I think I'm done with my changes, and coverage should be better here now (so no need to ignore in the config). The diff in my PR looks huge because it includes upstream changes. If you update this branch, it should look far saner. |
Summary
Adds
ak.from_cudf(series)— a new public function that converts acudf.Seriesinto anak.Arrayon GPU using only public pylibcudf APIs,with no device-to-host copy and no cuDF internal APIs.
Motivation
The existing
ak.to_cudfpath depends onSeries._from_column, a privatecuDF API tied to column internals that are evolving as part of the cuDF
column refactor (rapidsai/cudf#18726).
A stable public alternative is now available via
pylibcudf, together with the Arrow C Device Interface.This PR implements the Awkward side of that migration.
Conversion path:
series.to_pylibcudf()
└─ recurse(col) dispatches on col.type().id()
├─ primitive → NumpyArray via col.data_buffer() + cp.asarray()
├─ bool8 → NumpyArray (byte → bool cast)
├─ list → ListOffsetArray via col.offsets() + col.child(0)
├─ struct → RecordArray via col.child(i) for each field
└─ string → ListOffsetArray[char] via col.child(0) + col.data_buffer()
└─ _finalize() wraps nullable columns with BitMaskedArray.simplified()
Related to #3328
Closes #3605