-
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
Changes for kfserving 0.4.1 #4479
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hi @juliusvonkohout. 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. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
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.
/lgtm
cc @Tomcli
Thanks @juliusvonkohout, can you also update this line to version 0.4.0 as well? I updated the 0.4.0 image to based on your branch.
Otherwise it looks good to me. |
I have changed the version now. |
/lgtm |
/ok-to-test |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@juliusvonkohout Does this need a refacotr for kfserving 0.4.1? |
@davidspek tehere are two places where 0.4.0 should be replaced with 0.4.1 and it should be rebased against master |
i guess you have to update to 0.4.1 and build the image and test again. Feel free to make any edits. |
) | ||
print("Sample test commands: \n") | ||
# model_status['status']['url'] is like http://flowers-sample.kubeflow.example.com/v1/models/flowers-sample | ||
|
||
host, path = url.sub("", model_status["status"]["address"]["url"]).split("/", 1) |
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.
Few weeks ago, we discovered that this split command may not work with some DNS for OpenShift route. So we removed it to avoid any confusion and error.
7542f0e
to
44d22a6
Compare
@juliusvonkohout the kubeflow-pipelines-component-yaml test is fixed now. Can you rebase this PR and remove the old changes from line 410-422? thanks |
@Tomcli I strongly hope that my resolve was correct. Someone should test this first. |
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.
@juliusvonkohout I have few comments for the rebase, thanks.
Co-authored-by: Tommy Li <Tommy.chaoping.li@ibm.com>
We should squash commit such that there are not so many garbage mini edits from me. |
/lgtm |
@animeshsingh can you approve this PR? Thanks. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: animeshsingh, juliusvonkohout 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 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: animeshsingh, juliusvonkohout 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 |
1 similar comment
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: animeshsingh, juliusvonkohout 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 |
@Tomcli thank you very much for this effort. I already started to think that it will just be forgotten or is not compatible anymore. |
Description of your changes:
Checklist:
The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.
PR titles examples:
fix(frontend): fixes empty page. Fixes #1234
Use
fix
to indicate that this PR fixes a bug.feat(backend): configurable service account. Fixes #1234, fixes #1235
Use
feat
to indicate that this PR adds a new feature.chore: set up changelog generation tools
Use
chore
to indicate that this PR makes some changes that users don't need to know.test: fix CI failure. Part of #1234
Use
part of
to indicate that a PR is working on an issue, but shouldn't close the issue when merged.Do you want this pull request (PR) cherry-picked into the current release branch?
If yes, use one of the following options:
cherrypick-approved
label to this PR. The release manager adds this PR to the release branch in a batch update.