Skip to content

Fix integer overflows in pylibcudf from_column_view_of_arbitrary #18758

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

Merged

Conversation

wence-
Copy link
Contributor

@wence- wence- commented May 12, 2025

Description

Although the number of rows in a column_view cannot exceed size_type, the number of bytes surely can. So fix that.

This fixes multi-GPU errors we are seeing in rapidsmpf when communicating buffers.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- requested review from a team as code owners May 12, 2025 14:44
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. pylibcudf Issues specific to the pylibcudf package labels May 12, 2025
@GPUtester GPUtester moved this to In Progress in cuDF Python May 12, 2025
@wence- wence- added bug Something isn't working non-breaking Non-breaking change labels May 12, 2025
@vyasr
Copy link
Contributor

vyasr commented May 12, 2025

Other sources of overflow were fixed in #18734. Those fixes conflict partially with this one, but this fix is also necessary since that PR did not fix the chars case, so this PR is still necessary.

wence- added 4 commits May 12, 2025 15:00
libcudf size_of returns a size_t, not a size_type.
Can't unilaterally use int32 offsets.
Rather than reinventing the wheel, use the libcudf functionality.
@wence- wence- force-pushed the wence/fix/pylibcudf-integer-overflow branch from 4f2c8bf to 919ab40 Compare May 12, 2025 15:06
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing up all the bindings for size_of's return type as well, I didn't do that in my PR to get a fix in quickly but I agree that it is necessary.

Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default empty constructor for cudf::strings_column_view was left out on purpose but I understand the struggle that is Cython.

@wence-
Copy link
Contributor Author

wence- commented May 12, 2025

The default empty constructor for cudf::strings_column_view was left out on purpose but I understand the struggle that is Cython.

Thanks, yes.

@wence-
Copy link
Contributor Author

wence- commented May 12, 2025

Actually, by using cython's cpp_locals directive, I can avoid the need for the nullary ctor...

@github-actions github-actions bot removed the libcudf Affects libcudf (C++/CUDA) code. label May 12, 2025
@wence-
Copy link
Contributor Author

wence- commented May 12, 2025

Looks like I screwed something up :(

@quasiben
Copy link
Member

Currently failing with errors like:

  File "interop.pyx", line 129, in pylibcudf.interop._from_arrow_column
    return Column(pyarrow_object)
  File "column.pyx", line 249, in pylibcudf.column.Column.__init__
    self._init(obj, *args, **kwargs)
  File "/opt/conda/envs/test/lib/python3.10/functools.py", line 926, in _method
    return method.__get__(obj, cls)(*args, **kwargs)
  File "column.pyx", line 324, in pylibcudf.column.Column._
    result.col.swap(c_result)
AttributeError: C++ attribute 'col' is not initialized

@Matt711
Copy link
Contributor

Matt711 commented May 12, 2025

At least some of the tests pass if you initialize c_children

diff --git a/python/pylibcudf/pylibcudf/column.pyx b/python/pylibcudf/pylibcudf/column.pyx
index 41d7faad08..c5c056c049 100644
--- a/python/pylibcudf/pylibcudf/column.pyx
+++ b/python/pylibcudf/pylibcudf/column.pyx
@@ -353,7 +353,7 @@ cdef class Column:
 
         # TODO: Check if children can ever change. If not, this could be
         # computed once in the constructor and always be reused.
-        cdef vector[column_view] c_children
+        cdef vector[column_view] c_children = vector[column_view]()
         with gil:
             if self._children is not None:
                 for child in self._children:
@@ -388,7 +388,7 @@ cdef class Column:
         if self._mask is not None:
             null_mask = <bitmask_type*>self._mask.ptr
 
-        cdef vector[mutable_column_view] c_children
+        cdef vector[mutable_column_view] c_children = vector[mutable_column_view]()
         with gil:
             if self._children is not None:
                 for child in self._children:

@vyasr
Copy link
Contributor

vyasr commented May 12, 2025

Given the nature of the changeset, some bug with the cpp_locals directive seems like the most likely culprit.

@Matt711
Copy link
Contributor

Matt711 commented May 12, 2025

Given the nature of the changeset, some bug with the cpp_locals directive seems like the most likely culprit.

Ok will try reverting ea10cb3

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 12, 2025
@wence-
Copy link
Contributor Author

wence- commented May 13, 2025

Given the nature of the changeset, some bug with the cpp_locals directive seems like the most likely culprit.

Yeah, thanks all.

@wence-
Copy link
Contributor Author

wence- commented May 13, 2025

/merge

@rapids-bot rapids-bot bot merged commit 3fc67d9 into rapidsai:branch-25.06 May 13, 2025
127 checks passed
@wence- wence- deleted the wence/fix/pylibcudf-integer-overflow branch May 13, 2025 08:22
@github-project-automation github-project-automation bot moved this from In Progress to Done in cuDF Python May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants