feat(api): Split UserContainerLimitRequestFactor into two separate config values #367
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deadlycoconuts
force-pushed
the
split_user_container_limit_request_factor
branch
from
a70e3aa
to
a1de1a4
Compare
deadlycoconuts
force-pushed
the
split_user_container_limit_request_factor
branch
from
99b7793
to
8c71d73
Compare
deadlycoconuts
force-pushed
the
split_user_container_limit_request_factor
branch
from
8c71d73
to
8eb734c
Compare
deadlycoconuts
changed the title
deadlycoconuts
commented
Jan 17, 2024
deadlycoconuts
commented
Jan 17, 2024
Merged
6 tasks
deadlycoconuts
requested review from
ariefrahmansyah,
leonlnj and
tkpd-hafizhan
deadlycoconuts
force-pushed
the
split_user_container_limit_request_factor
branch
from
2cf892f
to
b08c568
Compare
|
Thanks for the review! Merging this now! 🚀 |
deadlycoconuts
added a commit
to caraml-dev/merlin
that referenced
this pull request
Jan 30, 2024
…er (#519) # Description In order to allow the resource limits of any pod deployed by the Merlin API server to be scaled automatically as a factor of the resource request value initially decided by the user, this PR introduces two new config values, `UserContainerCPULimitRequestFactor` and `UserContainerMemoryLimitRequestFactor`, which will be used to determine the resource limit when multiplied by the corresponding resource request value. In addition, this PR also allows the CPU resource limit values to be set as `0`, which would represent no resource limits being applied to pods deployed by the Merlin API server, when these new config values as set as `0`. **_This PR contains changes similar to what is implemented for Turing in caraml-dev/turing#367 🚨 **EDIT**: KServe automatically sets default resource request and limit values when they are not specified in the KService inference service specification. However, these default (e.g. CPU) values cannot be configured manually or removed (they are actually [hardcoded](https://github.com/kserve/kserve/blob/c254e704c3719d3310391ee994123d5b49588e5d/pkg/apis/serving/v1beta1/inference_service_defaults.go#L35) in the controller code itself), and as a (hopefully temporary) workaround to this, **this PR introduces another new configuration value** `UserContainerCPUDefaultLimit` **that specifies the default CPU limit of any pods deployed by the Merlin API server, when** `UserContainerCPULimitRequestFactor` **is set to** `0`. This ensures that KServe does not impose its own default CPU limit values on them. # Modifications - Introduction of the `UserContainerCPULimitRequestFactor` and `UserContainerMemoryLimitRequestFactor` config values - Introduction of conditional blocks to not apply CPU or memory limits if any of the corresponding values mentioned above are set as `0` # Tests <!-- Besides the existing / updated automated tests, what specific scenarios should be tested? Consider the backward compatibility of the changes, whether corner cases are covered, etc. Please describe the tests and check the ones that have been completed. Eg: - [x] Deploying new and existing standard models - [ ] Deploying PyFunc models --> - Ensure existing models redeployed have their resource limit values correctly set as a factor of their resource request values # Checklist - [x] Added PR label - [x] Added unit test, integration, and/or e2e tests - [ ] Tested locally - [ ] Updated documentation - [ ] Update Swagger spec if the PR introduce API changes - [ ] Regenerated Golang and Python client if the PR introduces API changes # Release Notes ```release-note NONE ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
PR #146 introduced a new config value
UserContainerLimitRequestFactorthat allows the resource limits of any pod deployed by the Turing API server to be set as the resource request value decided by the user, multiplied by the value ofUserContainerLimitRequestFactor. However, this factor applies to both CPU and memory resources and do not offer much flexibility in adjusting the resource limit values.In particular, we would like to allow CPU resource limit values to be set as
0, which would represent no CPU limits being applied to pods deployed by the Turing API server. Doing so would require splitting theUserContainerLimitRequestFactorconfig value into two separate variables -UserContainerCPULimitRequestFactorandUserContainerMemoryLimitRequestFactor.These two values serve pretty much the same function as the original
UserContainerLimitRequestFactorconfig value, with the additional ability to, when set as0, not apply any CPU or memory resource limits to the pod that's being deployed.This PR contains changes similar to what is implemented for Merlin in caraml-dev/merlin#519.
Modifications
UserContainerLimitRequestFactorintoUserContainerCPULimitRequestFactorandUserContainerMemoryLimitRequestFactor0Minor Changes