Skip to content

fix(cuDF): Fix debug build failure#17011

Open
pramodsatya wants to merge 2 commits intofacebookincubator:mainfrom
pramodsatya:fix_cudf_dbg_build
Open

fix(cuDF): Fix debug build failure#17011
pramodsatya wants to merge 2 commits intofacebookincubator:mainfrom
pramodsatya:fix_cudf_dbg_build

Conversation

@pramodsatya
Copy link
Copy Markdown
Collaborator

@pramodsatya pramodsatya commented Apr 2, 2026

Problem

Build error for T = double:

/home/vpcuser/velox/./velox/experimental/cudf/expression/AstUtils.h: In function ‘std::unique_ptr<cudf::scalar> facebook::velox::cudf_velox::makeScalarFromValue(const facebook::velox::TypePtr&, T, bool, std::optional<cudf::type_id>) [with T = double]’:
/home/vpcuser/velox/./velox/experimental/cudf/expression/AstUtils.h:79:41: error: call to ‘cudf::get_default_stream’ declared with attribute error: cudf default stream argument used. Pass stream explicitly.
   79 |   auto stream = cudf::get_default_stream();
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/vpcuser/velox/./velox/experimental/cudf/expression/AstUtils.h:80:8: error: call to ‘cudf::get_current_device_resource_ref’ declared with attribute error: cudf default memory resource argument used. Pass mr explicitly.
   80 |   auto mr = cudf::get_current_device_resource_ref();

makeScalarFromValue() in AstUtils.h calls cudf::get_default_stream() and cudf::get_current_device_resource_ref(). When any .cpp file includes both CudfNoDefaults.h and AstUtils.h (e.g. CudfHashJoin.cpp), these calls hit the __attribute__((error)) redeclarations that guard against accidental default argument usage.

In Release builds (-O2), GCC's dead-code elimination removes unused template instantiations before the error fires. In Debug builds (-O0), all instantiations are preserved, triggering a hard compile error.

Solution

Replaces the two calls with existing approved alternatives:

  • cudf::get_default_stream()cudf::get_default_stream(cudf::allow_default_stream) (from CudfDefaultStreamOverload.h)
  • cudf::get_current_device_resource_ref()rmm::mr::get_current_device_resource() (direct RMM call, same pattern as get_temp_mr() in CudfNoDefaults.h)

Both return identical values at runtime — zero performance impact.

Changes

  • velox/experimental/cudf/expression/AstUtils.h: Update makeScalarFromValue() to use poison-safe alternatives

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 2, 2026
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 2, 2026

Deploy Preview for meta-velox ready!

Name Link
🔨 Latest commit 816edb6
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/69cfd75199acb90008a0259a
😎 Deploy Preview https://deploy-preview-17011--meta-velox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@pramodsatya pramodsatya requested a review from czentgr April 2, 2026 15:21
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Build Impact Analysis

Full build recommended. Files outside the dependency graph changed:

  • velox/experimental/cudf/expression/AstUtils.h

These directories are not fully covered by the dependency graph. A full build is the safest option.

cmake --build _build/release

Fast path • Graph from main@65800681f99ac53fe2cb3d5c121dd8ba611e385b

@pramodsatya pramodsatya requested a review from majetideepak April 3, 2026 15:05
auto stream = cudf::get_default_stream();
auto mr = cudf::get_current_device_resource_ref();
auto stream = cudf::get_default_stream(cudf::allow_default_stream);
auto mr = rmm::mr::get_current_device_resource();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The API at velox/experimental/cudf/CudfNoDefaults.h says we could use get_temp_mr() instead.
@bdice can you please take a look at this fix? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants