-
Notifications
You must be signed in to change notification settings - Fork 835
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
Explainer Gateway Fix for Istio, Ambassador and Python Client #1668
Explainer Gateway Fix for Istio, Ambassador and Python Client #1668
Conversation
/test integration |
Mon Apr 6 17:11:27 UTC 2020 impatient try |
Mon Apr 6 17:11:28 UTC 2020 impatient try |
Mon Apr 6 17:13:04 UTC 2020 impatient try |
/test integration |
Mon Apr 6 19:21:21 UTC 2020 impatient try |
Mon Apr 6 19:22:08 UTC 2020 impatient try |
Mon Apr 6 19:22:28 UTC 2020 impatient try |
/test integration |
Mon Apr 6 19:55:42 UTC 2020 impatient try |
Mon Apr 6 19:55:53 UTC 2020 impatient try |
Mon Apr 6 19:56:23 UTC 2020 impatient try |
/test integration |
Mon Apr 6 20:01:47 UTC 2020 impatient try |
Mon Apr 6 20:01:50 UTC 2020 impatient try |
Mon Apr 6 20:03:17 UTC 2020 impatient try |
/test integration |
Mon Apr 6 21:37:24 UTC 2020 impatient try |
Wed Apr 8 17:57:15 UTC 2020 impatient try |
Wed Apr 8 17:57:19 UTC 2020 impatient try |
Thu Apr 9 08:20:49 UTC 2020 impatient try |
Thu Apr 9 08:20:56 UTC 2020 impatient try |
Thu Apr 9 09:03:06 UTC 2020 impatient try |
Thu Apr 9 09:03:07 UTC 2020 impatient try |
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.
Awesome! I've added a few comments, but it looks pretty good!
@@ -247,7 +247,7 @@ func createExplainerIstioResources(pSvcName string, p *machinelearningv1.Predict | |||
{ | |||
Uri: &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Prefix{Prefix: "/seldon.protos.Seldon/"}}, | |||
Headers: map[string]*istio_networking.StringMatch{ | |||
"seldon": &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Exact{Exact: mlDep.Name}}, | |||
"seldon": &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Exact{Exact: mlDep.GetName()}}, |
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.
Interesting, I didn't know that ObjectMeta
also had methods defined!
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.
No, those are methods that we added via composition, it's really one that we added to make the name with a hash if too long. Actually, this may have been intended tho - @cliveseldon could you confirm if this was intended?
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.
Which code are you referring to?
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.
oh - those are just convenience methods added by k8s. name
and GetName
are the same.
@@ -528,6 +529,7 @@ def explain( | |||
headers: Dict = None, | |||
http_path: str = None, | |||
client_return_type: str = None, | |||
predictor: str = None, |
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.
nit (feel free to ignore): should we call the arg predictor_name
instead of predictor
?
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.
Haha I thought of the same, I think the reason I ended up goign with this is because there were no other params with the format x_name, i.e. method, gateway, namespace, etc
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.
There is deployment_name
actually!
deployment_name: str = None, |
Thu Apr 9 14:00:58 UTC 2020 impatient try |
Thu Apr 9 14:01:10 UTC 2020 impatient try |
/test integration |
Sat Apr 11 08:37:42 UTC 2020 impatient try |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cliveseldon 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 |
/test integration |
Tue Apr 14 09:37:34 UTC 2020 impatient try |
Tue Apr 14 09:37:37 UTC 2020 impatient try |
Tue Apr 14 09:38:38 UTC 2020 impatient try |
Tue Apr 14 11:25:04 UTC 2020 impatient try |
Tue Apr 14 11:25:06 UTC 2020 impatient try |
Fixes #1627
Fixes #1626