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

feat: Add vModelId to PayloadProcessor Payload #123

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Conversation

njhill
Copy link
Member

@njhill njhill commented Nov 16, 2023

Motivation

Currently the payloads passed to PayloadProcessors only contain the modelId, which in the case of vModels will be a "resolved" modelId corresponding to a particular model revision (in particular this will be true when used in KServe modelmesh-serving). It would be useful to include the vModelId too.

Modifications

Add a vModelId field to the Payload class and correspondingly update built-in PayloadProcessor implementations where applicable.

It may be null if the request was directed at a concrete modelId rather than a vModelId.

Result

Both modelId and vModelId are available to PayloadProcessors

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

@njhill -- your code changes look good. Would it make sense though to add some unit tests (or modify some of the existing ones) to test what should (should not) happen when vModelId is (or is not) set?

@ckadner
Copy link
Member

ckadner commented Nov 17, 2023

FYI @RobGeada

@njhill
Copy link
Member Author

njhill commented Nov 21, 2023

@ckadner yes, ideally we should add such unit tests.

@ckadner ckadner requested a review from RobGeada November 22, 2023 21:39
ckadner
ckadner previously approved these changes Nov 22, 2023
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Changes look good. Should we wait for @tteofili and/or @RobGeada ?

@ckadner
Copy link
Member

ckadner commented Nov 22, 2023

@ckadner yes, ideally we should add such unit tests.

Do I remember you mentioned adding a unit test could be done using some existing helper functions without the overhead of setting up an extensive end-to-end test environment. If it could be done with reasonable effort, should we create an issue to track it where you could add some pointers?

Was it this? #112 (comment)

@kserve kserve deleted a comment from kserve-oss-bot Nov 22, 2023
@ckadner ckadner removed the request for review from tjohnson31415 November 23, 2023 00:38
Copy link
Contributor

@tteofili tteofili left a comment

Choose a reason for hiding this comment

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

LGTM

Motivation

Currently the payloads passed to PayloadProcessors only contain the modelId, which in the case of vModels will be a "resolved" modelId corresponding to a particular model revision (in particular this will be true when used in KServe modelmesh-serving). It would be useful to include the vModelId too.

Modifications

Add a vModelId field to the Payload class and correspondingly update built-in PayloadProcessor implementations where applicable.

It may be null if the request was directed at a concrete modelId rather than a vModelId.

Result

Both modelId and vModelId are available to PayloadProcessors

Signed-off-by: Nick Hill <[email protected]>
@ckadner
Copy link
Member

ckadner commented Nov 24, 2023

@njhill -- could you review the result of my merge conflict resolution after merging #120 ?

@ckadner ckadner assigned njhill and unassigned tteofili and ckadner Nov 24, 2023
@kserve-oss-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@ckadner ckadner added this to the v0.11.2 milestone Nov 25, 2023
@kserve kserve deleted a comment from kserve-oss-bot Nov 25, 2023
@ckadner
Copy link
Member

ckadner commented Nov 28, 2023

I see @njhill gave his +1 on the conflict resolution so I will merge this one

@ckadner ckadner added the lgtm label Nov 28, 2023
@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckadner, njhill, tteofili

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ckadner ckadner merged commit eaa2fde into main Nov 28, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants