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

[REVIEW] Drop kind parameter from Index.get_slice_bound #12856

Conversation

galipremsagar
Copy link
Contributor

Description

This PR drops kind parameter from Index.get_slice_bound to match pandas-2.0 API.

Checklist

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

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer improvement Improvement / enhancement to an existing function breaking Breaking change labels Feb 27, 2023
@galipremsagar galipremsagar requested a review from a team as a code owner February 27, 2023 21:31
@galipremsagar galipremsagar self-assigned this Feb 27, 2023
@galipremsagar galipremsagar requested review from wence- and isVoid and removed request for a team February 27, 2023 21:31
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Code looks great.

One question, do we want to collate (or somehow have consistent changelog naming) these PRs that drop deprecated functionality?

@galipremsagar
Copy link
Contributor Author

One question, do we want to collate (or somehow have consistent changelog naming) these PRs that drop deprecated functionality?

Having a list of all these breaking changes merged into the feature branch in the changelog will be very useful. @ajschmidt8 Would it be possible today, if not happy to add manual entries too.

@ajschmidt8
Copy link
Member

Having a list of all these breaking changes merged into the feature branch in the changelog will be very useful. @ajschmidt8 Would it be possible today, if not happy to add manual entries too.

Hmmm. Our current changelog process doesn't really allow for manual overrides.

The running changelogs (e.g. this one) are re-generated from scratch every time a PR is merged. If the PR has a breaking label on it, it's title and link will appear under the Breaking Changes section.

@galipremsagar
Copy link
Contributor Author

If the PR has a breaking label on it, it's title and link will appear under the Breaking Changes section.

Does this happen even if the PR was merged to a feature branch, I assume no? right?

@ajschmidt8
Copy link
Member

If the PR has a breaking label on it, it's title and link will appear under the Breaking Changes section.

Does this happen even if the PR was merged to a feature branch, I assume no? right?

Correct. But presumably this feature branch will eventually be merged into a regular development branch, right?

As long as that particular PR contains the breaking label, it will show up in Breaking Changes section.

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Mar 7, 2023

Correct. But presumably this feature branch will eventually be merged into a regular development branch, right?

As long as that particular PR contains the breaking label, it will show up in Breaking Changes section.

Right, but the thing we are trying to capture here is not the PR title of the final PR to regular dev branch. We actually want to capture the title's of the PRs being merged into pandas_2.0_feature_branch, so that we can later have them added to the actual regular development branch.

Today merging a feature branch to regular dev branch would generate something like this in changelog:


# cuDF 22.04.00 (Date..)

## 🚨 Breaking Changes

- Add pandas 2.0 support. ([#12113](https://github.com/rapidsai/cudf/pull/12113)) [@firestarman](https://github.com/firestarman)

But instead what we want to happen is:


# cuDF 22.04.00 (Date..)

## 🚨 Breaking Changes

- Add pandas 2.0 support. ([#12113](https://github.com/rapidsai/cudf/pull/12113)) [@firestarman](https://github.com/firestarman)
- Drop `kind` parameter from `Index.get_slice_bound`. ([#12856](https://github.com/rapidsai/cudf/pull/12856)) [@galipremsagar ](https://github.com/galipremsagar)
- Drop `inplace` parameter in categorical methods. ([#12846](https://github.com/rapidsai/cudf/pull/12846)) [@galipremsagar ](https://github.com/galipremsagar)

#12856 & #12846 are the PRs merged to the feature branch, but there are changelog-worthy entries too. Since pandas 2.0 support will be a non-trivial change to our APIs, we actually would like to capture PRs being merged to the feature branch as changelog entries.

@vyasr
Copy link
Contributor

vyasr commented Mar 8, 2023

More generally, given that we are trying to be better about using feature branches for long-running projects that can be broken up into smaller pieces for easier review, it would be helpful for us to have some ability to configure how those final merges are translated into a changelog. There will be other instances where we want the same behavior: including one changelog entry for every PR merged into the feature branch instead of only one changelog entry for the final merge of the feature branch into the trunk.

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Mar 8, 2023
@galipremsagar
Copy link
Contributor Author

I'm going to merge this PR and keep the discussion going.

@galipremsagar galipremsagar merged commit 7ec76b7 into rapidsai:pandas_2.0_feature_branch Mar 8, 2023
@ajschmidt8
Copy link
Member

I see. At the moment there is no clean way for us to add those entries automatically.

Here is our current process:

  • Every time a PR is merged to a dev branch, regenerate the entire body of the associated pre-release from scratch (thereby overriding any manual changes that might have been made)
  • At release time, copy the final version of the auto-generated pre-release body into a new non-pre-release
  • Update the CHANGELOG.md file with the final contents from the previous step

This is all scripted, so there's not currently any good way to interject and add manual entries.

The only way I can see to add additional entries would be to open a PR to update CHANGELOG.md after it's been automatically updated and then ping the Ops team to copy those new contents into the GitHub release as well.

@ajschmidt8
Copy link
Member

Our automated changelog process is fragile at the moment.

It's handled by the ops-bot. I had to effectively write a plugin that adapts this open-source release-drafter project for our unconventional branching strategy.

At some point, the Ops team would like to revisit our branching strategy and change it to something more sane.

Once that happens, we'll have to revisit our automated changelog process (among other things).

At that point, we can probably take this feedback into consideration and try to find a better solution for this.

No ETA on that yet though. We have a lot of other items we need to address first.

@wence-
Copy link
Contributor

wence- commented Mar 9, 2023

This is all scripted, so there's not currently any good way to interject and add manual entries.

The only way I can see to add additional entries would be to open a PR to update CHANGELOG.md after it's been automatically updated and then ping the Ops team to copy those new contents into the GitHub release as well.

How about we write (as part of the final pandas-2.0 compat merge) a page in the docs detailing all the deprecations (this has a stable link), and the PR title is something like:

Add pandas 2.0 support (see docs.rapids.ai/... for details on breaking changes) ...

Obviously this isn't perfect for someone scanning through, but at least there are enough breadcrumbs for them to see where to look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants