Reap deprecated **kwargs argument from optimize_acqf variants #2390
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2390 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 191 191
Lines 16715 16709 -6
=======================================
- Hits 16712 16706 -6
Misses 3 3 ☔ View full report in Codecov by Sentry. |
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jun 26, 2024
Summary: Passing `sequential` has been raising a deprecation warning, and stopping passing it enables pytorch/botorch#2390 . Currently, the `sequential` argument is always provided by Ax and can be overridden by the user. This solution silently ignores the argument whenever the mixed optimizer is used. A nicer solution would be for Ax to only construct the `sequential` argument when it is needed and for there to be an exception when the user passes `sequential=False` and the mixed optimizer is used. If BoTorch plans to eventually enable `sequential=True` with `optimize_acqf_mixed`, then the exception should be raised by BoTorch so that Ax doesn't have to stay in sync with BoTorch's current capabilities. However, I think this code could use a thorough cleanup, so I went with the simple solution rather than add more `if` statements. Differential Revision: D59057005
|
@esantorella merged this pull request in 1b11aca. |
facebook-github-bot
pushed a commit
to facebook/Ax
that referenced
this pull request
Jun 26, 2024
Summary: Pull Request resolved: #2545 Passing `sequential` has been raising a deprecation warning, and stopping passing it enables pytorch/botorch#2390 . Currently, the `sequential` argument is always provided by Ax and can be overridden by the user. This solution silently ignores the argument whenever the mixed optimizer is used. A nicer solution would be for Ax to only construct the `sequential` argument when it is needed and for there to be an exception when the user passes `sequential=False` and the mixed optimizer is used. If BoTorch plans to eventually enable `sequential=True` with `optimize_acqf_mixed`, then the exception should be raised by BoTorch so that Ax doesn't have to stay in sync with BoTorch's current capabilities. However, I think this code could use a thorough cleanup, so I went with the simple solution rather than add more `if` statements. Reviewed By: Balandat Differential Revision: D59057005 fbshipit-source-id: c545f030672b7e30c4405c734b7e7c6605d8a1f8
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jun 28, 2024
Summary: pytorch/botorch#2390 broke workflows that were passing this argument, which used to be silently ignored. This is a hotfix that will stop those usages from failing. Differential Revision: D59161641
facebook-github-bot
pushed a commit
to facebook/Ax
that referenced
this pull request
Jul 1, 2024
Summary: pytorch/botorch#2390 broke workflows that were passing this argument, which used to be silently ignored. This is a hotfix that will stop those usages from failing. More details: Combinerator integ tests failing with: ``` TypeError: optimize_acqf_discrete() got an unexpected keyword argument 'raw_samples' ``` e.g., https://fburl.com/logarithm/kyr18i16 (scuba query: https://fburl.com/scuba/fblearner_workflow_run_status/hid8tp94). Reviewed By: dme65 Differential Revision: D59161641 fbshipit-source-id: 16aae81c03016e68ed866e29c61e3ffb1d317b95
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jul 12, 2024
…ptimizer; cleanup (facebook#2571) Summary: Pull Request resolved: facebook#2571 # Context The current flow is 1. MBM `Acquisition.optimize` constructs arguments passed to BoTorch optimizers in `optimizer_argparse` without knowing which of the four optimizers it will be passing the arguments to. It has only a boolean flag indicating whether the optimizer is discrete. 2. `Acquisition.optimize` decides which of the four optimizers to use and does optimizer specific-logic (these two parts are not really sequential). 3. `Acquisition.optimize` calls a BoTorch optimizer, passing some arguments that may not apply to the optimizer that actually got used. 4. Prior to pytorch/botorch#2390, the inappropriate arguments would be silently ignored, but now they raise an exception. MBM can dispatch to four different BoTorch optimizers depending on the search space. Currently, `optimize_acqf_discrete_local_search` is failing because it is passed the inappropriate argument `sequential`. # This diff * Makes the flow within `Acquisition.optimize` more clear and changes `optimizer_argparse` so that inappropriate arguments such as `sequential` are not passed to optimizers they don't apply to: 1. `Acquisition.optimize` determines which of the four optimizers is appropriate. 2. `Acquisition.optimize` constructs arguments based on that optimizer, only constructing needed arguments. 3. Then it does any optimizer-specific logic. 4. Then it calls a BoTorch optimizer; there is no longer an error because only appropriate arguments were passed. * Extends unit tests for `optimizer_argparse` to check all optimizers * Reduces the usage and scope of mocks in test_acquisition so that `optimize_acqf` and its variants are actually run as much as possible. Differential Revision: D59354709
facebook-github-bot
pushed a commit
to facebook/Ax
that referenced
this pull request
Jul 13, 2024
…ptimizer; cleanup (#2571) Summary: Pull Request resolved: #2571 # Context The current flow is 1. MBM `Acquisition.optimize` constructs arguments passed to BoTorch optimizers in `optimizer_argparse` without knowing which of the four optimizers it will be passing the arguments to. It has only a boolean flag indicating whether the optimizer is discrete. 2. `Acquisition.optimize` decides which of the four optimizers to use and does optimizer specific-logic (these two parts are not really sequential). 3. `Acquisition.optimize` calls a BoTorch optimizer, passing some arguments that may not apply to the optimizer that actually got used. 4. Prior to pytorch/botorch#2390, the inappropriate arguments would be silently ignored, but now they raise an exception. MBM can dispatch to four different BoTorch optimizers depending on the search space. Currently, `optimize_acqf_discrete_local_search` is failing because it is passed the inappropriate argument `sequential`. # This diff * Makes the flow within `Acquisition.optimize` more clear and changes `optimizer_argparse` so that inappropriate arguments such as `sequential` are not passed to optimizers they don't apply to: 1. `Acquisition.optimize` determines which of the four optimizers is appropriate. 2. `Acquisition.optimize` constructs arguments based on that optimizer, only constructing needed arguments. 3. Then it does any optimizer-specific logic. 4. Then it calls a BoTorch optimizer; there is no longer an error because only appropriate arguments were passed. * Extends unit tests for `optimizer_argparse` to check all optimizers * Reduces the usage and scope of mocks in test_acquisition so that `optimize_acqf` and its variants are actually run as much as possible. Reviewed By: saitcakmak Differential Revision: D59354709 fbshipit-source-id: 88bdb464b6222cfb98f4263855288d3e0367ccc2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This code was deprecated somewhere between 0.8.0 and 0.9.0; as we are now past 0.11.0, it can be reaped.
Test Plan
Existing units, including tutorials.
Related PRs
#1677