-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(sdk): expose IfPresentPlaceholder and ConcatPlaceholder to kfp.dsl #8145
feat(sdk): expose IfPresentPlaceholder and ConcatPlaceholder to kfp.dsl #8145
Conversation
Hi @zichuan-scott-xu. 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. |
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.
@zichuan-scott-xu, this looks really good. I'm curious if we can find a way to improve the way children of BaseModel
are documented by our reference docs so that only the official user-facing methods and attributes are documented. Currently, methods like split_cel_concat_string
and to_placeholder_string
are getting documented. We don't want to promise the stability of anything other than the constructor API if we can help it. Plus, these helper methods add clutter to the docs.
Currently we use Sphinx automodule to automatically document the .dsl
module. The source .rst
file that specifies how to do this is here.
If you cd docs
then run make clean html
, then open _build/html/index.html
, you can build and inspect the docs from source.
An easy approach to handling this would be to use an _
prefix to make the method hidden, but this involves changing quite a bit of code, down to the BaseModel
methods. This is fine if we want to go this route, but I'm curious what you think and if there are any other approaches you like more.
/ok-to-test |
Let's handle this documentation change in a separate PR. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: connor-mccarthy 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 |
/retest |
2 similar comments
/retest |
/retest |
Description of your changes:
This PR exposes
IfPresentPlaceholder
andConcatPlaceholder
to kfp.dsl and add examples to show how they could be used to author containerized components.Checklist: