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

Introduce deprecation policy to developer guide. #10252

Merged
merged 7 commits into from
Apr 21, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 8, 2022

This PR resolves #10159.

@vyasr vyasr added 3 - Ready for Review Ready for review by team doc Documentation libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Feb 8, 2022
@vyasr vyasr self-assigned this Feb 8, 2022
@vyasr vyasr requested a review from a team as a code owner February 8, 2022 19:43
@vyasr vyasr requested review from jrhemstad and vuule February 8, 2022 19:43
cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

👍 for the PR.
The promise that we'll always keep old APIs for another release is a tad scary and maybe worth an additional discussion (unless it happened and I missed it).

@vyasr
Copy link
Contributor Author

vyasr commented Feb 10, 2022

+1 for the PR. The promise that we'll always keep old APIs for another release is a tad scary and maybe worth an additional discussion (unless it happened and I missed it).

Sounds good. I'm happy to get more opinions on this, perhaps in the meeting next week? I do think that promising users that their code won't break without warning is a large part of what makes a deprecation policy valuable, but I definitely don't mean to impose such a policy unilaterally.

@bdice
Copy link
Contributor

bdice commented Feb 11, 2022

I'm approving this PR's language but I am not yet certain about the policy itself and am eager to hear others' thoughts in our next libcudf meeting.

The promise that we'll always keep old APIs for another release is a tad scary and maybe worth an additional discussion

It seems to me that libcudf's willingness to break APIs quickly (and fix them internally for our Python/Java APIs) is important for the library's development, and changing this policy could impose significant maintenance costs that we currently do not have to pay. While I believe deprecations could be important for external C++ consumers (excluding Python/Java), I have not seen explicit external requests for us to offer more warning before breaking libcudf APIs. This is a key point that I think deserves input from @jrhemstad.

@vuule vuule added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Feb 11, 2022
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

I'm going to suggest a radically more laissez-faire policy.

libcudf is constantly growing and improving to improve performance and better meet our users' needs. As a result, we occasionally need to break or entirely remove APIs to respond to new and improved understanding of the functionality we provide. Remaining free to do this is essential to making libcudf an agile library that can rapidly accommodate our users needs. We encourage users to Live At Head.

In order to advertise these breaking changes, any PR that breaks or removes an existing API should be tagged as "breaking". This will ensure that the "Breaking" section of the release notes will include a description of what has broken from the past release.

On a best effort basis, the libcudf team will notify users of changes that we expect to have significant or widespread effects.

cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Feb 15, 2022

@jrhemstad I would also be fine with that policy. There are definitely pros and cons to consider here. I'll defer to whatever decision you, @harrism, @GregoryKimball, and @JohnZed feel is appropriate here, I would simply like us to have a clearly documented and agreed upon policy. I have less experience with our user base than others so it's difficult for me to weigh the tradeoffs.

@jrhemstad
Copy link
Contributor

@jrhemstad I would also be fine with that policy. There are definitely pros and cons to consider here. I'll defer to whatever decision you, @harrism, @GregoryKimball, and @JohnZed feel is appropriate here, I would simply like us to have a clearly documented and agreed upon policy. I have less experience with our user base than others so it's difficult for me to weigh the tradeoffs.

Couldn't agree more. It's important to write down what we do even if what we write down is "We reserve the right to break your code at any time without warning", which isn't that far from the truth :)

cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
@harrism
Copy link
Member

harrism commented Feb 16, 2022

@jrhemstad I would also be fine with that policy. There are definitely pros and cons to consider here. I'll defer to whatever decision you, @harrism, @GregoryKimball, and @JohnZed feel is appropriate here, I would simply like us to have a clearly documented and agreed upon policy. I have less experience with our user base than others so it's difficult for me to weigh the tradeoffs.

Jake's proposed policy is basically what we follow today. I am fine with keeping that agile approach, and documenting it. libcudf has the advantage that most users interface with it via Python or Java. As long as we ensure we don't break those users (by adapting / updating bindings in the same release that we change an API), changing the C++ API is usually OK.

I think we should use the [deprecated] policy for API removals, but allow breaking API changes as long as we fix all bindings we can and inform users (e.g. Spark) that we can directly, within the same release.

There are also other RAPIDS libraries that depend on libcudf, so we should be careful to not break those as well.

@vyasr vyasr requested review from jrhemstad and harrism February 17, 2022 02:25
@vyasr
Copy link
Contributor Author

vyasr commented Feb 17, 2022

@harrism @jrhemstad I've attempted to synthesize your suggestions into a consistent set of guidelines that basically say "we reserve the right to break things any time, anywhere, but we'll at least do our best to warn you before we remove APIs" 😄. Let me know if you would like any further changes.

cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/docs/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/docs/DOCUMENTATION.md Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from harrism February 17, 2022 20:01
@vyasr
Copy link
Contributor Author

vyasr commented Mar 21, 2022

This PR is waiting for @JohnZed to take a look after GTC is completed.

@vyasr vyasr closed this Mar 31, 2022
@vyasr vyasr deleted the doc/deprecations branch March 31, 2022 20:08
@ttnghia
Copy link
Contributor

ttnghia commented Mar 31, 2022

What?

@vyasr vyasr restored the doc/deprecations branch March 31, 2022 20:46
@vyasr
Copy link
Contributor Author

vyasr commented Mar 31, 2022

Whoops accidental closing.

@vyasr vyasr reopened this Mar 31, 2022
@vyasr vyasr changed the base branch from branch-22.04 to branch-22.06 April 8, 2022 17:37
@JohnZed
Copy link
Contributor

JohnZed commented Apr 20, 2022

I think the text around best effort deprecation is appropriate for libcudf. It has far fewer direct callers than, say, the Python API, so we can afford to evolve it more quickly. I would like to see a more formal deprecation policy for the Python side that does ensure we do a 1-2 release deprecation plan. That should not need to block this PR. Within Python, it is often easier to provide a deprecation path, and we have far more callers

I think the suggestion to live at head is somewhat unnecessary from this deprecation policy and likely overly prescriptive for end users. The linked video is an hour and a half, and it would be difficult if we imply users need to watch a 90 minute video to understand our deprecation approach. I don't think we're ready to meet all of the criteria that e.g. Abseil has in place for live at head. E.g. they commit to shipping refactoring tools to automate upgrades to support API removals and describe a process of "ship a tool, wait for people to apply the tool, then if practical remove the old API." This is much stricter than what is proposed elsewhere in the doc. In the absence of such tools and guarantees of easy upgrades, I suggest just omitting the "live at head" from text and keeping the best effort plan, while really ensuring we make that effort to simplify upgrades when possible.

We can revisit the best effort deprecation in the future if concrete user needs pop up. To a large extent, I think the Python API is the best place for us to have a "buffer" to allow us to upgrade libcudf APIs without impacting users of the most public-facing API, so we are just leaning on that buffer with this policy.

@vyasr
Copy link
Contributor Author

vyasr commented Apr 21, 2022

I have removed the recommendation (and link) to live at head. I agree that it's not explicitly necessary.

Regarding Python, I introduced a deprecation policy of 1 release when I wrote the current draft of the Python developer guide, and we have been following that rule (consistently but somewhat unofficially) for the last 3-4 releases. I am currently working on porting that developer guide into a more visible space, at which point it should become less unofficial, but I agree that the deprecation policy is more critical there and so I prioritized getting that in place first.

@vyasr vyasr force-pushed the doc/deprecations branch from 845bcd6 to 90c7156 Compare April 21, 2022 17:33
@vyasr vyasr removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Apr 21, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Apr 21, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8a4d1b2 into rapidsai:branch-22.06 Apr 21, 2022
@vyasr vyasr deleted the doc/deprecations branch April 21, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team doc Documentation libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libcudf deprecation policy
7 participants