-
Notifications
You must be signed in to change notification settings - Fork 179
moved main code to runner package under epp/cmd #956
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
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@JeffLuoo are you able to assist?
|
to just:
to see if it works? |
fyi, https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/951/files will cause a conflict |
I put a hold on 951 since the parameters should move to the config api anyway |
Signed-off-by: Nir Rozenbaum <[email protected]>
… layer Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
@JeffLuoo ok I think I caught the problem. it was a required line change in the Dockerfile, not related to CommitSHA. |
Have we validated this manually? Hitting CI issues with the build makes me think we need some stronger validation |
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.
@shmuelk this will cause a great conflict with your PR, need to figure out which one to get in first, probably this one since it is "simpler"
return r | ||
} | ||
|
||
func (r *Runner) Run() error { |
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.
Is this an exact copy of the logic that existed in main.go? if not, can you point out the diff?
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, exact copy with the only difference of initialization of the scheduler.
right. I took it into consideration and talked with Shmuel. obviously things may change here, but this simple change will allow us to get rid of all main related code duplication in llm-d. working iteratively, when his big PR merges we will do the adaptations. |
Are you able to run the e2e test? |
/approve I will get approve this to unblock the config api PR, it will trigger GKE's internal regression test, but please also try to run the e2e test. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, nirrozenbaum 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 |
@ahg-g sure, e2e-test pass. missed your question. |
@kfswain sure 👍🏻 |
* moved main code to runner package under epp/cmd Signed-off-by: Nir Rozenbaum <[email protected]> * added the ability to configure postResponse plugins in requestcontrol layer Signed-off-by: Nir Rozenbaum <[email protected]> * updated dockerfile Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
This PR lays the ground for having an extensible epp main with minimal changes.
the code in this PR moves the code in current
main.go
torunner
package underepp/cmd
.ALMOST NO CODE CHANGES were made except for:
main.go
, e.g., from llm-dmain.go
.This can be done via the
Runner.WithSchedulerConfig(...)
function orRunner.WithRequestControlConfig(...)
which is used to configure thePostResponse
plugins.That's it. NO OTHER CHANGES.
main.go
now looks like the following:** In this PR there is no intention to make improvements to main package, logging, or any other structuring issue than the one that is mentioned in the intro. This is only a first step out of a series of PRs that will be added to GIE to improve the way we initialize and run EPP in an extensible manner.
cc: @ahg-g @kfswain @elevran