-
Notifications
You must be signed in to change notification settings - Fork 675
fix: Extend add_tensor_model so that ModelDeploymentCard can be correctly picked up #4169
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
base: main
Are you sure you want to change the base?
fix: Extend add_tensor_model so that ModelDeploymentCard can be correctly picked up #4169
Conversation
Signed-off-by: zhongdaor <[email protected]>
Signed-off-by: zhongdaor <[email protected]>
Signed-off-by: zhongdaor <[email protected]>
WalkthroughThe KServe gRPC Rust bindings are extended to support optional runtime configuration for tensor models. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| runtime_config: Option<ModelRuntimeConfig>, | ||
| ) -> PyResult<()> { | ||
| // If runtime_config is provided, create and save a ModelDeploymentCard | ||
| // so the ModelConfig endpoint can return model configuration |
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.
Can you add or append to an existing test so that we can verify the ModelConfig endpoint works when runtime_config is provided?
| // Also remove the model card if it exists | ||
| // (It's ok if it doesn't exist since runtime_config is optional, we just ignore the None return) | ||
| let _ = self.inner.model_manager().remove_model_card(&model); |
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.
In dynamo, when a non-tensor model is unloaded, the MDC persists in etcd. From my understanding, with these changes, tensor model MDCs are explicitly removed from etcd. Does this create inconsistent cleanup behavior between model types?
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.
In dynamo, when a non-tensor model is unloaded, the MDC persists in etcd.
@grahamking changed this behavior in the past month or two, now MDCs are associated with model instances and should get cleaned up when the instance goes away.
Signed-off-by: zhongdaor <[email protected]>
Signed-off-by: zhongdaor <[email protected]>
Overview:
Extend add_tensor_model so that ModelDeploymentCard can be correctly picked up
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Bug Fixes