Skip to content

Conversation

bpkroth
Copy link
Contributor

@bpkroth bpkroth commented May 19, 2025

Pull Request

Title

Refactor Scheduler schema definitions to make it easier to add new ones.


Description

Simple refactor of the Scheduler schemas to allow making new ones as a copy/edit of the synchscheduler-subschema.json.


Type of Change

  • 🔄 Refactor

Testing

  • Exisiting CI checks for good and bad scheduler config files still pass.
  • Adding additional tests to actually load those configs with mlos_bench next.

Additional Notes (optional)


@Copilot Copilot AI review requested due to automatic review settings May 19, 2025 22:17
@bpkroth bpkroth requested a review from a team as a code owner May 19, 2025 22:17
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 refactors scheduler schema definitions by extracting common base config properties, introducing a dedicated subschema for the SyncScheduler, and updating the top-level scheduler schema to compose these subschemas.

  • Extracted shared scheduler config properties into base-scheduler-subschema.json
  • Added sync-scheduler-subschema.json for SyncScheduler-specific config
  • Refactored scheduler-schema.json to use allOf/oneOf composition instead of inline if/then

Reviewed Changes

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

File Description
mlos_bench/.../sync-scheduler-subschema.json New subschema defining SyncScheduler class & config
mlos_bench/.../scheduler-schema.json Top-level schema now composes base and sync subschemas
mlos_bench/.../base-scheduler-subschema.json New base subschema with common scheduler config defs
Comments suppressed due to low confidence (1)

mlos_bench/mlos_bench/config/schemas/schedulers/base-scheduler-subschema.json:1

  • The new base scheduler subschema isn’t yet covered by any validation tests. Consider adding unit tests that load and validate this schema to catch regressions early.
{

@motus motus merged commit 069452b into microsoft:main May 20, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants