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.

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

Consistently annotate Scheduler._timeout_hours as float | None #2951

Closed
wants to merge 2 commits into from

Conversation

esantorella
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 24, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64897260

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.68%. Comparing base (e63b462) to head (64b283f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2951   +/-   ##
=======================================
  Coverage   95.68%   95.68%           
=======================================
  Files         504      504           
  Lines       49533    49547   +14     
=======================================
+ Hits        47395    47411   +16     
+ Misses       2138     2136    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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
…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
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64897260

esantorella added a commit to esantorella/Ax that referenced this pull request Oct 24, 2024
…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 added a commit to esantorella/Ax that referenced this pull request Oct 24, 2024
…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 added a commit to esantorella/Ax that referenced this pull request Oct 24, 2024
…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
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 71b47ad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants