-
Notifications
You must be signed in to change notification settings - Fork 123
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: switch to v1 API paths for backend. #1121
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ScrapCodes 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 |
87c42ff
to
6bed352
Compare
currently trying to figure out, why it is querying v1beta1
|
which microservice is this? |
@ScrapCodes you probably want to update the deployment manifests too, seems like you are still deploying with the health checking pinning the v1beta1 api. |
In the package frontend/ , how does
|
Hi @Tomcli, this PR is ready for review. To be noted : change set of API (v1beta1) -> v1 is large and done by manual replacements. |
/retest |
@ScrapCodes for the apiserver it seems like your http proxy wasn't updated properly, so the pod just start crashing after the server enters the main program. |
@ScrapCodes same thing with the persistent agent |
@ScrapCodes also this const needs to change to v1 as well |
Also we haven't update the python client so all the tests with python client so the e2e test and integration will be broken for this PR, but we should at least pass the deployment test after fixing these apis and see |
@@ -25,5 +25,5 @@ docker build -t "${REGISTRY}/kfp-tekton/persistenceagent:latest" -f backend/Dock | |||
docker build -t "${REGISTRY}/kfp-tekton/metadata-writer:latest" -f backend/metadata_writer/Dockerfile . && docker push "${REGISTRY}/kfp-tekton/metadata-writer:latest" & | |||
docker build -t "${REGISTRY}/kfp-tekton/scheduledworkflow:latest" -f backend/Dockerfile.scheduledworkflow . && docker push "${REGISTRY}/kfp-tekton/scheduledworkflow:latest" & | |||
docker build -t "${REGISTRY}/kfp-tekton/cache-server:latest" -f backend/Dockerfile.cacheserver . && docker push "${REGISTRY}/kfp-tekton/cache-server:latest" & | |||
|
|||
docker build -t "${REGISTRY}/kfp-tekton/frontend:latest" -f frontend/Dockerfile . && docker push "${REGISTRY}/kfp-tekton/frontend:latest" & |
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.
just fyi we didn't add frontend in our github actions because it sometimes get over the github action vm quota. We do test our frontend code in our ibm cluster ci by running the Manual Trigger - e2e
at https://cloud.ibm.com/devops/pipelines/tekton/f85a45d2-36fc-45fb-a449-b01c13cf5191?env_id=ibm%3Ayp%3Aus-south
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.
The github action e2e script itself is communicating between the python client to api server, so it never talks to the ui itself.
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.
@yhwang fyi maybe we should have another github action to build ui images when the ui code is modified.
Thanks @ScrapCodes with the new fixes I verified the UI client works with the new V1 API. This PR haven't add any changes to the python sdk client so the sdk client failing since there's no v1beta1 API anymore. Since this PR is getting too big, I'm planning just merge this PR first and have the add the new changes to the SDK client as soon as possible. I will cut a v1beta1-api branch for now during this migration. @yhwang WDYT? |
agree. it's a good idea to cut a v1beta1-api branch. Another way around is to have a v1-api branch and land those v1 API related PRs there. once everything is settled, merge the branch back to the master. But this PR is open against the master now. so I guess you can just land this change into the master. we may do that next time. |
@ScrapCodes fyi i will modify the python sdk client to use the new api, once i have that ready working with this PR. Then I will merge this and the python client pr so that the master branch won't be failing. |
1. from modified swagger definition and python generator script. 2. Also deleted old python client files.
Hi @Tomcli , I was unable to figure - where it is referred by python SDK. |
Thanks @ScrapCodes, can you also add @yhwang and my ibm email to the maintainer list in the kfp-tekton-server-api package? Also, the original api client is referred in the kfp package. We need to create a new inherent client class to replace the v1beta1 client code to v1 as a new client function. https://github.com/kubeflow/pipelines/blob/sdk/release-1.8/sdk/python/requirements.in#L20 |
Hi Tommy, I will need your username, https://test.pypi.org/account/register/ |
@ScrapCodes my pypi user name is tomcli |
… containing the V1 API.
@ScrapCodes this is me: https://pypi.org/user/yihongwang/ |
sdk/python/requirements.in
Outdated
protobuf>=3.20.1,<4 | ||
kfp-tekton-server-api==1.5.0 |
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.
kfp-tekton-server-api==1.5.0 | |
kfp-tekton-server-api>=1.5.0 |
This way is more flexible if we need to add bug patches to the api package.
Thanks @ScrapCodes, I will open a PR shortly with the new api package you have. Can you fix the python lint so that I can merge this PR? Thanks
|
@Tomcli done! Also did you receive the invite to collaborate on pypi published package? |
thanks @ScrapCodes, I will merge this for now and add a patch for the e2e test. Some tests need to be updated. |
Which issue is resolved by this Pull Request:
Resolves #1081
Description of your changes:
Environment tested:
python --version
):tkn version
):kubectl version
):/etc/os-release
):Checklist: