refactor(cudf_polars): use cudf_streaming for cudf-specific streaming imports - #22786
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
/ok to test |
|
/ok to test |
Add cudf-streaming and libcudf-streaming wheel downloads to the cudf-polars wheel test scripts so they are installed from CI artifacts rather than resolved from a remote index. - Add wheel-build-cudf-streaming to needs for wheel-tests-cudf-polars and cudf-polars-polars-tests in pr.yaml - Download libcudf_streaming and cudf_streaming wheels in test_wheel_cudf_polars.sh and test_cudf_polars_polars_tests.sh - Add cudf-streaming as a runtime dependency in the cudf-polars conda recipe for conda-based test jobs (docs, narwhals)
8f6d82b to
75f17af
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates move streaming types and partition helpers from rapidsmpf to the new cudf_streaming package, add cudf-streaming to packaging and dependency matrices, and change CI/workflow scripts to build, download, and install libcudf_streaming and cudf_streaming wheels for tests. ChangesStreaming dependency migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
From copilot re: devcontainer failures FWIW: Solution for Failing JobThe failure is a CMake configuration error related to imported target promotion to global scope. The error occurs when building libcudf_kafka after the main cudf build completes. Root CauseWhen libcudf_kafka tries to import cudf's exported config file, it attempts to set SolutionThe fix is in the File to modify: Change needed: # Around line 129, modify the set_target_properties call to exclude IMPORTED_GLOBAL
# for targets that aren't strictly necessary for external consumers
# BEFORE (problematic):
set_target_properties(
cudf::cudftestutil
cudf::cudftestutil_impl
PROPERTIES IMPORTED_GLOBAL TRUE
)
# AFTER (fixed):
# Only mark as IMPORTED_GLOBAL if it's being used in the same build context
if(NOT CMAKE_FIND_PACKAGE_NAME STREQUAL "cudf" OR CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR)
set_target_properties(
cudf::cudftestutil
cudf::cudftestutil_impl
PROPERTIES IMPORTED_GLOBAL TRUE
)
endif() |
|
@Matt711 your agent is wrong. The issue isn't the kafka build, it's the new streaming build. The issue is from cuCascade, it's why we need NVIDIA/cuCascade#141. That PR is now merged, but since rapidsai/devcontainers#715 is also merged the dependency relationship has already been flipped in devcontainers, so we need the PR removing all cudf bits from rapidsmpf merged first now. I'm working through that now in rapidsai/rapidsmpf#1083, there's been some back and forth on the exact scope of that work today so we're trying to sort through it now. |
f67ae5f to
ca414c1
Compare
|
/merge |
This PR updates the cudf_polars execution engine to import cudf-specific streaming primitives from the new
cudf_streamingpackage instead of fromrapidsmpfdirectly.This is part of the ongoing work to make the dependency between cudf and rapidsmpf unidirectional by moving cudf-specific streaming code into the cudf repository.
Dependencies:
Changes:
cudf_streaminginstead ofrapidsmpffor cudf-specific streaming types