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

feature(backend): Allow KFP apiserver to create recursion pipelineLoop CRs for the users #512

Merged
merged 33 commits into from
Jul 21, 2021

Conversation

Tomcli
Copy link
Member

@Tomcli Tomcli commented Mar 25, 2021

Which issue is resolved by this Pull Request:
Resolves #496

Description of your changes:
Since we don't have support for putting custom task in the task spec. The below diagram shows how we could offload some of the extra api calls from the users to KFP apiserver. Ideally, having taskSpec for custom task is a better solution but it will take a while before it can be used in a Tekton stable release.

This PR handles the first step below where KFP apiserver creates the custom task CR x N for the users. Where N is the number of CRs and each CR needs a new API call.
Screen Shot 2021-03-24 at 6 41 07 PM

Although this feature allows users to deploy all the necessary custom resources with one KFP API call, KFP doesn't clean up these custom resources because the same custom resource could have multiple owners. This issue should be resolved in the upcoming taskSpec support for the custom task, so we don't want to create new dependencies on this temporary solution.

Environment tested:

  • Python Version (use python --version): 3.7
  • Tekton Version (use tkn version): 0.21.0
  • Kubernetes Version (use kubectl version): 1.18
  • OS (e.g. from /etc/os-release):

Checklist:

@Tomcli
Copy link
Member Author

Tomcli commented Mar 31, 2021

A new use case for this feature is with recursion. Since recursion loop has to reference a loop resource, we cannot use taskSpec to replace this feature because taskSpec is not a referable resource that can call itself in the same pipeline.

@Tomcli Tomcli changed the title (feature) Allow KFP apiserver to create custom task CRs for the users feature(backend): Allow KFP apiserver to create custom task CRs for the users Apr 5, 2021
@Tomcli
Copy link
Member Author

Tomcli commented Apr 27, 2021

/hold
for reviews

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Thanks @Tomcli, I like it :-)

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckadner, Tomcli

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ckadner
Copy link
Member

ckadner commented Apr 27, 2021

Good idea to put this PR on hold for others to review, so I can safely add my +1 while still giving others a chance for review 👍🏻

/lgtm

@Tomcli
Copy link
Member Author

Tomcli commented Jul 20, 2021

@Udiknedormin We are planning to make this feature as default for recursion. Can you give a quick review to make sure this won't break your team's use case? Thanks.

@Udiknedormin
Copy link
Contributor

@Tomcli It looks like it works similar to what we already implemented, i.e. auto-creates resources (although it seems to be at a different time). It also doesn't remove the resources while I think our version does. If I understood correctly, it also requires moving resources to some annotation, which our version doesn't do nor need.

We would rather still use our version, but if it's possible to simply turn this new feature off, I think we should be fine with it.

@Tomcli
Copy link
Member Author

Tomcli commented Jul 21, 2021

@Udiknedormin I added more details on how to disable this feature both from the API and SDK. The reason I didn't let kfp to remove the resources because there could be multiple pipelines that are referring to the same resource. If we think it is safe enough, I can add owner reference to those resources to bound to the latest pipelineRun that is using the recursive loop.

@Tomcli
Copy link
Member Author

Tomcli commented Jul 21, 2021

/unhold

@Tomcli
Copy link
Member Author

Tomcli commented Jul 21, 2021

@pugangxa can you give a final review and lgtm this PR? Thanks.

@Tomcli
Copy link
Member Author

Tomcli commented Jul 21, 2021

This feature should be enabled by default and can be opt-out with a flog

@pugangxa
Copy link
Contributor

/lgtm

@google-oss-robot google-oss-robot merged commit d9b8539 into kubeflow:master Jul 21, 2021
Tomcli added a commit to Tomcli/kfp-tekton that referenced this pull request Jul 22, 2021
@Tomcli Tomcli mentioned this pull request Jul 22, 2021
2 tasks
google-oss-robot pushed a commit that referenced this pull request Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for auto apply custom task resources until taskSpec is implemented.
6 participants