-
-
Notifications
You must be signed in to change notification settings - Fork 29
feat: add ownedBy and createdAt for OpenModel #438
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?
Conversation
will test locally soon |
cc8eca3
to
55df089
Compare
root@VM-0-3-ubuntu:/home/ubuntu/llmaz/docs/examples/ollama# kubectl get openmodel -oyaml
apiVersion: v1
items:
- apiVersion: llmaz.io/v1alpha1
kind: OpenModel
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"llmaz.io/v1alpha1","kind":"OpenModel","metadata":{"annotations":{},"name":"qwen2-0--5b"},"spec":{"familyName":"qwen2","source":{"uri":"ollama://qwen2:0.5b"}}}
creationTimestamp: "2025-06-03T13:30:42Z"
generation: 2
labels:
llmaz.io/model-family-name: qwen2
name: qwen2-0--5b
resourceVersion: "3586"
uid: 9df4445e-fbbb-4083-b3ff-4591dfc1f95e
spec:
createdAt: "2025-06-03T13:30:42Z"
familyName: qwen2
ownedBy: llmaz
source:
uri: ollama://qwen2:0.5b
kind: List
metadata:
resourceVersion: ""
root@VM-0-3-ubuntu:/home/ubuntu/llmaz/docs/examples/ollama# |
/kind feature |
@@ -73,3 +85,22 @@ func (r *ModelReconciler) SetupWithManager(mgr ctrl.Manager) error { | |||
For(&coreapi.OpenModel{}). | |||
Complete(r) | |||
} | |||
|
|||
// needsUpdated checks if the model needs to be updated. | |||
func needsUpdated(model *coreapi.OpenModel) (bool, *coreapi.OpenModel) { |
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.
Move them to the webhooks which I believe is the right place. And I think we don't need to set the OwnedBy since we have kubebuilder tag.
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.
sgtm. I forget that we have OpenModelWebhook... I will change this part
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.
Back to this again, if I understand correctly, when we put it in the webhook, it seems that model.CreationTimestamp is still empty when we create it. @kerthcet
🤔
func (w *OpenModelWebhook) Default(ctx context.Context, obj runtime.Object) error {
model := obj.(*coreapi.OpenModel)
if model.Labels == nil {
model.Labels = map[string]string{}
}
model.Labels[coreapi.ModelFamilyNameLabelKey] = string(model.Spec.FamilyName)
if model.Spec.CreatedAt == nil {
model.Spec.CreatedAt = &model.CreationTimestamp
}
return nil
}
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.
Yes, we should set it automatically.
Signed-off-by: googs1025 <[email protected]>
55df089
to
dcbc1e8
Compare
What this PR does / why we need it
Which issue(s) this PR fixes
Fixes #435
Special notes for your reviewer
Does this PR introduce a user-facing change?