-
Notifications
You must be signed in to change notification settings - Fork 149
Add a min-requests constraint #700
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -268,6 +268,117 @@ def _validate_max_duration( | |
| return value[0] if isinstance(value, list) and len(value) == 1 else value | ||
|
|
||
|
|
||
| @ConstraintsInitializerFactory.register( # type: ignore[arg-type] | ||
| ["min_number", "min_num", "min_requests", "min_req"] | ||
| ) | ||
| class MinNumberConstraint(PydanticConstraintInitializer): | ||
| """ | ||
| Constraint that limits execution based on minimum request counts. | ||
|
|
||
| Like MinNumberConstraint but instead of stopping request generation after reaching | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mistake: It should say "Like MaxNumberConstraint" I think this wording doesn't emphasize the nuances of this implementation enough. Maybe clarify generation and processing, and why this may be helpful. It's identical except that it doesn't stop queueing until the max processed quantity is reached. |
||
| a minimum, it ends the benchmark after completing a minimum number of requests. | ||
| """ | ||
|
|
||
| type_: Literal["min_number"] = "min_number" # type: ignore[assignment] | ||
| min_num: int | float | list[int | float] = Field( | ||
| description="Minimum number of requests allowed before triggering constraint", | ||
| ) | ||
| current_index: int = Field( | ||
| default=-1, description="Current index for list-based min_num values" | ||
| ) | ||
|
|
||
| @classmethod | ||
| def validated_kwargs( | ||
| cls, min_num: int | float | list[int | float], **kwargs | ||
| ) -> dict[str, Any]: | ||
| """ | ||
| Validate and process arguments for MinNumberConstraint creation. | ||
|
|
||
| :param min_num: Minimum number of requests to allow | ||
| :param kwargs: Supports min_num, min_number, min_requests, min_req, | ||
| and optional type_ | ||
| :return: Validated dictionary with min_num and type_ fields | ||
| """ | ||
| aliases = ["min_number", "min_num", "min_requests", "min_req"] | ||
| for alias in aliases: | ||
| if min_num is None: | ||
| min_num = kwargs.get(alias) | ||
|
|
||
| return {"min_num": min_num, "current_index": kwargs.get("current_index", -1)} | ||
|
|
||
| def create_constraint(self, **_kwargs) -> Constraint: | ||
| """ | ||
| Return self as the constraint instance. | ||
|
|
||
| :param kwargs: Additional keyword arguments (unused) | ||
| :return: Self instance as the constraint | ||
| """ | ||
| self.current_index += 1 | ||
|
|
||
| return cast("Constraint", self.model_copy()) | ||
|
|
||
| def __call__( | ||
| self, state: SchedulerState, request_info: RequestInfo | ||
| ) -> SchedulerUpdateAction: | ||
| """ | ||
| Evaluate constraint against current scheduler state and request count. | ||
|
|
||
| :param state: Current scheduler state with request counts | ||
| :param request_info: Individual request information (unused) | ||
| :return: Action indicating whether to continue or stop operations | ||
| """ | ||
| _ = request_info # Unused parameters | ||
| current_index = max(0, self.current_index) | ||
| min_num = ( | ||
| self.min_num | ||
| if isinstance(self.min_num, int | float) | ||
| else self.min_num[max(current_index, len(self.min_num) - 1)] | ||
| ) | ||
|
|
||
| processed_exceeded = state.processed_requests >= min_num | ||
| remaining_requests = min(max(0, min_num - state.processed_requests), min_num) | ||
| stop_time = ( | ||
| None if remaining_requests > 0 else request_info.completed_at or time.time() | ||
| ) | ||
|
|
||
| return SchedulerUpdateAction( | ||
| request_queuing="stop" if processed_exceeded else "continue", | ||
| request_processing="stop_local" if processed_exceeded else "continue", | ||
| metadata={ | ||
| "min_number": min_num, | ||
| "processed_exceeded": processed_exceeded, | ||
| "created_requests": state.created_requests, | ||
| "processed_requests": state.processed_requests, | ||
| "remaining_requests": remaining_requests, | ||
| "stop_time": stop_time, | ||
| }, | ||
| progress=SchedulerProgress( | ||
| remaining_requests=remaining_requests, | ||
| total_requests=min_num, | ||
| stop_time=stop_time, | ||
| ), | ||
| ) | ||
|
|
||
| @field_validator("min_num") | ||
| @classmethod | ||
| def _validate_min_num( | ||
| cls, value: int | float | list[int | float] | ||
| ) -> int | float | list[int | float]: | ||
| if not isinstance(value, list): | ||
| value = [value] | ||
| for val in value: | ||
| if not val: | ||
| raise ValueError( | ||
| f"min_num must be set and truthful, received {value} ({val} failed)" | ||
| ) | ||
| if not isinstance(val, int | float) or val <= 0: | ||
| raise ValueError( | ||
| f"min_num must be a positive num, received {value} ({val} failed)" | ||
| ) | ||
|
|
||
| return value[0] if isinstance(value, list) and len(value) == 1 else value | ||
|
|
||
|
|
||
| class RequestsExhaustedConstraint(StandardBaseModel, InfoMixin): | ||
| type_: Literal["requests_exhausted"] = "requests_exhausted" # type: ignore[assignment] | ||
| num_requests: int | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| """Tests for scheduler constraints.""" |
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.
It may make sense to instead rename this to max-processed. I think this would be less confusing. But I can see the argument for
min, since it's going to keep scheduling past that until the max-processed is reached. So I'm not sure what should be done.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.
Yeah...
max-processedis both a little too vague and also incorrect since we can end up processing more requests then set. I think min is fine actually. I'll just add some notes to the docs that clarify constraints are OR not AND. Maybe in the future we can support AND constraint combinations.