Fix Parquet stats pinned host copy lifetime - #23219
Conversation
|
Thanks for the PR. Let us know when it's ready for review. |
|
Thanks! This is ready for initial review. pre-commit is passing. I wasn’t able to complete a local GPU build/test because the targeted cuDF source build did not finish in the available runtime, so I’m waiting on RAPIDS CI for full validation. |
|
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:
📝 WalkthroughWalkthroughAdds Java CUDA/architecture artifact packaging and Maven repository assembly, introduces process-tree timeout diagnostics for Polars tests, updates pandas proxy mirroring and dtype casting, and refactors Parquet statistics device-buffer ownership. ChangesJava artifact build pipeline
Polars timeout diagnostics
Pandas proxy and dtype behavior
Parquet statistics conversion
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/io/parquet_reader_test.cpp (1)
1538-1563: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing regression coverage for the string statistics path.
This test only exercises
host_column<int32_t>::to_device()(fixed-width branch). Per the PR objectives, the same lifetime bug also affected the string statistics path (host_column<string_view>::to_device(), and itsnull_mask_buffer/make_strings_childrenhandling instats_filter_helpers.hpp), which remains unverified by this regression suite.The existing
poisoning_pinned_memory_resource/scoped_pinned_memory_settings/stream_blockerhelpers are directly reusable for a string-typed variant of this test.Would you like me to draft a
StatsFilterPinnedHostColumnCopyLifetimeStringtest analogous to this one, usinghost_column<cudf::string_view>?🤖 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/tests/io/parquet_reader_test.cpp` around lines 1538 - 1563, Add a string-specific regression test alongside StatsFilterPinnedHostColumnCopyLifetime, named StatsFilterPinnedHostColumnCopyLifetimeString, reusing the existing poisoning_pinned_memory_resource, scoped_pinned_memory_settings, and stream_blocker setup. Exercise stats_caster_base_test::host_column<cudf::string_view>::to_device(), including representative string values, destroy the host column before stream synchronization, and verify the resulting strings against an expected column to cover null_mask_buffer and make_strings_children lifetime handling.
🤖 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.
Nitpick comments:
In `@cpp/tests/io/parquet_reader_test.cpp`:
- Around line 1538-1563: Add a string-specific regression test alongside
StatsFilterPinnedHostColumnCopyLifetime, named
StatsFilterPinnedHostColumnCopyLifetimeString, reusing the existing
poisoning_pinned_memory_resource, scoped_pinned_memory_settings, and
stream_blocker setup. Exercise
stats_caster_base_test::host_column<cudf::string_view>::to_device(), including
representative string values, destroy the host column before stream
synchronization, and verify the resulting strings against an expected column to
cover null_mask_buffer and make_strings_children lifetime handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0b1fd67-228b-4176-b7e5-cfc504d8639b
📒 Files selected for processing (2)
cpp/src/io/parquet/stats_filter_helpers.hppcpp/tests/io/parquet_reader_test.cpp
|
/ok to test 00d0d7f |
|
@aryansri05 C++ tests are failing in CI |
00d0d7f to
428573b
Compare
|
Thanks for the heads up. I pushed a fix for the C++ test failure: the regression test was comparing against a non-nullable expected column, while the stats conversion path returns a nullable/all-valid column because it carries the null mask. I also rebased the branch onto current main. |
mhaseeb123
left a comment
mhaseeb123
left a comment
There was a problem hiding this comment.
The changes themselves look ok to me. I have, however, concerns regarding the new poisoning_pinned_mr and related changes in the tests since its scope/use case seems wider than just parquet reader.
| auto [d_chars, d_offsets, _] = make_strings_children(val, chars, stream, mr); | ||
| auto null_mask_buffer = rmm::device_buffer{ | ||
| null_mask.data(), cudf::bitmask_allocation_size_bytes(val.size()), stream, mr}; | ||
| // This host_column owns the source buffers, so they must outlive the async copy above. |
There was a problem hiding this comment.
| // This host_column owns the source buffers, so they must outlive the async copy above. |
| auto data = cudf::detail::make_device_uvector_async(val, stream, mr); | ||
| auto null_mask_buffer = rmm::device_buffer{ | ||
| null_mask.data(), cudf::bitmask_allocation_size_bytes(val.size()), stream, mr}; | ||
| // This host_column owns the source buffers, so they must outlive the async copies above. |
There was a problem hiding this comment.
| // This host_column owns the source buffers, so they must outlive the async copies above. |
428573b to
f5a1cf4
Compare
|
Removed the redundant source comments and rebased onto current main. On the pinned-memory poisoning helper: I kept it scoped to this regression test because it currently only has this use, but I’m happy to move it into a shared test utility if you think it would be useful elsewhere. |
|
/ok to test f5a1cf4 |
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
I would slightly prefer this PR without the tests, unless we plan to reuse the new utilities. Not a blocker.
mhaseeb123
left a comment
mhaseeb123
left a comment
There was a problem hiding this comment.
Same thoughts as @vuule
|
@aryansri05 would you mind removing the tests since that is @vuule and @mhaseeb123's preference? Thank you! |
f5a1cf4 to
8e86245
Compare
|
Done, removed the test additions as requested and rebased onto current main. The PR now only contains the Parquet stats lifetime fix in stats_filter_helpers.hpp. |
|
/ok to test 8e86245 |
mhaseeb123
added
5 - Ready to Merge
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
java/ci/argparse.sh (1)
9-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
require_valuedoesn't catch a missing value followed by another flag.If a flag's value is omitted and the next token happens to be a recognized flag (e.g.
--output-dir --cuda-version 12.9),valueis non-empty (it's the next flag's text) so this check passes silently, and the flag name gets assigned as if it were the value. This will manifest as a confusing downstream failure (e.g. wrong output dir) rather than a clear "requires a value" error.🛠️ Proposed tightening
require_value() { local flag=$1 local value=$2 - if [[ -z ${value} ]]; then + if [[ -z ${value} || ${value} == -* ]]; then echo "Error: ${flag} requires a value" >&2 exit 1 fi }🤖 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 `@java/ci/argparse.sh` around lines 9 - 19, Update require_value to reject values that are empty or match a recognized command-line flag, not just empty strings. Ensure calls such as --output-dir followed by --cuda-version fail with the existing “requires a value” error, while preserving valid non-flag values.dependencies.yaml (1)
648-658: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication with existing
test_javapackage group.
boost,maven, andopenjdk=8.*are already declared in the pre-existingdependencies.test_javagroup (used by thetest_javafile group). Consider whetherbuild_javashould reference/extend that group instead of re-declaring the same pins, to avoid version drift between the two toolchains over time.🤖 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, Update the build_java dependency definition to reuse or extend the existing test_java package group for boost, maven, and openjdk=8.* instead of redeclaring those pins. Keep build_java’s additional packages, including cuda-profiler-api and make, unchanged and preserve the shared versions between both toolchains.java/ci/build_static_libcudf.sh (1)
86-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnknown-argument errors print to stdout instead of stderr, unlike every other error path in these scripts. Each script's
case ... *)fallback uses a bareecho "Error: Unknown argument $1"while all sibling error messages in the same files use>&2.
java/ci/build_static_libcudf.sh#L86-L90: redirect theecho "Error: Unknown argument $1"to>&2.java/ci/build_cudf_java_jar.sh#L109-L113: redirect theecho "Error: Unknown argument $1"to>&2.java/ci/test_java_build_local.sh#L113-L117: redirect theecho "Error: Unknown argument $1"to>&2.java/ci/assemble_maven_repo.sh#L73-L77: redirect theecho "Error: Unknown argument $1"to>&2.🤖 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 `@java/ci/build_static_libcudf.sh` around lines 86 - 90, Redirect the unknown-argument error echo to stderr in the case fallback blocks of java/ci/build_static_libcudf.sh (lines 86-90), java/ci/build_cudf_java_jar.sh (lines 109-113), java/ci/test_java_build_local.sh (lines 113-117), and java/ci/assemble_maven_repo.sh (lines 73-77), matching the existing sibling error paths.java/ci/README.md (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code blocks.
markdownlint (MD040) flags these two directory-tree blocks as missing a language identifier.
📝 Proposed fix
-``` +```text /tmp/jars/cuda12/ cudf-26.08.0-SNAPSHOT-cuda12.jar cudf-26.08.0-SNAPSHOT.pom(apply the same for the second block at lines 75-80) </details> Also applies to: 75-80 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@java/ci/README.mdaround lines 47 - 51, Add the text language identifier to
both fenced directory-tree code blocks in the README, including the blocks
containing the CUDA 12 and second JAR listings, while preserving their contents
unchanged.</details> <!-- cr-comment:v1:5747114536d9cdf48465da3b --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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_polars_tests.sh:
- Around line 67-71: Restore per-test timeout protection while retaining the
3600-second diagnostic wrapper: in ci/run_cudf_polars_polars_tests.sh lines
67-71 and 88-92, re-enable the 240-second pytest-timeout configuration for both
invocations; in ci/run_cudf_polars_pytests.sh lines 13-14, preserve the
project-level timeout alongside the wrapper; restore pytest-timeout and timeout
= 45 in python/cudf_polars/pyproject.toml line 48; and restore pytest-timeout in
conda/environments/all_cuda-129_arch-aarch64.yaml line 80,
all_cuda-129_arch-x86_64.yaml line 80, all_cuda-133_arch-aarch64.yaml line 80,
and all_cuda-133_arch-x86_64.yaml line 80.In
@java/ci/build_static_libcudf_in_container.sh:
- Line 19: Ensure the script’s final ownership correction runs on both
successful and failed builds by adding an EXIT trap around the existing chown
logic, mirroring build_cudf_java_jar_in_container.sh. Keep the trap registered
before commands that can fail under set -e, and preserve the existing host-user
ownership behavior for /output.
Nitpick comments:
In@dependencies.yaml:
- Around line 648-658: Update the build_java dependency definition to reuse or
extend the existing test_java package group for boost, maven, and openjdk=8.*
instead of redeclaring those pins. Keep build_java’s additional packages,
including cuda-profiler-api and make, unchanged and preserve the shared versions
between both toolchains.In
@java/ci/argparse.sh:
- Around line 9-19: Update require_value to reject values that are empty or
match a recognized command-line flag, not just empty strings. Ensure calls such
as --output-dir followed by --cuda-version fail with the existing “requires a
value” error, while preserving valid non-flag values.In
@java/ci/build_static_libcudf.sh:
- Around line 86-90: Redirect the unknown-argument error echo to stderr in the
case fallback blocks of java/ci/build_static_libcudf.sh (lines 86-90),
java/ci/build_cudf_java_jar.sh (lines 109-113), java/ci/test_java_build_local.sh
(lines 113-117), and java/ci/assemble_maven_repo.sh (lines 73-77), matching the
existing sibling error paths.In
@java/ci/README.md:
- Around line 47-51: Add the text language identifier to both fenced
directory-tree code blocks in the README, including the blocks containing the
CUDA 12 and second JAR listings, while preserving their contents unchanged.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `1c2c7adf-7642-4d22-9c83-0f74618cbebd` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8e862457e042e4d449b20c331ec20c2989dd71ff and c435b4cceba05a3943e80c8e7e3d503a8c5e7c4d. </details> <details> <summary>📒 Files selected for processing (32)</summary> * `.github/workflows/build.yaml` * `ci/run_cudf_polars_polars_tests.sh` * `ci/run_cudf_polars_pytests.sh` * `ci/test_wheel_cudf_polars.sh` * `ci/timeout_with_stack.py` * `conda/environments/all_cuda-129_arch-aarch64.yaml` * `conda/environments/all_cuda-129_arch-x86_64.yaml` * `conda/environments/all_cuda-133_arch-aarch64.yaml` * `conda/environments/all_cuda-133_arch-x86_64.yaml` * `cpp/src/io/parquet/stats_filter_helpers.hpp` * `dependencies.yaml` * `java/ci/README.md` * `java/ci/argparse.sh` * `java/ci/assemble_maven_repo.sh` * `java/ci/build_cudf_java_jar.sh` * `java/ci/build_cudf_java_jar_in_container.sh` * `java/ci/build_static_libcudf.sh` * `java/ci/build_static_libcudf_in_container.sh` * `java/ci/test_java_build_local.sh` * `java/pom.xml` * `python/cudf/cudf/core/column/numerical.py` * `python/cudf/cudf/pandas/_wrappers/pandas.py` * `python/cudf/cudf/pandas/fast_slow_proxy.py` * `python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py` * `python/cudf/cudf/tests/series/methods/test_astype.py` * `python/cudf/cudf_pandas_tests/test_cudf_pandas.py` * `python/cudf/cudf_pandas_tests/test_fast_slow_proxy.py` * `python/cudf_polars/pyproject.toml` * `python/cudf_polars/tests/conftest.py` * `python/cudf_polars/tests/expressions/test_rolling.py` * `python/cudf_polars/tests/streaming/test_scan.py` * `python/cudf_polars/tests/streaming/test_sort.py` </details> <details> <summary>💤 Files with no reviewable changes (5)</summary> * python/cudf_polars/tests/streaming/test_scan.py * python/cudf_polars/tests/streaming/test_sort.py * python/cudf_polars/tests/conftest.py * python/cudf_polars/tests/expressions/test_rolling.py * python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * cpp/src/io/parquet/stats_filter_helpers.hpp </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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: 2
🧹 Nitpick comments (4)
java/ci/argparse.sh (1)
9-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
require_valuedoesn't catch a missing value followed by another flag.If a flag's value is omitted and the next token happens to be a recognized flag (e.g.
--output-dir --cuda-version 12.9),valueis non-empty (it's the next flag's text) so this check passes silently, and the flag name gets assigned as if it were the value. This will manifest as a confusing downstream failure (e.g. wrong output dir) rather than a clear "requires a value" error.🛠️ Proposed tightening
require_value() { local flag=$1 local value=$2 - if [[ -z ${value} ]]; then + if [[ -z ${value} || ${value} == -* ]]; then echo "Error: ${flag} requires a value" >&2 exit 1 fi }🤖 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 `@java/ci/argparse.sh` around lines 9 - 19, Update require_value to reject values that are empty or match a recognized command-line flag, not just empty strings. Ensure calls such as --output-dir followed by --cuda-version fail with the existing “requires a value” error, while preserving valid non-flag values.dependencies.yaml (1)
648-658: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication with existing
test_javapackage group.
boost,maven, andopenjdk=8.*are already declared in the pre-existingdependencies.test_javagroup (used by thetest_javafile group). Consider whetherbuild_javashould reference/extend that group instead of re-declaring the same pins, to avoid version drift between the two toolchains over time.🤖 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, Update the build_java dependency definition to reuse or extend the existing test_java package group for boost, maven, and openjdk=8.* instead of redeclaring those pins. Keep build_java’s additional packages, including cuda-profiler-api and make, unchanged and preserve the shared versions between both toolchains.java/ci/build_static_libcudf.sh (1)
86-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnknown-argument errors print to stdout instead of stderr, unlike every other error path in these scripts. Each script's
case ... *)fallback uses a bareecho "Error: Unknown argument $1"while all sibling error messages in the same files use>&2.
java/ci/build_static_libcudf.sh#L86-L90: redirect theecho "Error: Unknown argument $1"to>&2.java/ci/build_cudf_java_jar.sh#L109-L113: redirect theecho "Error: Unknown argument $1"to>&2.java/ci/test_java_build_local.sh#L113-L117: redirect theecho "Error: Unknown argument $1"to>&2.java/ci/assemble_maven_repo.sh#L73-L77: redirect theecho "Error: Unknown argument $1"to>&2.🤖 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 `@java/ci/build_static_libcudf.sh` around lines 86 - 90, Redirect the unknown-argument error echo to stderr in the case fallback blocks of java/ci/build_static_libcudf.sh (lines 86-90), java/ci/build_cudf_java_jar.sh (lines 109-113), java/ci/test_java_build_local.sh (lines 113-117), and java/ci/assemble_maven_repo.sh (lines 73-77), matching the existing sibling error paths.java/ci/README.md (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code blocks.
markdownlint (MD040) flags these two directory-tree blocks as missing a language identifier.
📝 Proposed fix
-``` +```text /tmp/jars/cuda12/ cudf-26.08.0-SNAPSHOT-cuda12.jar cudf-26.08.0-SNAPSHOT.pom(apply the same for the second block at lines 75-80) </details> Also applies to: 75-80 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@java/ci/README.mdaround lines 47 - 51, Add the text language identifier to
both fenced directory-tree code blocks in the README, including the blocks
containing the CUDA 12 and second JAR listings, while preserving their contents
unchanged.</details> <!-- cr-comment:v1:5747114536d9cdf48465da3b --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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_polars_tests.sh:
- Around line 67-71: Restore per-test timeout protection while retaining the
3600-second diagnostic wrapper: in ci/run_cudf_polars_polars_tests.sh lines
67-71 and 88-92, re-enable the 240-second pytest-timeout configuration for both
invocations; in ci/run_cudf_polars_pytests.sh lines 13-14, preserve the
project-level timeout alongside the wrapper; restore pytest-timeout and timeout
= 45 in python/cudf_polars/pyproject.toml line 48; and restore pytest-timeout in
conda/environments/all_cuda-129_arch-aarch64.yaml line 80,
all_cuda-129_arch-x86_64.yaml line 80, all_cuda-133_arch-aarch64.yaml line 80,
and all_cuda-133_arch-x86_64.yaml line 80.In
@java/ci/build_static_libcudf_in_container.sh:
- Line 19: Ensure the script’s final ownership correction runs on both
successful and failed builds by adding an EXIT trap around the existing chown
logic, mirroring build_cudf_java_jar_in_container.sh. Keep the trap registered
before commands that can fail under set -e, and preserve the existing host-user
ownership behavior for /output.
Nitpick comments:
In@dependencies.yaml:
- Around line 648-658: Update the build_java dependency definition to reuse or
extend the existing test_java package group for boost, maven, and openjdk=8.*
instead of redeclaring those pins. Keep build_java’s additional packages,
including cuda-profiler-api and make, unchanged and preserve the shared versions
between both toolchains.In
@java/ci/argparse.sh:
- Around line 9-19: Update require_value to reject values that are empty or
match a recognized command-line flag, not just empty strings. Ensure calls such
as --output-dir followed by --cuda-version fail with the existing “requires a
value” error, while preserving valid non-flag values.In
@java/ci/build_static_libcudf.sh:
- Around line 86-90: Redirect the unknown-argument error echo to stderr in the
case fallback blocks of java/ci/build_static_libcudf.sh (lines 86-90),
java/ci/build_cudf_java_jar.sh (lines 109-113), java/ci/test_java_build_local.sh
(lines 113-117), and java/ci/assemble_maven_repo.sh (lines 73-77), matching the
existing sibling error paths.In
@java/ci/README.md:
- Around line 47-51: Add the text language identifier to both fenced
directory-tree code blocks in the README, including the blocks containing the
CUDA 12 and second JAR listings, while preserving their contents unchanged.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `1c2c7adf-7642-4d22-9c83-0f74618cbebd` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8e862457e042e4d449b20c331ec20c2989dd71ff and c435b4cceba05a3943e80c8e7e3d503a8c5e7c4d. </details> <details> <summary>📒 Files selected for processing (32)</summary> * `.github/workflows/build.yaml` * `ci/run_cudf_polars_polars_tests.sh` * `ci/run_cudf_polars_pytests.sh` * `ci/test_wheel_cudf_polars.sh` * `ci/timeout_with_stack.py` * `conda/environments/all_cuda-129_arch-aarch64.yaml` * `conda/environments/all_cuda-129_arch-x86_64.yaml` * `conda/environments/all_cuda-133_arch-aarch64.yaml` * `conda/environments/all_cuda-133_arch-x86_64.yaml` * `cpp/src/io/parquet/stats_filter_helpers.hpp` * `dependencies.yaml` * `java/ci/README.md` * `java/ci/argparse.sh` * `java/ci/assemble_maven_repo.sh` * `java/ci/build_cudf_java_jar.sh` * `java/ci/build_cudf_java_jar_in_container.sh` * `java/ci/build_static_libcudf.sh` * `java/ci/build_static_libcudf_in_container.sh` * `java/ci/test_java_build_local.sh` * `java/pom.xml` * `python/cudf/cudf/core/column/numerical.py` * `python/cudf/cudf/pandas/_wrappers/pandas.py` * `python/cudf/cudf/pandas/fast_slow_proxy.py` * `python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py` * `python/cudf/cudf/tests/series/methods/test_astype.py` * `python/cudf/cudf_pandas_tests/test_cudf_pandas.py` * `python/cudf/cudf_pandas_tests/test_fast_slow_proxy.py` * `python/cudf_polars/pyproject.toml` * `python/cudf_polars/tests/conftest.py` * `python/cudf_polars/tests/expressions/test_rolling.py` * `python/cudf_polars/tests/streaming/test_scan.py` * `python/cudf_polars/tests/streaming/test_sort.py` </details> <details> <summary>💤 Files with no reviewable changes (5)</summary> * python/cudf_polars/tests/streaming/test_scan.py * python/cudf_polars/tests/streaming/test_sort.py * python/cudf_polars/tests/conftest.py * python/cudf_polars/tests/expressions/test_rolling.py * python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * cpp/src/io/parquet/stats_filter_helpers.hpp </details> </details> <details> <summary>🛑 Comments failed to post (2)</summary><blockquote> <details> <summary>ci/run_cudf_polars_polars_tests.sh (1)</summary><blockquote> 67-71: _🩺 Stability & Availability_ | _🟠 Major_ | _⚡ Quick win_ **Retain a per-test timeout beneath the 3600-second diagnostic wrapper.** Removing `pytest-timeout` changes a hung test from failing after 45/240 seconds to blocking the entire suite for up to one hour, delaying failure diagnostics and consuming CI capacity. Keep the outer wrapper for process-tree capture, but restore the per-test timeout plugin and configuration. - `ci/run_cudf_polars_polars_tests.sh#L67-L71`: restore the 240-second per-test timeout for the in-memory invocation. - `ci/run_cudf_polars_polars_tests.sh#L88-L92`: restore the 240-second per-test timeout for the SPMD invocation. - `ci/run_cudf_polars_pytests.sh#L13-L14`: retain the wrapper while preserving the project-level per-test timeout. - `python/cudf_polars/pyproject.toml#L48-L48`: restore `pytest-timeout` and the `timeout = 45` pytest setting. - `conda/environments/all_cuda-129_arch-aarch64.yaml#L80-L80`: restore `pytest-timeout`. - `conda/environments/all_cuda-129_arch-x86_64.yaml#L80-L80`: restore `pytest-timeout`. - `conda/environments/all_cuda-133_arch-aarch64.yaml#L80-L80`: restore `pytest-timeout`. - `conda/environments/all_cuda-133_arch-x86_64.yaml#L80-L80`: restore `pytest-timeout`. <details> <summary>📍 Affects 7 files</summary> - `ci/run_cudf_polars_polars_tests.sh#L67-L71` (this comment) - `ci/run_cudf_polars_polars_tests.sh#L88-L92` - `ci/run_cudf_polars_pytests.sh#L13-L14` - `python/cudf_polars/pyproject.toml#L48-L48` - `conda/environments/all_cuda-129_arch-aarch64.yaml#L80-L80` - `conda/environments/all_cuda-129_arch-x86_64.yaml#L80-L80` - `conda/environments/all_cuda-133_arch-aarch64.yaml#L80-L80` - `conda/environments/all_cuda-133_arch-x86_64.yaml#L80-L80` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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_polars_tests.sharound lines 67 - 71, Restore per-test
timeout protection while retaining the 3600-second diagnostic wrapper: in
ci/run_cudf_polars_polars_tests.sh lines 67-71 and 88-92, re-enable the
240-second pytest-timeout configuration for both invocations; in
ci/run_cudf_polars_pytests.sh lines 13-14, preserve the project-level timeout
alongside the wrapper; restore pytest-timeout and timeout = 45 in
python/cudf_polars/pyproject.toml line 48; and restore pytest-timeout in
conda/environments/all_cuda-129_arch-aarch64.yaml line 80,
all_cuda-129_arch-x86_64.yaml line 80, all_cuda-133_arch-aarch64.yaml line 80,
and all_cuda-133_arch-x86_64.yaml line 80.</details> <!-- consolidated_sites_start --> <!-- <consolidated_sites> <site> <role>anchor</role> <file>ci/run_cudf_polars_polars_tests.sh</file> <line_range>67-71</line_range> </site> <site> <role>sibling</role> <file>ci/run_cudf_polars_polars_tests.sh</file> <line_range>88-92</line_range> </site> <site> <role>sibling</role> <file>ci/run_cudf_polars_pytests.sh</file> <line_range>13-14</line_range> </site> <site> <role>sibling</role> <file>python/cudf_polars/pyproject.toml</file> <line_range>48-48</line_range> </site> <site> <role>sibling</role> <file>conda/environments/all_cuda-129_arch-aarch64.yaml</file> <line_range>80-80</line_range> </site> <site> <role>sibling</role> <file>conda/environments/all_cuda-129_arch-x86_64.yaml</file> <line_range>80-80</line_range> </site> <site> <role>sibling</role> <file>conda/environments/all_cuda-133_arch-aarch64.yaml</file> <line_range>80-80</line_range> </site> <site> <role>sibling</role> <file>conda/environments/all_cuda-133_arch-x86_64.yaml</file> <line_range>80-80</line_range> </site> </consolidated_sites> --> <!-- consolidated_sites_end --> <!-- fingerprinting:phantom:poseidon:terra --> <!-- cr-indicator-types:potential_issue --> <!-- cr-comment:v1:0d6b64c1d00469a3d29c465e --> <!-- This is an auto-generated comment by CodeRabbit --> </blockquote></details> <details> <summary>java/ci/build_static_libcudf_in_container.sh (1)</summary><blockquote> 19-19: _🩺 Stability & Availability_ | _🟠 Major_ | _⚡ Quick win_ **Chown only runs on success, leaving root-owned files on build failure.** `chown` executes unconditionally at the end of the script; under `set -e`, any earlier failure (conda env creation, cmake configure/build/install) exits before reaching it. The container runs as root by default, so a failed build leaves `/output` (host `--output-dir`) root-owned — breaking the README's "Build outputs are host-user-owned so plain `rm -rf` works" guarantee, and potentially blocking a non-root user's cleanup/retry (e.g. via `test_java_build_local.sh`'s pre-run `rm -rf`). The sibling script `build_cudf_java_jar_in_container.sh` already solves this correctly with an `EXIT` trap. <details> <summary>🔒 Proposed fix mirroring build_cudf_java_jar_in_container.sh</summary> ```diff if [[ -z ${HOST_UID} || -z ${HOST_GID} ]]; then echo "Error: HOST_UID and HOST_GID must both be set" >&2 exit 1 fi +_chown_output_on_exit() { + chown -R "${HOST_UID}:${HOST_GID}" "${INSTALL_PREFIX}" 2>/dev/null || true +} +trap _chown_output_on_exit EXIT + if [[ -z ${PARALLEL_LEVEL} ]]; then PARALLEL_LEVEL=$(nproc) firapids-logger "Installing static libcudf to ${INSTALL_PREFIX}" cmake --install "${BUILD_DIR}" - -rapids-logger "Chowning ${INSTALL_PREFIX} to ${HOST_UID}:${HOST_GID}" -chown -R "${HOST_UID}:${HOST_GID}" "${INSTALL_PREFIX}"Also applies to: 94-95
🤖 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 `@java/ci/build_static_libcudf_in_container.sh` at line 19, Ensure the script’s final ownership correction runs on both successful and failed builds by adding an EXIT trap around the existing chown logic, mirroring build_cudf_java_jar_in_container.sh. Keep the trap registered before commands that can fail under set -e, and preserve the existing host-user ownership behavior for /output.
|
/ok to test 51727f7 |
|
/merge |
|
/ok to test 8e2857b |
2fb0ed7
into
NVIDIA:release/26.08
9 participants
Description
Closes #23178.
Fixes a lifetime hazard in Parquet statistics filtering where
stats_caster_base::host_column::to_devicecould return while asynchronous copies were still reading buffers owned by the temporary host column. This keeps the host-owned value and null-mask buffers alive until the stream has consumed them, covering both fixed-width and string statistics.Checklist