Forward-merge release/26.08 into main - #23531
Conversation
## Description Upgrades nvcomp to version 5.3.0.16. Depends on rapidsai/rmm#2462. Replaces NVIDIA#23294. ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes. --------- Co-authored-by: Bradley Dice <bdice@bradleydice.com> Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
## Description On `main` and `release/26.08`, the following will error reliably: ``` pytest -p no:randomly -n0 \ python/cudf_polars/tests/streaming/test_ray.py::test_yields_engine \ python/cudf_polars/tests/quent/test_quent_integration.py ``` In CI, it will fail if a worker that previously happened to run one of the ray tests with 0 GPUs happened to worksteal the quent integration tests. This fixes the issue by passing through `ray_init_options` and `ray_num_ranks` when creating the RayEngine for the quent tests, similar to the rest of the tests Closes NVIDIA#23441 ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes.
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesnvCOMP dependency and linking changes
Ray integration fixture configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/CMakeLists.txt`:
- Around line 1310-1322: Update the nvcomp absorption branch around
target_link_libraries and _absorbed_nvcomp so it appends the nvcomp dependency
to the export list before setting _absorbed_nvcomp to TRUE. Ensure nvcomp is
exported exactly once when only nvcomp::nvcomp_static exists, allowing the Java
build’s nvcomp::nvcomp reference to resolve.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7262b6de-2ad2-4550-b775-fbad84d35388
📒 Files selected for processing (11)
conda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamlconda/recipes/libcudf/conda_build_config.yamlcpp/CMakeLists.txtcpp/cmake/thirdparty/get_nvcomp.cmakedependencies.yamljava/src/main/native/CMakeLists.txtpython/cudf_polars/tests/quent/test_quent_integration.pypython/libcudf/pyproject.toml
| set_property( | ||
| TARGET cudf | ||
| APPEND | ||
| PROPERTY CUDF_STATIC_LINKED_PROJECTS "${_dep_link}" | ||
| ) | ||
|
|
||
| # nvcomp_static has done a WHOLE_ARCHIVE on rmm and rapids_logger itself. Therefore it needs to | ||
| # come after our WHOLE_ARCHIVE otherwise we will get duplicate symbols for rmm and/or | ||
| # rapids_logger | ||
| if(TARGET nvcomp::nvcomp_static) | ||
| target_link_libraries(${_dep_link} INTERFACE $<BUILD_LOCAL_INTERFACE:nvcomp::nvcomp_static>) | ||
| set(_absorbed_nvcomp TRUE) | ||
| endif() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Trace writes to libcudf static-link metadata.
rg -n -C 6 \
'CUDF_STATIC_LINKED_PROJECTS|_absorbed_nvcomp|nvcomp::nvcomp_static|CUDF_nvcomp_TARGET' \
cpp/CMakeLists.txt
# Trace the Java consumer decision.
rg -n -C 6 \
'cudf_static_linked_projects|nvcomp IN_LIST|nvcomp::nvcomp_static|nvcomp::nvcomp' \
java/src/main/native/CMakeLists.txtRepository: rapidsai/cudf
Length of output: 4826
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- cpp/CMakeLists.txt relevant sections ---'
sed -n '1240,1400p' cpp/CMakeLists.txt
printf '%s\n' '--- Java native configuration and nvcomp target setup ---'
sed -n '80,125p' java/src/main/native/CMakeLists.txt
sed -n '250,300p' java/src/main/native/CMakeLists.txt
printf '%s\n' '--- all metadata consumers and nvcomp target references ---'
rg -n -C 4 \
'CUDF_STATIC_LINKED_PROJECTS|CUDF_JNI_LIBCUDF_STATIC|CUDF_nvcomp_TARGET|nvcomp::nvcomp(_static)?' \
--glob 'CMakeLists.txt' --glob '*.cmake' .Repository: rapidsai/cudf
Length of output: 22714
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
cpp = Path("cpp/CMakeLists.txt").read_text()
java = Path("java/src/main/native/CMakeLists.txt").read_text()
loop = cpp[cpp.index("set(_absorbed_deps"):cpp.index("if(TARGET spdlog::spdlog)")]
assert "set(_absorbed_nvcomp FALSE)" in loop
assert "if(TARGET nvcomp::nvcomp_static)" in loop
assert "set(_absorbed_nvcomp TRUE)" in loop
assert 'PROPERTY CUDF_STATIC_LINKED_PROJECTS "${_dep_link}"' in loop
assert 'PROPERTY CUDF_STATIC_LINKED_PROJECTS "nvcomp"' not in loop
fallback = cpp[cpp.index("if(NOT _absorbed_nvcomp)"):cpp.index("# Add Conda library")]
assert 'PROPERTY CUDF_STATIC_LINKED_PROJECTS "nvcomp"' in fallback
consumer = java[java.index("get_target_property(cudf_static_linked_projects"):
java.index("target_link_libraries(cudfjni PRIVATE nvtx3::nvtx3-cpp)")]
assert "if(nvcomp IN_LIST cudf_static_linked_projects)" in consumer
assert "nvcomp::nvcomp_static" in consumer
assert "nvcomp::nvcomp" in consumer
# Model the proposed guard across the two absorbed dependencies.
for static_deps in ([], ["rmm"], ["rapids_logger"], ["rmm", "rapids_logger"]):
absorbed = False
entries = []
for dep in static_deps:
if not absorbed:
entries.append(dep)
if True: # nvcomp::nvcomp_static exists
if not absorbed:
entries.append("nvcomp")
absorbed = True
assert entries.count("nvcomp") == (1 if static_deps else 0)
print("The current loop omits nvcomp metadata on the absorption path.")
print("The proposed guard records nvcomp once for any absorbed static dependency.")
print("The Java consumer uses nvcomp::nvcomp_static only when nvcomp is in the exported list.")
PYRepository: rapidsai/cudf
Length of output: 376
Export nvcomp for the absorption path.
When nvcomp::nvcomp_static exists, this branch sets _absorbed_nvcomp, so the later block does not export nvcomp. The Java build then selects nvcomp::nvcomp; if only the static target exists, CMake cannot resolve that target. Append nvcomp once before setting _absorbed_nvcomp to TRUE.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/CMakeLists.txt` around lines 1310 - 1322, Update the nvcomp absorption
branch around target_link_libraries and _absorbed_nvcomp so it appends the
nvcomp dependency to the export list before setting _absorbed_nvcomp to TRUE.
Ensure nvcomp is exported exactly once when only nvcomp::nvcomp_static exists,
allowing the Java build’s nvcomp::nvcomp reference to resolve.
|
/merge nosquash |
Resolves #23474