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

Fix usage of custom ServingRuntimes #47

Closed
wants to merge 2 commits into from

Conversation

disrupted
Copy link
Contributor

Fixes #46

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: disrupted
To complete the pull request process, please assign kimwnasptd after the PR has been reviewed.
You can assign the PR to them by writing /assign @kimwnasptd 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

@harperjuanl
Copy link

harperjuanl commented Nov 30, 2022

hi @disrupted,

when I deployed the paddle inferecenservice with KServe, the kubeflow models-web-app had the display problem and had the error as following figure,

WechatIMG255

I think there is not the paddle servingruntime, and return None to cause the error. When I add the 'paddle' PredictType, patch and rebuild the models-web-app image, the problem can be fixed and the display become correctly, there is my changes,

https://github.com/harperjuanl/models-web-app

I also test your method, it does work too. I see it returns the predictor.model and remove the judgement. Does it have the problem that some frameworks are returned, which do not be supported by KServe?

@disrupted
Copy link
Contributor Author

disrupted commented Nov 30, 2022

hi @harperjuanl,

thanks for your feedback, as well as verifying the issue & potential fix. Perhaps you're right, that this could lead to some issues with other frameworks, however I thought that ServingRuntimes are always specific to KServe and not used in other scenarios.

Adding custom ServingRuntimes (such as paddle in your case) to the known list of predictor types seems like a reasonable approach as well. But this would be better if it could be done through the KServe configuration, rather than having to patch the code, as it is the case in your example.

@ghost ghost mentioned this pull request Oct 18, 2023
@juliusvonkohout
Copy link
Contributor

@disrupted Please rebase to master

Copy link

oss-prow-bot bot commented Jun 26, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: disrupted
Once this PR has been reviewed and has the lgtm label, please assign juliusvonkohout for approval by writing /assign @juliusvonkohout in a comment. For more information see:The Kubernetes Code Review Process.

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

@disrupted
Copy link
Contributor Author

@disrupted Please rebase to master

done

@juliusvonkohout
Copy link
Contributor

@disrupted you also have to sign the DCO to pass all tests.

Signed-off-by: Salomon Popp <[email protected]>
@disrupted disrupted force-pushed the fix/serving-runtime-support branch from e551e42 to 3a8c824 Compare June 26, 2024 14:14
@disrupted
Copy link
Contributor Author

@disrupted you also have to sign the DCO to pass all tests.

done

Signed-off-by: Salomon Popp <[email protected]>
@disrupted
Copy link
Contributor Author

/assign @juliusvonkohout

@juliusvonkohout
Copy link
Contributor

@disrupted the frontend test is not passing. Maybe it is broken.

@disrupted
Copy link
Contributor Author

@disrupted the frontend test is not passing. Maybe it is broken.

it appears to me that the error is unrelated to the changes made in this PR

@juliusvonkohout
Copy link
Contributor

@disrupted the frontend test is not passing. Maybe it is broken.

it appears to me that the error is unrelated to the changes made in this PR

You might be right, but we still have to fix the tests either way.

@juliusvonkohout
Copy link
Contributor

merged in #92

@disrupted disrupted deleted the fix/serving-runtime-support branch September 3, 2024 13:10
This was referenced Sep 14, 2024
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.

Usage of custom ClusterServingRuntimes breaks web app
4 participants