Skip to content

Conversation

markurtz
Copy link
Collaborator

Summary

This PR introduces the new concept of constraints and request timings for the scheduler refactor while reworking the base objects and strategies needed to work with the refactor. The implementations are setup now so that Strategy classes return request timings instances which are then used with in the worker processes; the core reason for this is to ensure scheduled request times are calculated per request rather than precalculated which disables any ability to dynamically adjust rates. Additionally, the constraints are added in which are now used to control when the scheduler stops running rather than the hardcoded implementations for number of requests and duration. These will enable much more complicated implementations as seen in the added error constraints.

Details

  • Added constraints.py module with the new implementations and constructs including Protocols for constraint and initializer implementations. Additionally added constraints for number of requests, benchmark duration, total number of errors, error rate (both global and window)
  • Refactored result.py and types.py into the new objects.py and expanded their capabilities. The scheduler package itself is now fully generic and not tied to the backend package enabling more flexibility. Additionally, object types have been expanded out to include more details for benchmarking.
  • Refactored strategy.py to introduce the concept of request timings and reimplemented the strategy types to work with the new pathways.

Test Plan

  • Execute new utility module test suites covering smoke, sanity, and regression scenarios

Related Issues


  • [X ] "I certify that all code in this PR is my own, except as noted below."

Use of AI

  • [ X] Includes AI-assisted code completion
  • [ X] Includes code generated by an AI application
  • Includes AI-generated tests (NOTE: AI written tests should have a docstring that includes ## WRITTEN BY AI ##)

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a comprehensive refactor of the scheduler system, implementing constraints-based control and separating request timing logic from strategy implementations. The refactor enables more dynamic rate adjustments and sophisticated termination conditions while improving code modularity and testability.

  • Introduces constraint system with Protocol-based interfaces for controlling when the scheduler stops
  • Replaces fixed timing with request timings instances that calculate per-request rather than precalculated schedules
  • Refactors base objects to be fully generic and independent of backend package constraints

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/scheduler/test_strategy.py Comprehensive test suite for new strategy and timing implementations
tests/unit/scheduler/test_objects.py Test coverage for new object model including request info and scheduler state
tests/unit/scheduler/test_constraints.py Test suite for new constraint system and factory pattern
src/guidellm/scheduler/strategy.py Complete rewrite with timing abstractions and updated strategy implementations
src/guidellm/scheduler/objects.py New object model replacing result.py and types.py with enhanced capabilities
src/guidellm/scheduler/types.py Removed - functionality moved to objects.py
src/guidellm/scheduler/result.py Removed - functionality moved to objects.py

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@markurtz markurtz force-pushed the feature/refactor/utils-concurrency branch from 1ecc54a to cff8d91 Compare August 26, 2025 19:52
@markurtz markurtz force-pushed the feature/refactor/scheduler-objects branch from 6566035 to 1af3a45 Compare August 26, 2025 19:59
Copy link
Collaborator

@sjmonson sjmonson left a comment

Choose a reason for hiding this comment

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

We have been over this section quite a few times before so looks good aside from one minor nit. I did not check the constraint calculations to closely @AlonKellner-RedHat if you could quickly run over them.

Copy link
Collaborator

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few minor comments.

start_time = time.time()
if self.startup_duration > 0:
# Vary offset by up to 5% of the startup duration for a bit of variance
offset = 0.05 * self.startup_duration * (local_rank / local_world_size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this hard-coded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we'll need to rework this logic in the future, but there's no way to enable it currently, so I'd say let's revisit it after this goes out

@markurtz markurtz force-pushed the feature/refactor/scheduler-objects branch from 38aff11 to 0f011c3 Compare August 27, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants