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.

Sign up for GitHub

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

Jump to bottom

InferencePool config proposal for API review #162

Merged
merged 9 commits into from
Feb 1, 2025
Merged

InferencePool config proposal for API review #162

merged 9 commits into from
Feb 1, 2025

Conversation

ahg-g
Copy link
Contributor

@ahg-g ahg-g commented Jan 7, 2025 •
edited
Loading

This proposes to expand the InferencePool API with configuration parameters required by gateway controllers to configure the gateway with the endpoint picker extension.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2025
@k8s-ci-robot 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 7, 2025
@kfswain
Copy link
Collaborator

kfswain commented Jan 7, 2025

This is intended to be post-v0.1, correct?

@ahg-g ahg-g mentioned this pull request Jan 7, 2025
Merged
@kfswain kfswain mentioned this pull request Jan 15, 2025
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2025
@danehans
Copy link
Contributor

@ahg-g when you have a moment, please provide an update on the status of this PR. From my understanding based on community feedback, the config API will be simplified to focus on the connection information required for the Gateway implementation to connect with the inference extension.

@danehans
Copy link
Contributor

xref for securing the gateway<>inference extension connection.

@ahg-g
Copy link
Contributor Author

ahg-g commented Jan 28, 2025

Right, I was thinking that we want to hold on to this until after the first release

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2025
@netlify Netlify
Copy link

netlify bot commented Jan 28, 2025 •
edited
Loading

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

Name Link
🔨 Latest commit c94b10e
🔍 Latest deploy log https://app.netlify.com/sites/gateway-api-inference-extension/deploys/679d72fe40d8ad0008a957cf
😎 Deploy Preview https://deploy-preview-162--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2025
@ahg-g
Copy link
Contributor Author

ahg-g commented Jan 28, 2025

@ahg-g when you have a moment, please provide an update on the status of this PR. From my understanding based on community feedback, the config API will be simplified to focus on the connection information required for the Gateway implementation to connect with the inference extension.

I made the updates, lets see if we can get this settled.

@ahg-g
Copy link
Contributor Author

ahg-g commented Jan 28, 2025

xref for securing the gateway<>inference extension connection.

Why is the config api relevant to this?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 28, 2025
@ahg-g ahg-g mentioned this pull request Jan 30, 2025
Closed
@ahg-g ahg-g force-pushed the config-api branch 2 times, most recently from 77fe502 to 02db190 Compare January 30, 2025 22:25
@ahg-g ahg-g force-pushed the config-api branch 3 times, most recently from 62f4366 to e946cda Compare January 31, 2025 06:25
danehans
danehans approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor nits that can be resolved in a follow-up PR so I'm tagging.

api/v1alpha1/inferencepool_types.go Outdated
// Extension configures an endpoint picker as an extension service.
//
// +optional
Extension *ExtensionConfig `json:"extension"`
Copy link
Contributor

@danehans danehans Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahg-g This field is still missing ,omitempty. Additionally, The +kubebuilder:validation:Required marker is intended to ensure that a field is always present in API requests so this field should not be a pointer.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, danehans

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:

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

robscott
robscott reviewed Feb 1, 2025
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple tiny nits, otherwise LGTM

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 1, 2025
@robscott
Copy link
Member

robscott commented Feb 1, 2025

Thanks @ahg-g!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2025
@ahg-g
Copy link
Contributor Author

ahg-g commented Feb 1, 2025

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2025
@k8s-ci-robot k8s-ci-robot merged commit d66b732 into kubernetes-sigs:main Feb 1, 2025
7 of 8 checks passed
@danehans
Copy link
Contributor

danehans commented Feb 1, 2025

@ahg-g thanks for working through all the review feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robscott robscott robscott left review comments

@danehans danehans danehans approved these changes

@terrytangyuan terrytangyuan terrytangyuan left review comments

@arkodg arkodg arkodg left review comments

@AndresGuedez AndresGuedez AndresGuedez left review comments

@kfswain kfswain Awaiting requested review from kfswain

@shaneutt shaneutt Awaiting requested review from shaneutt

Assignees

@robscott robscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.1.0-rc.1
Development

Successfully merging this pull request may close these issues.
9 participants
@ahg-g @kfswain @danehans @robscott @shaneutt @k8s-ci-robot @terrytangyuan @arkodg @AndresGuedez