Skip to content

Conversation

bpkroth
Copy link
Contributor

@bpkroth bpkroth commented May 9, 2025

Pull Request

Title

Rename and reorganize some Scheduler methods


Description

In preparation for ParallelScheduler PR (#971) this PR

  1. Renames a few methods for clarity (e.g., schedule_trial --> add_trial_to_queue) so that the role of other methods (e.g., assign_trial_runners) is more clear and readily overridable.
  2. Moves some methods to the base class.
    This is done based on an observation that some of the methods (e.g., the main start loop) are largely reusable across both SyncScheduler and ParallelScheduler if the method separation is done a little bit more fine grained such that each can override the only the parts they need to.

Additionall, it also

  1. Also adds a subtle tweak on the pending_trials method in order to filter based on whether or not the trial already has a runner assigned or not.
  2. Adjusts TrialRunner.run_trial to return the results of the Trial.
    This isn't really used anywhere, but it is helpful to check that TrialRunners run in a child process finished successfully by using that result as a check in the return value, even though for Optimizer bulk_registering it actually pulls the results back off of Storage in the MainProcess.

Type of Change

  • 🔄 Refactor
  • 🧪 Tests

Testing

  • New CI tests for tweaks in pending_trials
  • Existing CI tests

Additional Notes (optional)


@bpkroth bpkroth added the ready for review Ready for review label May 9, 2025
@bpkroth bpkroth marked this pull request as ready for review May 9, 2025 22:53
@Copilot Copilot AI review requested due to automatic review settings May 9, 2025 22:53
@bpkroth bpkroth requested a review from a team as a code owner May 9, 2025 22:53
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 the Scheduler methods to improve clarity and support the upcoming ParallelScheduler changes. Key changes include renaming methods (e.g., schedule_trial to add_trial_to_queue), moving common functionality into the base class, and subtly updating trial filtering logic in pending_trials.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
mlos_bench/tests/storage/trial_schedule_test.py Added tests for the new trial_runner_assigned parameter in pending_trials
mlos_bench/storage/sql/experiment.py Updated pending_trials to support optional filtering based on trial runner status
mlos_bench/storage/base_storage.py Revised interface of pending_trials with new optional trial_runner_assigned flag
mlos_bench/schedulers/trial_runner.py Modified run_trial to return a tuple containing status, timestamp, and results
mlos_bench/schedulers/sync_scheduler.py Removed redundant start method implementation
mlos_bench/schedulers/base_scheduler.py Renamed methods and restructured the scheduling loop
.vscode/settings.json Adjusted VS Code settings for import organization
Comments suppressed due to low confidence (2)

mlos_bench/mlos_bench/schedulers/trial_runner.py:171

  • The docstring for run_trial does not reflect its updated return type. Please update the documentation to specify that the function returns a tuple with (Status, datetime, dict[str, TunableValue] | None).
def run_trial(self, trial: Storage.Trial, global_config: dict[str, Any] | None = None) -> tuple[Status, datetime, dict[str, TunableValue] | None]:

mlos_bench/mlos_bench/schedulers/base_scheduler.py:523

  • Since pending_trials is filtered using trial_runner_assigned=True, the following check for trial.trial_runner_id being None might be redundant. Please verify that this extra safeguard is intended and necessary.
if trial.trial_runner_id is None:

Copy link
Member

@motus motus left a comment

Choose a reason for hiding this comment

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

Looks very nice!

Copy link
Member

@motus motus left a comment

Choose a reason for hiding this comment

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

Looks very nice and clean! Thanks!

@motus motus merged commit ca4c78e into microsoft:main May 12, 2025
15 checks passed
@bpkroth bpkroth deleted the refactor/rename-reorg-scheduler-methods branch May 12, 2025 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants