chore: Clean up KFP SDK docstrings, make formatting a little more consistent #4218
Conversation
|
This change is |
|
Hi @alexlatchford. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @paveldournov ~~Looks like you're the recommended reviewer, this PR isn't 100% ready although it could be merged, I've really only gone through ~50% of the modules but I'm planning on giving the same treatment to all of them if this approach is approved. Current progress:~~ Finished all the modules now:
|
|
/assign Super thanks! |
| @@ -140,6 +139,10 @@ | |||
| # | |||
| # html_sidebars = {} | |||
|
|
|||
| html_css_files = [ | |||
| 'custom.css', | |||
| ] | |||
There was a problem hiding this comment.
Needed this to break the lines of long code blocks, see here for an example.
sdk/python/kfp/_client.py
Outdated
| other_client_secret: The client secret used to obtain the auth codes and refresh tokens. | ||
| existing_token: Pass in token directly, it's used for cases better get token outside of SDK, e.x. GCP | ||
| Cloud Functions or caller already has a token | ||
| cookies: CookieJar object containing cookies that will be passed to the pipelines API. |
There was a problem hiding this comment.
Something I noticed consistently was that we were missing the API docs for constructors despite them being there. Looks like Sphinx reads from the class docstring by default, you can have it read from the class docstring or __init__ or both so happy to conform to either if people have a preference. For now I just kept the Sphinx default.
| @@ -79,17 +80,19 @@ def build_image_from_working_dir(image_name: str = None, working_dir: str = None | |||
| file_filter_re: Optional. A regular expression that will be used to decide which files to include in the container building context. | |||
| timeout: Optional. The image building timeout in seconds. | |||
| base_image: Optional. The container image to use as the base for the new image. If not set, the Google Deep Learning Tensorflow CPU image will be used. | |||
| builder: Optional. An instance of ContainerBuilder or compatible class that will be used to build the image. | |||
| builder: Optional. An instance of :py:class:`kfp.containers.ContainerBuilder` or compatible class that will be used to build the image. | |||
There was a problem hiding this comment.
Just wanted to highlight the syntax we can use to add in cross-SDK reference hyperlinks. There are a bunch more than just :py:class: but basically all follow a similar format: https://www.sphinx-doc.org/en/1.7/domains.html#python-roles
| ... | ||
| ``` | ||
| Example: | ||
| :: |
There was a problem hiding this comment.
Another common problem I fixed was setting injecting literal (::) so Sphinx would interpret the example as a formatted code block.
Looks like Sphinx needs the :: to be followed by a blank line then the code block indented for it to actually format nicely. Guessing there is likely something we can do with a linter to begin to enforce this but for now I've just fixed them manually.
alexlatchford
force-pushed
the
alexla/cleanup-sdk-docs
branch
from
1afcfbe to
b7b132f
Compare
|
Wow! Thank you for this tremendous work!
JFYI: Do not bother with I only have couple of small comments:
|
|
/ok-to-test |
Bobgy
added
the
cherrypick-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@alexlatchford Do you have suggestions on where to read about syntax and how to verify rendered documentation locally? Also, is there any lint tools that can catch the most common mistakes? These will be helpful to let us follow best practices along new changes. |
|
Hey @Bobgy I can write up something and add it to the Do you need anything from me to fix those pending tests (which I'm guessing will block the merging)?
|
|
That will be awesome! Separate PR also works. I think they are already fixed. |
|
Prow seems to be having trouble merging this PR. |
|
/hold |
|
Seems to be a known issue #1653 The cla/google check is the only thing not passed (without the hold), so Prow thinks it can merge, but GitHub doesn't. |
|
@googlebot rescan |
|
/unhold Sorry for confusion @chases2, sometimes the cla check stuck. |
|
/retest Looks like some build failure in master. I will fix it next week |
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
/test kubeflow-pipeline-backend-test |
|
/retest |
|
@alexlatchford: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Force merged because failures are flaky. |
…sistent (kubeflow#4218) * Prepare SDK docs environment so its easier to understand how to build the docs locally so theyre consistent with ReadTheDocs. * Clean up docstrings for kfp.Client * Add in updates to the docs for compiler and components * Update components area to add in code references and make formatting a little more consistent. * Clean up containers, add in custom CSS to ensure we do not overflow on inline code blocks * Clean up containers, add in custom CSS to ensure we do not overflow on inline code blocks * Remove unused kfp.notebook package links * Clean up a few more errant references * Clean up the DSL docs some more * Update SDK docs for KFP extensions to follow Sphinx guidelines * Clean up formatting of docstrings after Ark-Kuns comments
Bobgy
removed
the
cherrypick-approved
|
Cannot really cherrypick this to 1.0.1 because there are merge conflicts |
…sistent (kubeflow#4218) * Prepare SDK docs environment so its easier to understand how to build the docs locally so theyre consistent with ReadTheDocs. * Clean up docstrings for kfp.Client * Add in updates to the docs for compiler and components * Update components area to add in code references and make formatting a little more consistent. * Clean up containers, add in custom CSS to ensure we do not overflow on inline code blocks * Clean up containers, add in custom CSS to ensure we do not overflow on inline code blocks * Remove unused kfp.notebook package links * Clean up a few more errant references * Clean up the DSL docs some more * Update SDK docs for KFP extensions to follow Sphinx guidelines * Clean up formatting of docstrings after Ark-Kuns comments
If you want to check out the rendering of this build you can see it here: https://al-kubeflow-pipelines.readthedocs.io/en/alexla-cleanup-sdk-docs/
Some of the biggest changes can be seen in the
kfp.Clientchanges, old version vs new version in this PR.Description of your changes:
Done:
kfp.notebookpackage for now as it's not actually rendering anything.modules.rstlisting, wasn't actually linked anywhere and it's duplicated by the functionality in theindex.rst..readthedocs.ymlconfig file to make it easier to understand the dependency structure of the KFP SDK docs.My plan is to get a little bit of feedback on whether this is a good approach before updating the remainder of the modules in a similar fashion.
Would be nice to
cherry-pickthis across since it makes the formatting of the KFP SDK docs a little more consistent.