Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate cudf::grouped_time_range_rolling_window #17589

Open
wants to merge 2 commits into
base: branch-25.02
Choose a base branch
from

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Dec 13, 2024

Description

Since the range-based rolling window APIs were extended to support any orderable column (rather than just time-based windows), this function has been a thin forwarding wrapper to
cudf::grouped_range_rolling_window. It could have been deprecated at the time, but was not. It seems there are no usages outside of the libcudf test suite.

Partially addresses #13050.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- requested a review from a team as a code owner December 13, 2024 11:08
@wence- wence- requested review from mythrocks and lamarrr December 13, 2024 11:08
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Dec 13, 2024
@wence- wence- added improvement Improvement / enhancement to an existing function breaking Breaking change labels Dec 13, 2024
@mythrocks
Copy link
Contributor

Thank you, @wence-. I've been dragging my feet on this one.

I have checked. It appears that I've already removed references to the time-range version of these calls from the Java API, and switched it to numeric ranges.

spark-rapids and spark-rapids-jni should not be affected by this: All references should be through JNI, which has already moved.

This is looking good to go.

@mythrocks
Copy link
Contributor

mythrocks commented Dec 13, 2024

The failures seem to be transient and unrelated to the change.

Cloning into 'rapids_logger-src'...
fatal: unable to read tree (14bb233d2420f7187a690f0bb528ec0420c70d48)
CMake Error at rapids_logger-subbuild/rapids_logger-populate-prefix/tmp/rapids_logger-populate-gitclone.cmake:61 (message):
  Failed to checkout tag: '14bb233d2420f7187a690f0bb528ec0420c70d48'


FAILED: rapids_logger-populate-prefix/src/rapids_logger-populate-stamp/rapids_logger-populate-download /__w/cudf/cudf/cpp/build/_deps/rapids_logger-subbuild/rapids_logger-populate-prefix/src/rapids_logger-populate-stamp/rapids_logger-populate-download 
cd /__w/cudf/cudf/cpp/build/_deps && /opt/conda/envs/clang_tidy/bin/cmake -DCMAKE_MESSAGE_LOG_LEVEL=VERBOSE -P /__w/cudf/cudf/cpp/build/_deps/rapids_logger-subbuild/rapids_logger-populate-prefix/tmp/rapids_logger-populate-gitclone.cmake && /opt/conda/envs/clang_tidy/bin/cmake -E touch /__w/cudf/cudf/cpp/build/_deps/rapids_logger-subbuild/rapids_logger-populate-prefix/src/rapids_logger-populate-stamp/rapids_logger-populate-download
ninja: build stopped: subcommand failed.

I've restarted the failing CI jobs.

@wence- wence- force-pushed the wence/fea/rolling-deprecate branch 2 times, most recently from 6e6b5b4 to 3bb751d Compare December 17, 2024 12:01
Since the range-based rolling window APIs were extended to support any
orderable column (rather than just time-based windows), this function
has been a thin forwarding wrapper to
cudf::grouped_range_rolling_window. It could have been deprecated at
the time, but was not. It seems there are no usages outside of the
libcudf test suite.

Partially addresses rapidsai#13050.
@wence- wence- force-pushed the wence/fea/rolling-deprecate branch from 3bb751d to f5366e5 Compare December 17, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants