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.

Sign up for GitHub

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

Jump to bottom

Exposed stream-ordering to datetime API #16774

Merged
merged 1 commit into from
Sep 21, 2024
Merged

Exposed stream-ordering to datetime API #16774

merged 1 commit into from
Sep 21, 2024

Conversation

lamarrr
Copy link
Contributor

@lamarrr lamarrr commented Sep 9, 2024 •
edited
Loading

Description

This merge request exposes stream-ordering to the public-facing datetime APIs.

  • extract_year
  • extract_month
  • extract_day
  • extract_weekday
  • extract_hour
  • extract_minute
  • extract_second
  • extract_millisecond_fraction
  • extract_microsecond_fraction
  • extract_nanosecond_fraction
  • last_day_of_month
  • day_of_year
  • add_calendrical_months
  • is_leap_year
  • days_in_month
  • extract_quarter
  • ceil_datetimes
  • floor_datetimes
  • round_datetimes

Follows-up #13744
Closes #16775

Checklist

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

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 9, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 9, 2024
@lamarrr lamarrr marked this pull request as ready for review September 9, 2024 16:36
@lamarrr lamarrr requested a review from a team as a code owner September 9, 2024 16:36
@lamarrr lamarrr changed the title Added stream-ordering to datetime API Exposed stream-ordering to datetime API Sep 9, 2024
@lamarrr lamarrr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 9, 2024
@davidwendt
Copy link
Contributor

May want to consider the following issue/comment as well #16735 (comment)

bdice
bdice reviewed Sep 9, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I have a couple suggestions. Please add stream tests. You can copy existing datetime tests into the streams test directory, and pass cudf::test::get_default_stream() to the new stream-ordered public APIs.

cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Sep 9, 2024

May want to consider the following issue/comment as well #16735 (comment)

I like that idea but it should be done in a separate PR. Let's just add stream ordered public APIs in this one.

@davidwendt
Copy link
Contributor

May want to consider the following issue/comment as well #16735 (comment)

I like that idea but it should be done in a separate PR. Let's just add stream ordered public APIs in this one.

But it would be wasted effort to add streams to APIs that are going to be deleted, right?

@bdice
Copy link
Contributor

bdice commented Sep 9, 2024

We could remove the stream-ordering changes from the extract APIs in this PR if you're worried about duplication of effort, but I would just move forward with stream-ordering and then worry about deprecation/removal at a later time. Adding the single extract-with-enum API and migrating to it may require a bit of effort.

@karthikeyann
Copy link
Contributor

/ok to test

1 similar comment
@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 11, 2024

/ok to test

@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 11, 2024

/ok to test

2 similar comments
@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 11, 2024

/ok to test

@karthikeyann
Copy link
Contributor

/ok to test

karthikeyann
karthikeyann approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 11, 2024

/ok to test

PointKernel
PointKernel requested changes Sep 14, 2024
View reviewed changes
Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

Good to go once the corresponding stream test has been added.

@github-actions github-actions bot added the CMake CMake build issue label Sep 16, 2024
@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 16, 2024

/ok to test

@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 16, 2024

Looks great.

Good to go once the corresponding stream test has been added.

thanks! I've added them now

PointKernel
PointKernel approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final suggestion. Thank you @lamarrr

cpp/tests/streams/datetime_test.cpp Outdated Show resolved Hide resolved
@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 16, 2024

/ok to test

1 similar comment
@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 17, 2024

/ok to test

@lamarrr lamarrr force-pushed the datetime-stream-ordering branch from c4453b9 to 7d79cd3 Compare September 19, 2024 12:30
@lamarrr
Added stream-ordering to datetime API
d01bda9 
Fixed include style and stream documentation

Fixed stream param documentation formatting

updated doxygen copydoc for cudf APIs

Fixed include formatting

Removed duplicated stream param

added default stream to make_timezone_transition_table

added default stream to make_timezone_transition_table

added default stream to make_timezone_transition_table

Added stream tests for datetime API

update license notice

Co-authored-by: Yunsong Wang <[email protected]>

collapsed test fixtures

removed redundant include
@lamarrr lamarrr force-pushed the datetime-stream-ordering branch from 7d79cd3 to d01bda9 Compare September 21, 2024 11:20
@lamarrr
Copy link
Contributor Author

lamarrr commented Sep 21, 2024

/merge

@rapids-bot rapids-bot bot merged commit 9b4c4c7 into rapidsai:branch-24.10 Sep 21, 2024
97 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt left review comments

@bdice bdice bdice left review comments

@PointKernel PointKernel PointKernel approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

Assignees
No one assigned
Labels
CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Expose Stream Ordering to the Datetime APIs
5 participants
@lamarrr @davidwendt @bdice @karthikeyann @PointKernel