Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Consistently annotate Scheduler._timeout_hours as float | None #2951

Closed
wants to merge 2 commits into from
Closed

Consistently annotate Scheduler._timeout_hours as float | None #2951

wants to merge 2 commits into from

Commits on Oct 24, 2024

  1. Make BoTorch test problems into ParamBasedTestProblems (facebook#2944) 

    Summary:

Context:

In the benchmarks, we currently have a zoo of five runners designed to handle different ways of generating data. This will make it challenging to support other changes to runners, such as running trials asynchronously. This diff is a step towards consolidating into one Runner, abstracting away the data-generating process into a test problem.

* BoTorchTestProblemRunner has a BoTorch problem
* ParamBasedTestProblemRunner has a ParamBasedTestProblem
* SurrogateRunner has a SurrogateProblem
* `BoTorchTestProblemRunner` and `ParamBasedTestProblemRunner` subclass `BenchmarkRunner`
* `SurrogateRunner` subclasses `BenchmarkRunner`

This diff will enable going from 5 runners to 3, wrapping BoTorch problems with `ParamBasedTestProblem`. This will allow for combining `BoTorchTestProblemRunner`, `ParamBasedTestProblemRunner`, and `SyntheticProblemRunner`.

This PR:
* Introduces `BoTorchTestProblem`, which is a `ParamBasedTestProblem` and thus can be used with `ParamBasedTestProblemRunner`. It localizes tensor-related logic, including `modified_bounds`, into the BoTorch problem so it doesn't need to be handled by the runner.
* Gets rid of `SyntheticProblemRunner`, merging it with `ParamBasedTestProblemRunner`. We may want to rename this class after more consolidation.
* Makes `BoTorchTestProblemRunner` a do-nothing subclass of `ParamBasedTestProblemRunner`

Reviewed By: Balandat

Differential Revision: D63639190
    @esantorella @facebook-github-bot
    esantorella authored and facebook-github-bot committed Oct 24, 2024
    Configuration menu
    Copy the full SHA
    d14cbc7 View commit details
    Browse the repository at this point in the history

  2. Consistently annotate Scheduler._timeout_hours as float | None (faceb… 

    …ook#2951)

Summary:

Context:
* `Scheduler._timeout_hours`, `ScheduleOptions.timeout_hours`, and `timeout_hours` arguments are variously annotated as `int | None`, `float | None`, or `int | float | None`, even though ints and floats are both consistently supported.
* PEP 484 states that `float` is an acceptable annotation where `int | float` is accepted; an int can be treated as a subtype of a float for typing purposes.

Note: Before finding out that int can be considered a subtype of float, I tried to use the ABCs from `numbers` as a nicer way of annotating `int | float`, but then found out that `numbers.Real` does not support post-multiplication by an int, and floats are not covered by `numbers.Rational`, so that didn't work.


This PR:
* Changes all these annotations to `float | None`

Differential Revision: D64897260
    @esantorella @facebook-github-bot
    esantorella authored and facebook-github-bot committed Oct 24, 2024
    Configuration menu
    Copy the full SHA
    64b283f View commit details
    Browse the repository at this point in the history