Speed up test_ax_sweep_impl_minimal by mocking Sobol sensitivity MC samples - #5282
Closed
saitcakmak wants to merge 1 commit into
Closed
Speed up test_ax_sweep_impl_minimal by mocking Sobol sensitivity MC samples#5282saitcakmak wants to merge 1 commit into
saitcakmak wants to merge 1 commit into
Conversation
…amples Summary: `automl.internal.ax.ax_sweep.tests.test_ax_sweep_feature_matrix.TestAxSweepFeatureMatrix.test_ax_sweep_impl_minimal` (T562950220699357) has been timing out on stress runs against TPX's 600s limit. Profiling shows the runtime is dominated by variance-based (Sobol) sensitivity analysis, not by the number of analysis cards: - `AxSweepOrchestrator.report_results` is called 4x and accounts for 110.2s of a 127.2s local run (87%): `_compute_and_save_analysis_cards` 67.3s, legacy `get_standard_plots` 38.5s. - Exclusive per-`Analysis` accounting: `SensitivityAnalysisPlot` 65.9s over 2 calls; every other analysis is under 0.5s. - `ax_parameter_sens` defaults to `num_mc_samples = 10**4`, so each call is O(num_mc_samples * num_params) posterior-mean evaluations, multiplied by the MCMC sample count on this test's fully Bayesian (SAASBO) model. `mock_botorch_optimize` does not help, since it cheapens fitting, not posterior evaluation. - It runs 4x per test: twice via `get_standard_plots` -> `ax_parameter_sens(order="total")` and twice via `OverviewAnalysis` -> `InsightsAnalysis` -> `TopSurfacesAnalysis` -> `SensitivityAnalysisPlot`, where `_choose_sensitivity_order(3)` selects the most expensive `"second"` order. This adds an opt-in `mock_sensitivity_analysis` decorator / context manager to `ax/utils/testing/mock.py` that forces `num_mc_samples=32` in `compute_sobol_indices_from_model_list` and `compute_derivatives_from_model_list`, and applies it to `execute_ax_sweep_and_make_assertions`. Nothing in that test asserts on sensitivity values -- only that the expected number of plots is produced -- so the reduced sample count is safe here. Note that Ax's own `TestCase.MAX_TEST_SECONDS = 120` guard does fire on this test, but the resulting `TimeoutError` is swallowed by `Analysis.compute_result`'s broad `except Exception` (surfacing only as a "Failed to compute SensitivityAnalysisPlot" error card) and by `report_results`' broad handlers, so the test still reported ok. That is worth fixing separately. Differential Revision: D118535425
|
@saitcakmak has exported this pull request. If you are a Meta employee, you can view the originating Diff in D118535425. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5282 +/- ##
==========================================
- Coverage 96.59% 96.57% -0.03%
==========================================
Files 621 621
Lines 70894 70911 +17
==========================================
- Hits 68481 68480 -1
- Misses 2413 2431 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This pull request has been merged in 778e22f. |
This file contains hidden or 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
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.
Summary:
automl.internal.ax.ax_sweep.tests.test_ax_sweep_feature_matrix.TestAxSweepFeatureMatrix.test_ax_sweep_impl_minimal(T562950220699357) has been timing out on stress runs against TPX's 600s limit. Profiling shows the runtime is dominated by variance-based (Sobol) sensitivity analysis, not by the number of analysis cards:AxSweepOrchestrator.report_resultsis called 4x and accounts for 110.2s of a 127.2s local run (87%):_compute_and_save_analysis_cards67.3s, legacyget_standard_plots38.5s.Analysisaccounting:SensitivityAnalysisPlot65.9s over 2 calls; every other analysis is under 0.5s.ax_parameter_sensdefaults tonum_mc_samples = 10**4, so each call is O(num_mc_samples * num_params) posterior-mean evaluations, multiplied by the MCMC sample count on this test's fully Bayesian (SAASBO) model.mock_botorch_optimizedoes not help, since it cheapens fitting, not posterior evaluation.get_standard_plots->ax_parameter_sens(order="total")and twice viaOverviewAnalysis->InsightsAnalysis->TopSurfacesAnalysis->SensitivityAnalysisPlot, where_choose_sensitivity_order(3)selects the most expensive"second"order.This adds an opt-in
mock_sensitivity_analysisdecorator / context manager toax/utils/testing/mock.pythat forcesnum_mc_samples=32incompute_sobol_indices_from_model_listandcompute_derivatives_from_model_list, and applies it toexecute_ax_sweep_and_make_assertions. Nothing in that test asserts on sensitivity values -- only that the expected number of plots is produced -- so the reduced sample count is safe here.Note that Ax's own
TestCase.MAX_TEST_SECONDS = 120guard does fire on this test, but the resultingTimeoutErroris swallowed byAnalysis.compute_result's broadexcept Exception(surfacing only as a "Failed to compute SensitivityAnalysisPlot" error card) and byreport_results' broad handlers, so the test still reported ok. That is worth fixing separately.Differential Revision: D118535425