-
Notifications
You must be signed in to change notification settings - Fork 568
DOC Add developer documentation about deprecation policy #6476
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
base: branch-25.06
Are you sure you want to change the base?
Conversation
Please target branch-25.06 for this. |
558918b
to
ec0b0e2
Compare
ec0b0e2
to
3e02c86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for kicking this off!
docs/source/developers/index.rst
Outdated
cuML follows the policy of deprecating code for one release prior to removal. This applies | ||
to publicly accessible functions, classes, methods, attributes and parameters. During the | ||
deprecation cycle the old name or value is still supported, but will raise a deprecation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try to better defined "publicly accessible"? In my proposal, I tried to provide such a definition by referring to the user documentation.
Breaking changes are defined as changes that would break user code that is implemented according to our examples and API reference as published within the user documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure. I think we need a fairly simple rule that people who "don't care about cuml" can easily understand and follow. I'm thinking of people who use cuml but also use 20 other libraries to do their work. So they don't pay attention to cuml developments and can't remember what exactly the rules are for the libraries they depend on.
This makes me think that using public vs private Python API is a good way to go. It is straightforward and uses a concept that is known to everyone (cuml.foo.bar()
is public but cuml._foo.bar()
is not). I read "implemented according to our API reference as published" as "things that are public follow this deprecation policy". Less clear to me is what "according to our examples" means - do I have to find an example that uses a particular function to know it follows the deprecation policy? Do private functions that are used in examples follow the deprecation policy?
So in the end I thought "all public classes, methods, attributes and functions" is easy to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a fairly simple rule that people who "don't care about cuml" can easily understand and follow.
The rule we are proposing here is primarily targeting cuML maintainers and developers and in so far can and should be a bit more specific.
This makes me think that using public vs private Python API is a good way to go.
In principle yes, in practice there are many, many components that are currently not prefixed as such and I would safely assume to be private, e.g., everything within the cuml.internals
namespace. On the flip side, everything that is prefixed as such should be considered private. In the absence of very consistent prefixing, we need more concrete guidance on what should be considered be part of the public API and therefore must go through deprecation.
[...] do I have to find an example that uses a particular function to know it follows the deprecation policy?
Yes, reviewing the user documentation should be part of the process of determining whether deprecation is warranted or not.
Do private functions that are used in examples follow the deprecation policy?
If they have been a prominent part of our user documentation for a while they are probably by definition public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle yes, in practice there are many, many components that are currently not prefixed as such and I would safely assume to be private, e.g., everything within the
cuml.internals
namespace. On the flip side, everything that is prefixed as such should be considered private. In the absence of very consistent prefixing, we need more concrete guidance on what should be considered be part of the public API and therefore must go through deprecation.
I'd turn it around. Use a simple rule for private vs public, some common sense when applying deprecations(?), and fix the prefixing. There are things which we can just move (like cuml.internals
) and things that maybe we need a deprecation cycle to move. Making a more complicated rule because there is a bug in the library (inconsistent prefixing) feels like we are addressing the symptom and not the cause. I can get excited about addressing the cause, because it pays dividends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can work on improving the namespace, but until then the current guidance should not rely on prefixing except for "everything that is prefixed with _
is generally considered private".
docs/source/developers/index.rst
Outdated
Code in cuML should not use deprecated cuML APIs. | ||
|
||
|
||
.. code-block:: python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we show an example for libcuml as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea, but what is libcuml
? I feel like I should know the answer but I don't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C++ library that some of our algorithms and functions utilize. I've only found one example for API deprecation:
Lines 23 to 33 in b38d8be
/** | |
* @brief Synchronize CUDA stream and push a named nvtx range | |
* @param name range name | |
* @param stream stream to synchronize | |
*/ | |
[[deprecated("Use new raft::common::nvtx::push_range from <raft/core/nvtx.hpp>")]] inline void | |
PUSH_RANGE(const char* name, cudaStream_t stream) | |
{ | |
raft::common::nvtx::push_range(name); | |
} | |
Based on this, I would propose this example:
[[deprecated("Use new function_name from <header_file>")]]
void old_function() {
// Implementation that calls new function
new_function();
}
@divyegala can you confirm this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some references from RMM as well. @csadorf what you linked seems to me acceptable as precedent set by other RAPIDS libraries https://github.com/rapidsai/rmm/pull/1690/files#diff-db547576924fdcc827a79d62925a34ea52e3e81f51908f1cee4b606d006f6ab8R115-R117
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@divyegala Thanks a lot!
@betatim Can you adopt that, please?
This documents our deprecation policy. I started a new section for developers, right now only contains the deprecation policy.
closes #6436