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

Drop legacy Kserve input validation #219

Merged
merged 8 commits into from
Mar 9, 2023
Merged

Conversation

bolasim
Copy link
Collaborator

@bolasim bolasim commented Feb 14, 2023

What

Kserve expects that inputs with the inputs or instances keyword to match up to being either a list or a numpy array. However, since we have moved to allow truss users to specify the input type to their predict function in a very flexible way, this legacy kserve behavior is incompatible and gets in the user's way by returning opaque 400 errors.

This PR drops that input validation from kserve and passes the decoded input to the model's predict function directly. Any user errors are expected to return a 500 and to show the stacktrace in the logs of the service.

@pankajroark
Copy link
Collaborator

I think we also need to drop the part of validate function in model_wrapper, where we validate the input and raise InvalidInput error. We'd want to keep the assign_request_to_inputs_instances_after_validation for now, because that has backwards compatibility considerations. But that would mean the validate wouldn't be doing validation. I think it would be best to move assign_request_to_inputs_instances_after_validation somewhere else, perhaps to preprocess function there.

@bolasim bolasim marked this pull request as ready for review February 14, 2023 21:55
Copy link
Collaborator

@pankajroark pankajroark left a comment

Choose a reason for hiding this comment

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

lgtm, good to add tests for the case where previously we would have done input validation.

@bolasim
Copy link
Collaborator Author

bolasim commented Feb 14, 2023

I still worry about doing this on pre-process:

payload = assign_request_to_inputs_instances_after_validation(payload)

Do you think we can get away with only doing this for truss_spec: 1.0 which is supposed to respect the "inputs" keywork to be list as kserve used to expect it?

I think doing this for truss_spec: 2.0 will cause more confusion in case people are using "instances" and/or "inputs" in their request signature.

@bolasim bolasim merged commit 7d73c69 into main Mar 9, 2023
@bolasim bolasim deleted the bola/drop-input-validation branch March 9, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants