-
Notifications
You must be signed in to change notification settings - Fork 59
Implement ModelMesh support in ISVC handling and shift to python Black formatting #129
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
Conversation
Signed-off-by: Harshit Nayan <[email protected]>
Signed-off-by: Harshit Nayan <[email protected]>
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.
Just one question otherwise great work!
backend/apps/common/utils.py
Outdated
# Try to find ModelMesh deployment (usually named modelmesh-serving) | ||
try: | ||
modelmesh_deployment = api.get_custom_rsrc( | ||
**versions.K8S_DEPLOYMENT, | ||
namespace=namespace, | ||
name="modelmesh-serving" | ||
) |
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.
Are you sure it's safe to assume this deployment is always named modelmesh-serving
?
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.
You're correct to point it out. I did some digging and modelmesh-serving is named as per the template at: https://github.com/kserve/modelmesh-serving/blob/05994465603e8baa3d23bae126a085f5f8bb591f/config/internal/base/deployment.yaml.tmpl#L17
name: {{.ServiceName}}-{{.Name}}
labels:
modelmesh-service: {{.ServiceName}}
…sh resources using label selectors Signed-off-by: Harshit Nayan <[email protected]>
Signed-off-by: Harshit Nayan <[email protected]>
…and efficiency Signed-off-by: Harshit Nayan <[email protected]>
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.
Much more readable now just a couple comments that I think you can adjust to make this even simpler and we should be good if it still works for you
backend/apps/common/utils.py
Outdated
# Fallback: infer from model format (e.g., "sklearn" -> "mlserver-sklearn") | ||
if ( | ||
model_format := component_spec.get("model", {}) | ||
.get("modelFormat", {}) | ||
.get("name") | ||
): | ||
return f"mlserver-{model_format.lower()}" |
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.
I'm not sure you really need this. If an inference service doesn't specify the runtime it'll never work 🤷
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.
Yup removed this part and it works just fine.
backend/apps/common/utils.py
Outdated
""" | ||
default_name = "modelmesh-serving" | ||
# A configurable environment variable is more practical than discovery | ||
service_name = os.environ.get("MODELMESH_SERVICE_NAME", default_name) |
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.
so the user is supposed to set this environment variable? Is there no other way to get the modelmesh service? It's probably ok to assume the user didn't change the name of the modelmesh service IMO
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.
the default namespace for modelmesh is set here: https://github.com/kserve/modelmesh-serving/blob/05994465603e8baa3d23bae126a085f5f8bb591f/docs/quickstart.md?plain=1#L37
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.
I'll remove the env part then.
…e retrieval Signed-off-by: Harshit Nayan <[email protected]>
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.
Nice work!
Fixes: #97
Summary:
The changes allow users to view ModelMesh-specific resources and status for each component of an InferenceService.
Testing