Skip to content

feature(BackendRuntime): support lifecycle hook fields for BackendRuntime #303

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
Merged
InftyAI-Agent merged 2 commits into from
Mar 13, 2025
Merged

feature(BackendRuntime): support lifecycle hook fields for BackendRuntime #303

InftyAI-Agent merged 2 commits into from
Mar 13, 2025

Conversation

@googs1025
Copy link
Member

@googs1025 googs1025 commented Mar 10, 2025 •
edited
Loading

/kind feature

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #288 (comment)

Special notes for your reviewer

Does this PR introduce a user-facing change?

support lifecycle hook fields for BackendRuntime crd

@googs1025 googs1025 marked this pull request as draft March 10, 2025 12:23
@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Mar 10, 2025
@InftyAI-Agent InftyAI-Agent requested a review from kerthcet March 10, 2025 12:23
@googs1025
Copy link
Member Author

googs1025 commented Mar 10, 2025 •
edited
Loading

no sure if we need integration or e2e test to verify whether this function meets the expected

@googs1025 googs1025 force-pushed the main branch 2 times, most recently from 5b3133c to fae9d23 Compare March 10, 2025 13:19
@googs1025 googs1025 changed the title feature(BackendRuntime): support lifeCycle hook fields for BackendRuntime feature(BackendRuntime): support lifecycle hook fields for BackendRuntime Mar 10, 2025
@googs1025 googs1025 force-pushed the main branch 2 times, most recently from 3248956 to e91df2e Compare March 10, 2025 14:06
googs1025
googs1025 commented Mar 10, 2025
View reviewed changes
@googs1025 googs1025 force-pushed the main branch 10 times, most recently from aca4f30 to e6dde20 Compare March 11, 2025 02:56
@kerthcet
Copy link
Member

kerthcet commented Mar 11, 2025

no sure if we need integration or e2e test to verify whether this function meets the expected

one integration test makes sense to me.

@kerthcet
Copy link
Member

kerthcet commented Mar 11, 2025

E2e test is somehow too slow because we have to download the models. What we need to do is just make sure the lifecycle already injects, the capacity of lifecycle is not something we should touch it's the capacity of kubernetes, we always assume that it works as expected.

@googs1025 googs1025 force-pushed the main branch 2 times, most recently from 61f3462 to b8e9897 Compare March 11, 2025 06:56
@googs1025 googs1025 marked this pull request as ready for review March 11, 2025 06:59
@kerthcet
Copy link
Member

kerthcet commented Mar 11, 2025

/retest

@kerthcet
Copy link
Member

kerthcet commented Mar 11, 2025

The new e2e test is running in the background, but the failure check is still there, I may need time to figure out the reason.

@googs1025 googs1025 force-pushed the main branch 2 times, most recently from 13cf0a1 to 151d4bc Compare March 12, 2025 05:10
@googs1025
Copy link
Member Author

googs1025 commented Mar 12, 2025

/kind feature
/api-change

@InftyAI-Agent InftyAI-Agent added feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Mar 12, 2025
@kerthcet
Copy link
Member

kerthcet commented Mar 12, 2025

/kind api-change

@InftyAI-Agent InftyAI-Agent added the api-change Indicates PR includes api change. label Mar 12, 2025
kerthcet
kerthcet reviewed Mar 12, 2025
View reviewed changes
Copy link
Member

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

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

I think we need an integration test with the fakeBackend to prove the validation works.

googs1025
googs1025 commented Mar 12, 2025
View reviewed changes
kerthcet
kerthcet reviewed Mar 12, 2025
View reviewed changes
Copy link
Member

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

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

Generally LGTM. One nit.

test/integration/controller/inference/playground_test.go Outdated
},
},
}),
ginkgo.Entry("Playground with backend name configured", &testValidatingCase{
Copy link
Member

@kerthcet kerthcet Mar 12, 2025

Choose a reason for hiding this comment

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

Suggested change
ginkgo.Entry("Playground with backend name configured", &testValidatingCase{
ginkgo.Entry("Playground with fake-backend", &testValidatingCase{

Copy link
Member Author

@googs1025 googs1025 Mar 12, 2025

Choose a reason for hiding this comment

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

done

@kerthcet
Copy link
Member

kerthcet commented Mar 12, 2025

Kindly remind: no longer need to squash the commits, we'll merge with the PR title, one commit per PR.

kerthcet
kerthcet reviewed Mar 13, 2025
View reviewed changes
Copy link
Member

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

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

It's a mistake I think.

test/integration/controller/inference/playground_test.go Outdated
},
},
}),
ginkgo.Entry("Playground with Playground with fake-backend", &testValidatingCase{
Copy link
Member

@kerthcet kerthcet Mar 13, 2025

Choose a reason for hiding this comment

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

Suggested change
ginkgo.Entry("Playground with Playground with fake-backend", &testValidatingCase{
ginkgo.Entry("Playground with fake-backend", &testValidatingCase{

Copy link
Member Author

@googs1025 googs1025 Mar 13, 2025

Choose a reason for hiding this comment

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

opos.... my bad

@kerthcet
Copy link
Member

kerthcet commented Mar 13, 2025

/lgtm
/approve

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 13, 2025
@InftyAI-Agent InftyAI-Agent merged commit 4f9d28c into InftyAI:main Mar 13, 2025
19 checks passed
@kerthcet
Copy link
Member

kerthcet commented Mar 13, 2025

Next we need to add the configurations for the in-tree backendRuntimes, for example: vLLM

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

Reviewers

@kerthcet kerthcet kerthcet left review comments

Assignees

@kerthcet kerthcet

Labels

api-change Indicates PR includes api change. approved Indicates a PR has been approved by an approver from all required OWNERS files. feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PreStop command injection for inference engines

3 participants

@googs1025 @kerthcet @InftyAI-Agent