Remove rmm::device_buffer forward declaration from types.hpp - #23373
Conversation
davidwendt
added
libcudf
📝 WalkthroughWalkthroughThe change adds Java CUDA/architecture build automation, a stack-aware timeout wrapper for Polars tests, a masked numeric cast fix, related pandas testing updates, and removes an unused RMM forward declaration. ChangesJava build pipeline
Timeout-aware Polars testing
Masked dtype behavior
Types header cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
b4809c5 to
cfa473a
Compare
github-actions
Bot
added
Python
Yes we have access to the macros. So the forward dec will look like: |
|
pre-commit.ci run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ci/timeout_with_stack.py (1)
210-238: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
psutil.wait_procsinstead of sequential per-child waits.
terminate_process_treewaits on each child one at a time (child.wait(timeout=3)in a loop), so worst-case wait time scales with the number of children (up to3s * N).psutil.wait_procs(children, timeout=3)waits on the whole list concurrently and returns(gone, alive), letting you kill only the processes that are actually still alive.♻️ Proposed refactor
try: parent = psutil.Process(pid) children = parent.children(recursive=True) # Terminate children first for child in children: with suppress(psutil.NoSuchProcess): child.terminate() - # Create a copy of children list - terminated_children = list(children) - - # Wait for all children to terminate - for child in terminated_children: - with suppress(psutil.TimeoutExpired): - child.wait(timeout=3) - - # Kill any remaining children - for child in terminated_children: - with suppress(psutil.NoSuchProcess): - child.kill() + # Wait for all children concurrently, then kill any stragglers + _, alive = psutil.wait_procs(children, timeout=3) + for child in alive: + with suppress(psutil.NoSuchProcess): + child.kill()🤖 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 `@ci/timeout_with_stack.py` around lines 210 - 238, Update terminate_process_tree to replace the sequential terminated_children child.wait loop with psutil.wait_procs(children, timeout=3), capturing its gone and alive results; retain the existing termination flow, but kill only the processes returned in alive while preserving suppression of processes that no longer exist.dependencies.yaml (1)
648-658: 📐 Maintainability & Code Quality | 🔵 TrivialConsider sharing the boost/maven/openjdk trio with
test_javavia anchors.
boost,maven, andopenjdk=8.*are duplicated verbatim in the existingtest_javagroup (Line 1040-1042) and this newbuild_javagroup. This file already uses YAML anchors (e.g.&cmake_ver,&pandas) for shared packages elsewhere — doing the same here would prevent version drift between the two toolchains.🤖 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 `@dependencies.yaml` around lines 648 - 658, Deduplicate the shared boost, maven, and openjdk=8.* entries between build_java and test_java using YAML anchors and aliases, following the existing anchor conventions in dependencies.yaml. Keep cuda-profiler-api and make specific to build_java, while preserving the current package versions and group contents.
🤖 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 `@ci/run_cudf_polars_pytests.sh`:
- Line 1: Restore per-test timeout protection alongside the whole-run timeout:
add a compatible pytest-timeout setting for cudf_polars via the pyproject
configuration and retain the 240-second timeout for the vendored polars
invocation in run_cudf_polars_polars_tests.sh. Keep the 3600-second
timeout_with_stack.py wrapper, ensuring individual hangs are terminated well
before the overall timeout.
In `@python/cudf/cudf/core/column/numerical.py`:
- Around line 914-930: Update the equivalent-dtype cast path in
ColumnBase.astype, including the nullable-extension branch around
NumericalColumn and ColumnBase.create, so copy=True produces independent storage
rather than a wrapper over self.plc_column. Apply copy handling after all cast
paths or deep-copy the newly created result, while preserving the existing
NaN-to-null conversion behavior.
In `@python/cudf/cudf/tests/series/methods/test_astype.py`:
- Around line 1631-1652: Expand the parametrization for
test_astype_masked_equivalent_dtype_no_source_mutation to include empty,
all-null/all-NaN, single-element, and mixed numeric inputs, covering both
supported source dtypes where applicable. Preserve assertions for source dtype
immutability, equivalent masked result dtype, and pandas-equivalent values,
including NaN normalization.
---
Nitpick comments:
In `@ci/timeout_with_stack.py`:
- Around line 210-238: Update terminate_process_tree to replace the sequential
terminated_children child.wait loop with psutil.wait_procs(children, timeout=3),
capturing its gone and alive results; retain the existing termination flow, but
kill only the processes returned in alive while preserving suppression of
processes that no longer exist.
In `@dependencies.yaml`:
- Around line 648-658: Deduplicate the shared boost, maven, and openjdk=8.*
entries between build_java and test_java using YAML anchors and aliases,
following the existing anchor conventions in dependencies.yaml. Keep
cuda-profiler-api and make specific to build_java, while preserving the current
package versions and group contents.
🪄 Autofix (Beta)
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: 99dc10eb-f4a3-4747-9185-35369d3fe2be
📒 Files selected for processing (28)
.github/workflows/build.yamlci/run_cudf_polars_polars_tests.shci/run_cudf_polars_pytests.shci/test_wheel_cudf_polars.shci/timeout_with_stack.pyconda/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.yamlcpp/include/cudf/types.hppdependencies.yamljava/ci/README.mdjava/ci/argparse.shjava/ci/assemble_maven_repo.shjava/ci/build_cudf_java_jar.shjava/ci/build_cudf_java_jar_in_container.shjava/ci/build_static_libcudf.shjava/ci/build_static_libcudf_in_container.shjava/ci/test_java_build_local.shjava/pom.xmlpython/cudf/cudf/core/column/numerical.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/series/methods/test_astype.pypython/cudf_polars/pyproject.tomlpython/cudf_polars/tests/conftest.pypython/cudf_polars/tests/expressions/test_rolling.pypython/cudf_polars/tests/streaming/test_scan.pypython/cudf_polars/tests/streaming/test_sort.py
💤 Files with no reviewable changes (6)
- python/cudf_polars/tests/conftest.py
- python/cudf_polars/tests/expressions/test_rolling.py
- python/cudf_polars/tests/streaming/test_sort.py
- cpp/include/cudf/types.hpp
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
- python/cudf_polars/tests/streaming/test_scan.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ci/timeout_with_stack.py (1)
210-238: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
psutil.wait_procsinstead of sequential per-child waits.
terminate_process_treewaits on each child one at a time (child.wait(timeout=3)in a loop), so worst-case wait time scales with the number of children (up to3s * N).psutil.wait_procs(children, timeout=3)waits on the whole list concurrently and returns(gone, alive), letting you kill only the processes that are actually still alive.♻️ Proposed refactor
try: parent = psutil.Process(pid) children = parent.children(recursive=True) # Terminate children first for child in children: with suppress(psutil.NoSuchProcess): child.terminate() - # Create a copy of children list - terminated_children = list(children) - - # Wait for all children to terminate - for child in terminated_children: - with suppress(psutil.TimeoutExpired): - child.wait(timeout=3) - - # Kill any remaining children - for child in terminated_children: - with suppress(psutil.NoSuchProcess): - child.kill() + # Wait for all children concurrently, then kill any stragglers + _, alive = psutil.wait_procs(children, timeout=3) + for child in alive: + with suppress(psutil.NoSuchProcess): + child.kill()🤖 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 `@ci/timeout_with_stack.py` around lines 210 - 238, Update terminate_process_tree to replace the sequential terminated_children child.wait loop with psutil.wait_procs(children, timeout=3), capturing its gone and alive results; retain the existing termination flow, but kill only the processes returned in alive while preserving suppression of processes that no longer exist.dependencies.yaml (1)
648-658: 📐 Maintainability & Code Quality | 🔵 TrivialConsider sharing the boost/maven/openjdk trio with
test_javavia anchors.
boost,maven, andopenjdk=8.*are duplicated verbatim in the existingtest_javagroup (Line 1040-1042) and this newbuild_javagroup. This file already uses YAML anchors (e.g.&cmake_ver,&pandas) for shared packages elsewhere — doing the same here would prevent version drift between the two toolchains.🤖 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 `@dependencies.yaml` around lines 648 - 658, Deduplicate the shared boost, maven, and openjdk=8.* entries between build_java and test_java using YAML anchors and aliases, following the existing anchor conventions in dependencies.yaml. Keep cuda-profiler-api and make specific to build_java, while preserving the current package versions and group contents.
🤖 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 `@ci/run_cudf_polars_pytests.sh`:
- Line 1: Restore per-test timeout protection alongside the whole-run timeout:
add a compatible pytest-timeout setting for cudf_polars via the pyproject
configuration and retain the 240-second timeout for the vendored polars
invocation in run_cudf_polars_polars_tests.sh. Keep the 3600-second
timeout_with_stack.py wrapper, ensuring individual hangs are terminated well
before the overall timeout.
In `@python/cudf/cudf/core/column/numerical.py`:
- Around line 914-930: Update the equivalent-dtype cast path in
ColumnBase.astype, including the nullable-extension branch around
NumericalColumn and ColumnBase.create, so copy=True produces independent storage
rather than a wrapper over self.plc_column. Apply copy handling after all cast
paths or deep-copy the newly created result, while preserving the existing
NaN-to-null conversion behavior.
In `@python/cudf/cudf/tests/series/methods/test_astype.py`:
- Around line 1631-1652: Expand the parametrization for
test_astype_masked_equivalent_dtype_no_source_mutation to include empty,
all-null/all-NaN, single-element, and mixed numeric inputs, covering both
supported source dtypes where applicable. Preserve assertions for source dtype
immutability, equivalent masked result dtype, and pandas-equivalent values,
including NaN normalization.
---
Nitpick comments:
In `@ci/timeout_with_stack.py`:
- Around line 210-238: Update terminate_process_tree to replace the sequential
terminated_children child.wait loop with psutil.wait_procs(children, timeout=3),
capturing its gone and alive results; retain the existing termination flow, but
kill only the processes returned in alive while preserving suppression of
processes that no longer exist.
In `@dependencies.yaml`:
- Around line 648-658: Deduplicate the shared boost, maven, and openjdk=8.*
entries between build_java and test_java using YAML anchors and aliases,
following the existing anchor conventions in dependencies.yaml. Keep
cuda-profiler-api and make specific to build_java, while preserving the current
package versions and group contents.
🪄 Autofix (Beta)
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: 99dc10eb-f4a3-4747-9185-35369d3fe2be
📒 Files selected for processing (28)
.github/workflows/build.yamlci/run_cudf_polars_polars_tests.shci/run_cudf_polars_pytests.shci/test_wheel_cudf_polars.shci/timeout_with_stack.pyconda/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.yamlcpp/include/cudf/types.hppdependencies.yamljava/ci/README.mdjava/ci/argparse.shjava/ci/assemble_maven_repo.shjava/ci/build_cudf_java_jar.shjava/ci/build_cudf_java_jar_in_container.shjava/ci/build_static_libcudf.shjava/ci/build_static_libcudf_in_container.shjava/ci/test_java_build_local.shjava/pom.xmlpython/cudf/cudf/core/column/numerical.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/series/methods/test_astype.pypython/cudf_polars/pyproject.tomlpython/cudf_polars/tests/conftest.pypython/cudf_polars/tests/expressions/test_rolling.pypython/cudf_polars/tests/streaming/test_scan.pypython/cudf_polars/tests/streaming/test_sort.py
💤 Files with no reviewable changes (6)
- python/cudf_polars/tests/conftest.py
- python/cudf_polars/tests/expressions/test_rolling.py
- python/cudf_polars/tests/streaming/test_sort.py
- cpp/include/cudf/types.hpp
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
- python/cudf_polars/tests/streaming/test_scan.py
🛑 Comments failed to post (3)
ci/run_cudf_polars_pytests.sh (1)
1-1: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Per-test timeout granularity is replaced by a single whole-run timeout — confirm this trade-off is intended. All three sites share one root cause:
pytest-timeout(45s per test incudf_polars, 240s per test in the vendored polars suite) is dropped in favor of wrapping the entire pytest invocation intimeout_with_stack.pywith a flat 3600s budget. A hang or pathological slowdown in any single test (especially under-n 4/--dist=worksteal, where it may not even be the first test to run) will now take up to an hour to be caught instead of tens of seconds, and-xonly helps once a test actually fails/errors, not while it's hanging.
ci/run_cudf_polars_pytests.sh#L9-14: confirm the loss of per-test timeout is acceptable forcudf_polarstests, or consider keeping apytest-timeoutvalue (even a generous one) alongside the wrapper so individual hangs are still caught well before the 3600s ceiling.ci/run_cudf_polars_polars_tests.sh#L65-99: same concern for the vendored polars suite, which previously enforced a 240s per-test ceiling via--timeout=240.python/cudf_polars/pyproject.toml#L91-94: this is where the per-testtimeout = 45ini option is deleted; if per-test timeouts are kept, this is the config to restore (at a value compatible with running under the wrapper).🤖 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 `@ci/run_cudf_polars_pytests.sh` at line 1, Restore per-test timeout protection alongside the whole-run timeout: add a compatible pytest-timeout setting for cudf_polars via the pyproject configuration and retain the 240-second timeout for the vendored polars invocation in run_cudf_polars_polars_tests.sh. Keep the 3600-second timeout_with_stack.py wrapper, ensuring individual hangs are terminated well before the overall timeout.python/cudf/cudf/core/column/numerical.py (1)
914-930: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the relevant implementations and surrounding context. git ls-files 'python/cudf/cudf/core/column/*.py' 'python/cudf/cudf/core/column/*.pyx' | sed -n '1,200p' echo '--- numerical.py outline ---' ast-grep outline python/cudf/cudf/core/column/numerical.py --view expanded || true echo '--- ColumnBase.astype references ---' rg -n "def astype|astype\(" python/cudf/cudf/core/column -g '*.py' -g '*.pyx' echo '--- ColumnBase.create references ---' rg -n "def create|`@classmethod`\s+def create|ColumnBase\.create" python/cudf/cudf/core/column -g '*.py' -g '*.pyx' echo '--- relevant slice of numerical.py ---' sed -n '880,960p' python/cudf/cudf/core/column/numerical.py echo '--- relevant slice of columnbase.py ---' sed -n '1,260p' python/cudf/cudf/core/column/column.py echo '--- deeper slice around astype in columnbase.py ---' rg -n -A40 -B20 "def astype" python/cudf/cudf/core/column/column.pyRepository: rapidsai/cudf
Length of output: 46545
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- rest of ColumnBase.astype ---' sed -n '2345,2395p' python/cudf/cudf/core/column/column.py echo '--- ColumnBase.create implementation ---' sed -n '930,1035p' python/cudf/cudf/core/column/column.py echo '--- copy helpers / access semantics ---' rg -n -A30 -B20 "copy=True|copy: bool|deep.*copy|plc_column.copy|self is result|result is self" python/cudf/cudf/core/column/column.py python/cudf/cudf/core/column/numerical.pyRepository: rapidsai/cudf
Length of output: 15229
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- astype docstrings / copy semantics ---' sed -n '2298,2360p' python/cudf/cudf/core/column/column.py echo '--- search for copy=True handling in astype-like APIs ---' rg -n -A8 -B8 "astype\(.*copy: bool|copy=True|copy and result is self|return result.copy\(deep=copy\)" python/cudf/cudf/core/column -g '*.py' echo '--- search for any tests or comments about astype(copy=...) ---' rg -n "astype\(.*copy=|copy=True.*astype|equivalent.*dtype|shared storage|copy-on-write" python/cudf -g '*.py' -g '*.pyx'Repository: rapidsai/cudf
Length of output: 11986
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- relevant Series/DataFrame astype docs / implementation ---' rg -n -A20 -B10 "def astype\(.*copy=|copy-on-write|astype\(..., copy=False\)|astype\(dtype=.*copy=" python/cudf/cudf/core/indexed_frame.py python/cudf/cudf/core/frame.py python/cudf/cudf/core/index.py -g '*.py' echo '--- the astype test that mentions equivalent masked dtype ---' sed -n '1628,1675p' python/cudf/cudf/tests/series/methods/test_astype.py echo '--- copy-on-write buffer semantics ---' rg -n -A20 -B10 "copy-on-write|deep copy|shared data|write.*copy" python/cudf/cudf/core/buffer python/cudf/cudf/core/column -g '*.py'Repository: rapidsai/cudf
Length of output: 34192
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- public astype docs ---' rg -n -A18 -B8 "def astype\(|copy : bool|copy-on-write|Returns a new object" python/cudf/cudf/core/indexed_frame.py python/cudf/cudf/core/frame.py python/cudf/cudf/core/index.py -g '*.py' echo '--- astype call sites in indexed_frame.py ---' rg -n -A12 -B8 "\.astype\(.*copy=" python/cudf/cudf/core/indexed_frame.py -g '*.py' echo '--- any dtype-equivalent cast tests with copy=True ---' rg -n "equivalent dtype|masked dtype|copy=True" python/cudf/cudf/tests/series/methods/test_astype.py python/cudf/cudf/tests/dataframe/methods/test_astype.py python/cudf/cudf/tests/indexes/index/methods/test_astype.py -g '*.py'Repository: rapidsai/cudf
Length of output: 44744
Honor
copy=Truefor equivalent dtype casts. Inpython/cudf/cudf/core/column/numerical.py, this short-circuit builds a new wrapper over the same buffers, butColumnBase.astypeonly deep-copies whenresult is self. That leavesastype(copy=True)on this path sharing storage with the source instead of returning an independent copy. Move thecopyhandling after all cast paths or deep-copy this branch too.🤖 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 `@python/cudf/cudf/core/column/numerical.py` around lines 914 - 930, Update the equivalent-dtype cast path in ColumnBase.astype, including the nullable-extension branch around NumericalColumn and ColumnBase.create, so copy=True produces independent storage rather than a wrapper over self.plc_column. Apply copy handling after all cast paths or deep-copy the newly created result, while preserving the existing NaN-to-null conversion behavior.python/cudf/cudf/tests/series/methods/test_astype.py (1)
1631-1652: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expand edge-case coverage for the new cast path.
The parametrization currently covers only non-empty, multi-element inputs. Add empty, all-null/all-NaN, single-element, and mixed numeric cases so NaN normalization and equivalent nullable dtype handling are exercised broadly.
As per coding guidelines,
python/**/test_*.pytests must provide comprehensive coverage for empty, all-null, single-element, and mixed types.🤖 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 `@python/cudf/cudf/tests/series/methods/test_astype.py` around lines 1631 - 1652, Expand the parametrization for test_astype_masked_equivalent_dtype_no_source_mutation to include empty, all-null/all-NaN, single-element, and mixed numeric inputs, covering both supported source dtypes where applicable. Preserve assertions for source dtype immutability, equivalent masked result dtype, and pandas-equivalent values, including NaN normalization.Source: Coding guidelines
mhaseeb123
added
5 - Ready to Merge
|
/merge |
2e3d754
into
NVIDIA:release/26.08
10 participants
Description
Fixes build error introduced by an RMM change. The forward declaration in types.hpp is not actually needed and removing it fixes the build errors.
Checklist