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

Proposal update for the API names and latency objective #91

Merged
merged 3 commits into from
Dec 11, 2024
Merged

Proposal update for the API names and latency objective #91

merged 3 commits into from
Dec 11, 2024

Conversation

ahg-g
Copy link
Contributor

@ahg-g ahg-g commented Dec 10, 2024

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 10, 2024
@k8s-ci-robot k8s-ci-robot added 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 Dec 10, 2024
@ahg-g
Copy link
Contributor Author

ahg-g commented Dec 11, 2024

/hold

just so it doesn't get merged by accident

@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 Dec 11, 2024
@ahg-g ahg-g changed the title Proposal update for the API names latency objective Proposal update for the API names and latency objective Dec 11, 2024
robscott
robscott reviewed Dec 11, 2024
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.

Thanks @ahg-g! Mostly LGTM, just a few nits

@kfswain
Copy link
Collaborator

kfswain commented Dec 11, 2024

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2024
liu-cong
liu-cong reviewed Dec 11, 2024
View reviewed changes
docs/proposals/002-api-proposal/proposal.md Outdated
@@ -28,7 +28,7 @@

## Summary

This proposal presents 2 new CRD objects to express the needs of the LLM Instance Gateway. **LLMServerPool** and **LLMService** (names up for debate). The LLMServerPool is the logical grouping of compute, owned by the Inference Platform Admin persona. While the LLMService defines the serving objectives of a specific model or LoRA adapter, and is owned by the LLM Service Owner.
This proposal presents 2 new CRD objects to express the needs of the LLM Instance Gateway. **InferencePool** and **InferenceModel**. The InferencePool is the logical grouping of compute, owned by the Inference Platform Admin persona. While the InferenceModel defines the serving objectives of a specific model or LoRA adapter, and is owned by the Inference Workload Owner.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still use "LLM Instance Gateway"? And it seems we didn't define "LLM Instance Gateway"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to match the new name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated throughput the proposal

docs/proposals/002-api-proposal/proposal.md
- Enforce any common set of adapters or base models are available on the Pods
- Manage Deployments of Pods within the Pool
- Manage Pod lifecycle of pods within the pool

Additionally, any Pod that seeks to join a LLMServerPool would need to support a protocol, defined by LLM Instance Gateway, to ensure the Pool has adequate information to intelligently route requests.
Additionally, any Pod that seeks to join an InferencePool would need to support a protocol, defined by this project, to ensure the Pool has adequate information to intelligently route requests.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's link the protocol doc here? https://docs.google.com/document/d/18VRJ2ufZmAwBZ2jArfvGjQGaWtsQtAP6_yF2Xn6zcms/edit?tab=t.0#heading=h.sw2xdf66jh6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should spec this protocol and have it in this repo. Can you submit a proposal based on the doc?

Copy link
Contributor

Choose a reason for hiding this comment

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

sg, will do

docs/proposals/002-api-proposal/proposal.md Outdated

![K8s Gateway with LLMServerPools](./images/gw_w_lsp.svg)
![K8s Gateway with InferencePools](./images/gw_w_lsp.svg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the diagram

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing with @kfswain, we decided to remove them.

docs/proposals/002-api-proposal/proposal.md Outdated
@@ -336,8 +318,8 @@ Our original idea was to define all LLMService config at the Kubernetes Gateway

- Reasonable defaults (how do we behave in the absence of user-specified values in optional fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the FAQs are pretty confusing or outdated (the SLO part) to me. Perhaps we can delete them, or add some more detail/example to explain them better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the open questions for now

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2024
ahg-g
ahg-g commented Dec 11, 2024
View reviewed changes
Copy link
Contributor Author

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

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

Thanks

docs/proposals/002-api-proposal/proposal.md Outdated
@@ -28,7 +28,7 @@

## Summary

This proposal presents 2 new CRD objects to express the needs of the LLM Instance Gateway. **LLMServerPool** and **LLMService** (names up for debate). The LLMServerPool is the logical grouping of compute, owned by the Inference Platform Admin persona. While the LLMService defines the serving objectives of a specific model or LoRA adapter, and is owned by the LLM Service Owner.
This proposal presents 2 new CRD objects to express the needs of the LLM Instance Gateway. **InferencePool** and **InferenceModel**. The InferencePool is the logical grouping of compute, owned by the Inference Platform Admin persona. While the InferenceModel defines the serving objectives of a specific model or LoRA adapter, and is owned by the Inference Workload Owner.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to match the new name.

docs/proposals/002-api-proposal/proposal.md
- Enforce any common set of adapters or base models are available on the Pods
- Manage Deployments of Pods within the Pool
- Manage Pod lifecycle of pods within the pool

Additionally, any Pod that seeks to join a LLMServerPool would need to support a protocol, defined by LLM Instance Gateway, to ensure the Pool has adequate information to intelligently route requests.
Additionally, any Pod that seeks to join an InferencePool would need to support a protocol, defined by this project, to ensure the Pool has adequate information to intelligently route requests.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should spec this protocol and have it in this repo. Can you submit a proposal based on the doc?

docs/proposals/002-api-proposal/proposal.md Outdated

A LLMService allows the LLM Service Owner to define:
An InferenceModel allows the Inference Workload Owner to define:
- Which LoRA adapter(s) to consume
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/proposals/002-api-proposal/proposal.md Outdated
@@ -28,7 +28,7 @@

## Summary

This proposal presents 2 new CRD objects to express the needs of the LLM Instance Gateway. **LLMServerPool** and **LLMService** (names up for debate). The LLMServerPool is the logical grouping of compute, owned by the Inference Platform Admin persona. While the LLMService defines the serving objectives of a specific model or LoRA adapter, and is owned by the LLM Service Owner.
This proposal presents 2 new CRD objects to express the needs of the LLM Instance Gateway. **InferencePool** and **InferenceModel**. The InferencePool is the logical grouping of compute, owned by the Inference Platform Admin persona. While the InferenceModel defines the serving objectives of a specific model or LoRA adapter, and is owned by the Inference Workload Owner.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated throughput the proposal

@liu-cong
Copy link
Contributor

Thanks for updating this!

/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 Dec 11, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, kfswain, 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

@ahg-g
Copy link
Contributor Author

ahg-g commented Dec 11, 2024

/label tide/merge-method-squash
/hold cancel

Thanks all for the great feedback

@k8s-ci-robot k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 11, 2024
@k8s-ci-robot k8s-ci-robot merged commit 28e8383 into kubernetes-sigs:main Dec 11, 2024
1 of 2 checks passed
courageJ pushed a commit to courageJ/llm-instance-gateway that referenced this pull request Dec 12, 2024
@ahg-g @courageJ
Proposal update for the API names and latency objective (kubernetes-s…
17915b3 
…igs#91)

* Update the API names and add criticality parameter instead of latency objective in the proposal.

* Addressed comments

* Addressing comments round 2
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

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

Assignees

@liu-cong liu-cong

@kfswain kfswain

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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ahg-g @kfswain @liu-cong @k8s-ci-robot @robscott