[DOC] Add docstrings to functions, classes, and methods in api_context_managers.py #5544
Conversation
|
Pull requests from external contributors require approval from a |
csadorf
added
doc
| @@ -66,10 +66,31 @@ def _using_mirror_output_type(): | |||
|
|
|||
|
|
|||
| def in_internal_api(): | |||
| """ | |||
| Checks if the code is currently executing within an internal API context. | |||
There was a problem hiding this comment.
For this kind of call, it would be good to provide some context on exactly what this means. The name of the method is already fairly descriptive, so it is more important to emphasize why we care about this. For this particular case, it is important because once we are making calls internal to a cuML API call, we have already performed the necessary conversion to a CumlArray, and we want to avoid extra copies to device.
| return GlobalSettings().root_cm is not None | ||
|
|
||
|
|
||
| def set_api_output_type(output_type: str): | ||
| """ | ||
| Sets the output type for the current internal API context. |
There was a problem hiding this comment.
Explain what the output type is and what the impact is of setting this.
| @@ -87,6 +108,19 @@ def set_api_output_type(output_type: str): | |||
|
|
|||
|
|
|||
| def set_api_memory_type(mem_type): | |||
| """ | |||
| Sets the memory type for the current internal API context. | |||
There was a problem hiding this comment.
Explain what the memory type is and the impact of changing it.
| Raises | ||
| ------ | ||
| AssertionError | ||
| If the root context manager is not set. |
There was a problem hiding this comment.
In general, we do not want developers to have to dig too deeply into the internals of this, especially worrying about what the "root context manager" is or why it is important. Instead, let's describe under what circumstances the root context manager might not be set.
| @@ -100,6 +134,19 @@ def set_api_memory_type(mem_type): | |||
|
|
|||
|
|
|||
| def set_api_output_dtype(output_dtype): | |||
| """ | |||
| Sets the output data type for the current internal API context. | |||
There was a problem hiding this comment.
Explain the impact of setting this and what is meant by data type. I would actually stick to the word dtype, since it will be clearer to those familiar with numpy/cupy what this is.
| for cb in self._process_enter_cbs: | ||
| cb() | ||
|
|
||
|
|
||
| class ProcessReturn(object): | ||
| def __init__(self, context: "InternalAPIContextBase"): | ||
| """ | ||
| Constructor for the ProcessReturn class. |
There was a problem hiding this comment.
It should already be obvious that this is a constructor. Explain what this class does and why it is necessary.
| @@ -238,7 +336,19 @@ def __init__(self, context: "InternalAPIContextBase"): | |||
| ] = deque() | |||
|
|
|||
| def process_return(self, ret_val): | |||
|
|
|||
| """ | |||
| Execute the registered process return callbacks on the given return value. | |||
There was a problem hiding this comment.
Can you rephrase this to give a bit more of an intuitive sense of what is going on here? An internal call returns a value, and then the callbacks are executed on that value in order to do what?
| """ | ||
| Base class for the internal API context. | ||
|
|
||
| This class provides a context manager for managing the internal API context |
|
|
||
| ProcessEnter_Type: typing.Type[EnterT] = None | ||
| ProcessReturn_Type: typing.Type[ProcessT] = None | ||
|
|
||
| def __init__(self, func=None, args=None): | ||
| """ | ||
| Constructor for the InternalAPIContextBase class. |
There was a problem hiding this comment.
It should already be obvious that this is a constructor. Explain what this class does and why it is necessary.
| int | ||
| The count of how many times this context has been entered. | ||
| """ | ||
| # ... (Details not shown in the provided code snippet) |
|
@theaaravmakadia Can you respond to the review comments, please? |
|
Going to close this PR due to inactivity, especially given that it has fallen out of date with the current development branch. @theaaravmakadia, please do feel free to reopen or update if you have time/interest later on. |
Report needed documentation
While the estimator guide offers a great breakdown of how to use many of the tools in api_context_managers.py, it would be helpful to have information right in the docstring during development to more easily understand what is actually going on in each of the provided functions/classes/methods. This is particularly important for other developers who are trying to make changes to api_context_managers.py itself.
Describe the documentation you'd like
It would be great if each function/class/method had at least a brief docstring description for its intended behavior so that developers making changes have greater confidence that their changes are going in the appropriate place for the intended usage of each function/class/method. Even better would be a full docstring with parameter descriptions, etc.
Steps taken to search for needed documentation
Read the estimator guide and all documentation it links to.
@csadorf @wphicks --> PR related to #3470.