Skip to content

fix(Python): Ensure we don't call cuMemAlloc with 0 bytesize#534

Merged
paleolimbot merged 2 commits intoapache:mainfrom
shwina:fix-mem-alloc-0
Jun 20, 2024
Merged

fix(Python): Ensure we don't call cuMemAlloc with 0 bytesize#534
paleolimbot merged 2 commits intoapache:mainfrom
shwina:fix-mem-alloc-0

Conversation

@shwina
Copy link
Copy Markdown
Contributor

@shwina shwina commented Jun 20, 2024

According to the CUDA docs for cuMemAlloc:

If bytesize is 0, cuMemAlloc() returns CUDA_ERROR_INVALID_VALUE.

We end up calling cuMemAlloc() with 0 bytesize when allocating device buffers with no null mask. Thus, the following change was causing nanoarrow_device_cuda_test to fail:

@@ -207,7 +207,8 @@ TEST_P(StringTypeParameterizedTestFixture, ArrowDeviceCudaArrayViewString) {
   ASSERT_EQ(ArrowArrayStartAppending(&array), NANOARROW_OK);
   ASSERT_EQ(ArrowArrayAppendString(&array, ArrowCharView("abc")), NANOARROW_OK);
   ASSERT_EQ(ArrowArrayAppendString(&array, ArrowCharView("defg")), NANOARROW_OK);
-  ASSERT_EQ(ArrowArrayAppendNull(&array, 1), NANOARROW_OK);
+  ASSERT_EQ(ArrowArrayAppendString(&array, ArrowCharView("defg")), NANOARROW_OK);
+  // ASSERT_EQ(ArrowArrayAppendNull(&array, 1), NANOARROW_OK);
   ASSERT_EQ(ArrowArrayFinishBuildingDefault(&array, nullptr), NANOARROW_OK);
 
   ASSERT_EQ(ArrowDeviceArrayInit(cpu, &device_array, &array, nullptr), NANOARROW_OK);

In this PR, I've fixed this by simply skipping the call to cuMemAlloc. The resulting buffer will have nullptr as its data member and 0 as its size_bytes, which I believe is the desired outcome.

I also modified the test above to include cases with no nulls.

@shwina shwina mentioned this pull request Jun 20, 2024
Merged
@shwina
Copy link
Copy Markdown
Contributor Author

shwina commented Jun 20, 2024

I see that this logic is being overhauled in #509. If the maintainers want to close this PR out, that's fine by me!

It's probably worth including the changes to the tests though.

paleolimbot
paleolimbot approved these changes Jun 20, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

(Even though we're rewriting this, this is a nice quality-of-life improvement in the tests!)

@paleolimbot paleolimbot merged commit c651e84 into apache:main Jun 20, 2024
@paleolimbot paleolimbot added this to the nanoarrow 0.6.0 milestone Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paleolimbot paleolimbot paleolimbot approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

nanoarrow 0.6.0

Development

Successfully merging this pull request may close these issues.

2 participants

@shwina @paleolimbot