feat: make KFP v2 compatible mode work out of the box #284
Conversation
|
[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 |
|
/hold |
|
/ok-to-test |
| export KUBEFLOW_PIPELINES_VERSION=1.5.0 | ||
| export KUBEFLOW_PIPELINES_REPO=https://github.com/kubeflow/pipelines.git | ||
| # Pull Kubeflow Pipelines upstream manifests. | ||
| ./apps/pipelines/pull-upstream.sh |
There was a problem hiding this comment.
Should we use make -C apps/pipelines pull-upstream instead?
There was a problem hiding this comment.
This is a good point. I am not sure. If we add it to makefile the model is more consistent with other commands.
If use separate pull-upstream.sh, packages without makefile can easily add them too.
There was a problem hiding this comment.
The reason I ask is that you have already created a pull-upstream make target in https://github.com/kubeflow/gcp-blueprints/blob/2b8e52dd10796d78e40f40d6d9617b9e64b773b3/kubeflow/apps/pipelines/Makefile#L73, so I guess the goal is to use it.
As mentioned in the other comment, if the eventual state is to separate pull-upstream commands to each sub-folder, or use kpt subpackage when it becomes available, I prefer to create a template script for pull-upstream, so each sub-folder can call this template from Makefile, instead of having pull-upstream.sh in each sub-folder. Because it will increase amount of files in the whole repo, and it is hard to keep them consistent. If there is no Makefile, then we will use the default template to pull upstream.
For this PR, feel free to keep the current usage. And we can update them at one go when we break down the kubeflow/pull-upstream.sh.
There was a problem hiding this comment.
Yes, I added the makefile rule for consistency.
If there is no Makefile, then we will use the default template to pull upstream.
I think we need to know where upstream is and which version, these info need to be somewhere in the folder.
|
/lgtm Waiting for upstream target to be changed to KFP 1.6.1. |
|
Perhaps we can rename this PR "make KFP v2 compatible mode work out of the box". |
Bobgy
changed the title
Bobgy
changed the title
|
/hold |
|
Any plan for merging this PR? |
|
/unhold |
Part of kubeflow/pipelines#5680
Depends on kubeflow/pipelines#5750
Changes:
default-pipeline-root