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

Changes InferencePool EPP Flags #152

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Changes InferencePool EPP Flags #152

merged 1 commit into from
Jan 7, 2025

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Jan 6, 2025

Changes the EPP CLI flags and internal API fields related to the InferencePool ns/name that the EPP belongs to.

Fixes #150

@danehans
Changes InferencePool EPP Flags
2d3a884 
Signed-off-by: Daneyon Hansen <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 6, 2025
@k8s-ci-robot k8s-ci-robot requested review from ahg-g and liu-cong January 6, 2025 18:08
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 6, 2025
liu-cong
liu-cong reviewed Jan 6, 2025
View reviewed changes
Copy link
Contributor

@liu-cong liu-cong left a comment

Choose a reason for hiding this comment

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

Just a few nits. Thanks!

pkg/manifests/ext_proc.yaml
@@ -51,7 +51,7 @@ spec:
# TODO(https://github.com/kubernetes-sigs/llm-instance-gateway/issues/34) Update the image and args.
image: us-central1-docker.pkg.dev/k8s-staging-images/llm-instance-gateway/epp:main
args:
- -serverPoolName
- -poolName
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- -poolName
- -inferencePoolName

pkg/ext-proc/backend/endpointslice_reconciler.go
Datastore *K8sDatastore
Scheme *runtime.Scheme
Record record.EventRecorder
PoolName string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PoolName string
InferencePoolName string

pkg/ext-proc/main.go
serverPoolName = flag.String(
"serverPoolName",
poolName = flag.String(
"poolName",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"poolName",
"inferencePoolName",

pkg/ext-proc/main.go
"Name of the serverPool this Endpoint Picker is associated with.")
"Name of the InferencePool this Endpoint Picker is associated with.")
poolNamespace = flag.String(
"poolNamespace",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"poolNamespace",
"inferencePoolNamespace",

@danehans
Copy link
Contributor Author

danehans commented Jan 6, 2025

@liu-cong thanks for the review. I decided on poolNamespace and poolName as CLI flags since inferencePoolName and inferencePoolNamespace are quite verbose. The description for each flag explains that poolX means InferencePool. I'm happy to use the more verbose flags if others agree with @liu-cong's review comments.

@kfswain
Copy link
Collaborator

kfswain commented Jan 6, 2025

@liu-cong thanks for the review. I decided on poolNamespace and poolName as CLI flags since inferencePoolName and inferencePoolNamespace are quite verbose. The description for each flag explains that poolX means InferencePool. I'm happy to use the more verbose flags if others agree with @liu-cong's review comments.

I agree with you on poolNamespace and poolName

@liu-cong
Copy link
Contributor

liu-cong commented Jan 7, 2025

/lgtm

/approve

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

ahg-g commented Jan 7, 2025

/lgtm
/approve

Thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2025
@k8s-ci-robot k8s-ci-robot merged commit 0985c9d into kubernetes-sigs:main Jan 7, 2025
4 checks passed
courageJ pushed a commit to courageJ/llm-instance-gateway that referenced this pull request Jan 8, 2025
@danehans @courageJ
Changes InferencePool EPP Flags (kubernetes-sigs#152)
774fb62 
Signed-off-by: Daneyon Hansen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liu-cong liu-cong liu-cong left review comments

@ahg-g ahg-g Awaiting requested review from ahg-g

Assignees

@liu-cong liu-cong

@ahg-g ahg-g

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update serverPoolName Flag Extension to Align With API Changes
5 participants
@danehans @kfswain @liu-cong @ahg-g @k8s-ci-robot