Modify simple-limit strategy to be more intuitive - fixes #413 #450
Conversation
WalkthroughThe changes enhance the documentation and descriptions for the KRR algorithm's resource recommendation strategies. The README now details both the "simple" and "simple-limit" strategies, clarifying their behavior. In the code, field descriptions and default values for CPU percentiles are updated for both strategies, with expanded explanatory text in their descriptions. Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
robusta_krr/strategies/simple.py (1)
95-101: Fix the typo and CLI command naming to avoid confusion
Typo – missing “t”:
If you want to se CPU limits→If you want to set CPU limits.CLI name mismatch – here you recommend
krr simple-limit, but the actual
display_namefor the new strategy issimple_limit(underscore).
Having two spellings in docs/user-facing strings will trip users and break copy-paste examples.- This is the default strategy. If you want to se CPU limits, use 'krr simple-limit' instead. + This is the default strategy. If you want to set CPU limits, use 'krr simple_limit' instead.robusta_krr/strategies/simple_limit.py (1)
100-103: Minor wording tweak“se CPU limits” → “set CPU limits”.
- Unlike the 'simple' strategy, this strategy sets explicit CPU limits instead of leaving them unset. - Use this when you want to prevent CPU bursting beyond the configured limit. + Unlike the 'simple' strategy, this strategy sets explicit CPU limits instead of leaving them unset. + Use this when you want to prevent CPU bursting beyond the configured limit.(only the missing “t” needs fixing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)robusta_krr/strategies/simple.py(2 hunks)robusta_krr/strategies/simple_limit.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
robusta_krr/strategies/simple_limit.py (1)
robusta_krr/strategies/simple.py (1)
description(93-113)
robusta_krr/strategies/simple.py (2)
robusta_krr/strategies/simple_limit.py (1)
description(94-115)robusta_krr/core/abstract/strategies.py (1)
description(117-122)
🔇 Additional comments (1)
README.md (1)
452-458: Ensure strategy name is consistent across docs and codeThroughout the README you use the CLI form
krr simple_limit, but in
simple.pythe hint string refers tokrr simple-limit(hyphen). Pick one
(spoiler: the code registerssimple_limit) and keep it consistent in all
examples to avoid user errors.
| cpu_request: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU request.") | ||
| cpu_limit: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU limit. Unlike 'simple' strategy, this strategy sets explicit CPU limits.") | ||
| memory_buffer_percentage: float = pd.Field( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation so cpu_limit is not lower than cpu_request
With both defaults set to 95, users might pass values such that
cpu_limit < cpu_request, which is an invalid Kubernetes spec and will fail
server-side. A simple Pydantic validator prevents misconfiguration:
class SimpleLimitStrategySettings(StrategySettings):
@@
cpu_request: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU request.")
cpu_limit: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU limit. Unlike 'simple' strategy, this strategy sets explicit CPU limits.")
+
+ @pd.validator("cpu_limit")
+ def _limit_not_lower_than_request(cls, v, values):
+ req = values.get("cpu_request")
+ if req is not None and v < req:
+ raise ValueError("cpu_limit must be greater than or equal to cpu_request")
+ return v📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cpu_request: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU request.") | |
| cpu_limit: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU limit. Unlike 'simple' strategy, this strategy sets explicit CPU limits.") | |
| memory_buffer_percentage: float = pd.Field( | |
| class SimpleLimitStrategySettings(StrategySettings): | |
| cpu_request: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU request.") | |
| cpu_limit: float = pd.Field(95, gt=0, le=100, description="The percentile to use for the CPU limit. Unlike 'simple' strategy, this strategy sets explicit CPU limits.") | |
| @pd.validator("cpu_limit") | |
| def _limit_not_lower_than_request(cls, v, values): | |
| req = values.get("cpu_request") | |
| if req is not None and v < req: | |
| raise ValueError("cpu_limit must be greater than or equal to cpu_request") | |
| return v | |
| memory_buffer_percentage: float = pd.Field( |
🤖 Prompt for AI Agents
In robusta_krr/strategies/simple_limit.py around lines 28 to 30, add a Pydantic
validator to ensure that cpu_limit is never set lower than cpu_request.
Implement a method decorated with @pd.validator that compares cpu_limit and
cpu_request values and raises a validation error if cpu_limit is less than
cpu_request. This prevents invalid Kubernetes specs and user misconfiguration.
This is a breaking change for users of the simple-limit strategy (non-default strategy), but I believe its worth it, as it removes very confusing default behaviour. See #413