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

Add model_name when chaining requests #3805

Merged
merged 10 commits into from
Feb 3, 2022

Conversation

adriangonz
Copy link
Contributor

What this PR does / why we need it:

  • Ensure that model_name is updated when chaining requests across a model graph.
  • Ensure that model_name is present on the first request (i.e. so that it's not necessary that the user sends it on the initial request to the graph).
  • Add integration tests around inference graphs in the V2 protocol (for both REST and gRPC).

Which issue(s) this PR fixes:

Fixes #3792

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@adriangonz
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign adriangonz
You can assign the PR to them by writing /assign @adriangonz in a comment when ready.

The full list of commands accepted by this bot can be found 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

@adriangonz
Copy link
Contributor Author

/test integration

Copy link
Member

@sakoush sakoush left a comment

Choose a reason for hiding this comment

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

you mentioned that there is a model version as well possibly, but I dont see how is that handled in what you have done so far.

Another point, do we have also model_name and model_version in REST payload? if not how are we handling then this case? in other words are we going to suffer from the same issue in REST?

Comment on lines +35 to +36
if model_name:
endpoint = f"{root_endpoint}/v2/models/{model_name}/infer"
Copy link
Member

Choose a reason for hiding this comment

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

you mentioned a version that we need to handle in the PR description, where is that handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, missed this comment.

Mmmh not sure what you mean. Was this on the PR description? Could you point out where?

@adriangonz
Copy link
Contributor Author

The failed integration tests seem to be known flaky ones,

  • TestRollingHttp.test_rolling_update10[ambas]
  • TestRollingHttp.test_rolling_update5[ambas]
  • test_label_update[1.5.0]

So this PR should be OK to go ahead 👍

@adriangonz
Copy link
Contributor Author

/test integration

1 similar comment
@adriangonz
Copy link
Contributor Author

/test integration

@adriangonz
Copy link
Contributor Author

/retest

@adriangonz adriangonz force-pushed the fix-grpc-transformers branch from cb33320 to 211597f Compare January 7, 2022 13:25
@adriangonz
Copy link
Contributor Author

/test integration

2 similar comments
@adriangonz
Copy link
Contributor Author

/test integration

@adriangonz
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

@adriangonz: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
integration 211597f link /test integration

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 jenkins-x/lighthouse repository. I understand the commands that are listed here.

@adriangonz
Copy link
Contributor Author

It seems that the failed integration tests are known flaky ones unrelated to the changes on this PR, so I think it should be good to go:

  • TestRollingHttp.test_rolling_update10[ambas]
  • TestRollingHttp.test_rolling_update5[ambas]
  • test_label_update[1.5.0]
  • test_namespace_update[1.7.0]

@adriangonz
Copy link
Contributor Author

Hey @sakoush ,

Apologies, just noticed your review comment now.

Not sure what you mean about the model version. Could you point out where was that mentioned?

When using REST, the model name and version are part of the endpoint, so not required to be on the payload as well. This is only required for gRPC, as per the V2 Protocol spec.

@adriangonz adriangonz requested a review from sakoush January 14, 2022 13:20
@sakoush
Copy link
Member

sakoush commented Jan 14, 2022

Ok thanks for clarification

@adriangonz adriangonz merged commit 2470b3d into SeldonIO:master Feb 3, 2022
@adriangonz adriangonz deleted the fix-grpc-transformers branch February 3, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update model_name field when chaining V2 gRPC payloads
3 participants