[v0.1 API Review] Grammatical fixes and TypedCondition creation/defaulting #186

kfswain · 2025-01-10T18:15:55Z

This addresses comments:

3f971ca#r1907775587

3f971ca#r1907785698

3f971ca#r1907790075

and other smaller ones (can make the list exhaustive if needed but they are mostly grammatical or more detailed wording

k8s-ci-robot · 2025-01-10T18:16:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kfswain

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kfswain]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kfswain · 2025-01-10T18:16:16Z

CC: @candita

api/v1alpha1/inferencemodel_types.go

api/v1alpha1/inferencepool_types.go

danehans · 2025-01-13T21:05:40Z

xref #190 for introducing the ResolvedRefs status condition.

danehans · 2025-01-13T21:12:49Z

api/v1alpha1/inferencemodel_types.go

+	// This condition indicates if the model is ready to accept traffic, and if not, why.
+	//
+	// Possible reasons for this condition to be True are:
+	//
+	// * "Ready"
+	//
+	// Possible reasons for this condition to be False are:
+	//
+	// * "ModelNameInUse"
+	//
+	// Possible reasons for this condition to be Unknown are:
+	//
+	// * "Pending"
+	//
+	ModelConditionReady InferenceModelConditionType = "Ready"


The Gateway API community had some learning lessons with the Ready condition (xref). From the godocs:

// If used in the future, "Ready" will represent the final state where all configuration is confirmed good
// and has completely propagated to the data plane. That is, it is a guarantee that, as soon as something
// sees the Condition as true, then connections will be correctly routed immediately.

Should we start with the Programmed condition and reserve Ready until we can guarantee a model server is ready to accept connections when Ready: True?

+1, agree on reusing Programmed or Accepted here

+1; I actually think we should start with Accepted since ModelNameInUse is more about that the InferenceModel is not accepted, and less about the inability to program it.

Agreed with all comments. Accepted seems the most appropriate for the given intent

ahg-g · 2025-01-15T02:31:35Z

api/v1alpha1/inferencemodel_types.go

+	// Details about naming conflict resolution are on the ModelName field itself.
+	ModelReasonNameInUse InferenceModelConditionReason = "ModelNameInUse"
+
+	// This reason is the initial state, and indicates that the controller has not yet reconciled the InferenceModel.


Do we expect that an InferenceModel status to revert back to Pending from Ready? or is that left to the implementation?

I doubt it would ever go from Accepted back to Pending. But leaving it up to the implementation is fine with me

ahg-g · 2025-01-15T03:40:20Z

api/v1alpha1/inferencemodel_types.go

+	// This condition indicates if the model is ready to accept traffic, and if not, why.
+	//
+	// Possible reasons for this condition to be True are:
+	//
+	// * "Ready"
+	//
+	// Possible reasons for this condition to be False are:
+	//
+	// * "ModelNameInUse"
+	//
+	// Possible reasons for this condition to be Unknown are:
+	//
+	// * "Pending"
+	//
+	ModelConditionReady InferenceModelConditionType = "Ready"


+1; I actually think we should start with Accepted since ModelNameInUse is more about that the InferenceModel is not accepted, and less about the inability to program it.

robscott

Thanks @kfswain !

robscott · 2025-01-14T21:09:50Z

api/v1alpha1/inferencemodel_types.go

+	// This condition indicates if the model is ready to accept traffic, and if not, why.
+	//
+	// Possible reasons for this condition to be True are:
+	//
+	// * "Ready"
+	//
+	// Possible reasons for this condition to be False are:
+	//
+	// * "ModelNameInUse"
+	//
+	// Possible reasons for this condition to be Unknown are:
+	//
+	// * "Pending"
+	//
+	ModelConditionReady InferenceModelConditionType = "Ready"


+1, agree on reusing Programmed or Accepted here

api/v1alpha1/inferencemodel_types.go

robscott

Thanks @kfswain !

netlify · 2025-01-15T21:06:24Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`4fb9e22`
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/6788343a67fbc800086cbf1d
😎 Deploy Preview	https://deploy-preview-186--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ahg-g · 2025-01-15T22:08:26Z

api/v1alpha1/inferencemodel_types.go

+	//
+	// * "Pending"
+	//
+	ModelConditionReady InferenceModelConditionType = "Accepted"


update variable names too

ahg-g · 2025-01-15T22:40:15Z

/lgtm

/hold

holding in case others have more suggestions

ahg-g · 2025-01-16T21:33:36Z

/hold cancel

…lting (kubernetes-sigs#186) * Feedback updates + code gen * Adding explanations for condition reasons * typo fix * Updating Condition Type * generated manifests * var name update

k8s-ci-robot requested review from ahg-g and liu-cong January 10, 2025 18:16

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2025

kfswain mentioned this pull request Jan 10, 2025

v0.1 API Review #154

Closed

candita reviewed Jan 10, 2025

View reviewed changes

api/v1alpha1/inferencemodel_types.go Outdated Show resolved Hide resolved

candita reviewed Jan 10, 2025

View reviewed changes

api/v1alpha1/inferencepool_types.go Outdated Show resolved Hide resolved

candita reviewed Jan 10, 2025

View reviewed changes

api/v1alpha1/inferencepool_types.go Show resolved Hide resolved

danehans reviewed Jan 13, 2025

View reviewed changes

ahg-g mentioned this pull request Jan 14, 2025

Tracking issue for release 0.1.0 #73

Closed

ahg-g reviewed Jan 15, 2025

View reviewed changes

robscott reviewed Jan 15, 2025

View reviewed changes

kfswain added 4 commits January 15, 2025 21:06

Feedback updates + code gen

050d707

Adding explanations for condition reasons

a9f18ce

typo fix

5fab019

Updating Condition Type

9942311

kfswain force-pushed the api-feedback-1 branch from ea47745 to 9942311 Compare January 15, 2025 21:06

generated manifests

34bd2b2

ahg-g reviewed Jan 15, 2025

View reviewed changes

var name update

4fb9e22

k8s-ci-robot assigned ahg-g Jan 15, 2025

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 15, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2025

k8s-ci-robot merged commit 6a4b025 into kubernetes-sigs:main Jan 16, 2025
8 checks passed

kfswain deleted the api-feedback-1 branch January 23, 2025 21:04

danehans added this to the v0.1.0-rc.1 milestone Jan 30, 2025

[v0.1 API Review] Grammatical fixes and TypedCondition creation/defaulting #186

[v0.1 API Review] Grammatical fixes and TypedCondition creation/defaulting #186

Uh oh!

Conversation

kfswain commented Jan 10, 2025

Uh oh!

k8s-ci-robot commented Jan 10, 2025

Uh oh!

kfswain commented Jan 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danehans commented Jan 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

netlify bot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for gateway-api-inference-extension ready!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahg-g commented Jan 15, 2025

Uh oh!

ahg-g commented Jan 16, 2025

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Jan 15, 2025 •

edited

Loading