Commit fb5afd8
Clean up align_partial_results, eliminate double alignment, and remove Data round-trip
Summary:
Profiling-driven cleanup and performance improvements to the early stopping pipeline.
**1. Remove redundant validations from `align_partial_results` (utils.py)**
`align_partial_results` contained three validation/logging blocks that are redundant with upstream checks already performed by `_lookup_and_validate` in `base.py`:
- **Missing metrics check + `raise ValueError` (lines 166-168):** `_lookup_and_validate` already verifies each metric signature exists in the data (base.py lines 234-240) and returns `None` before `align_partial_results` is ever called.
- **Per-metric logging loop (lines 172-180):** The "no data" branch is unreachable because the upstream checks guarantee data exists for each metric. The debug logging about MAP_KEY ranges is misplaced for a pure data-alignment function.
- **Trial-to-arm uniqueness check (lines 186-198):** Redundant with `is_eligible_any` (base.py lines 344-352) which already rejects `BatchTrial` -- the only way a trial gets multiple arms.
The arm-to-trial uniqueness check was moved (not removed) to `_lookup_and_validate`, where it properly guards all downstream consumers, not just alignment.
The `isin` filter (`df = df[df['metric_signature'].isin(metrics)]`) was kept -- it is part of `align_partial_results`'s own contract since the function accepts a `metrics` argument and callers depend on it filtering to those metrics.
After cleanup, `align_partial_results` is a focused alignment function: filter to requested metrics -> drop `arm_name` -> drop duplicates -> sort -> pivot -> interpolate.
**2. Eliminate double data alignment in `PercentileEarlyStoppingStrategy` (percentile.py)**
When `check_safe=True`, the base class `should_stop_trials_early` calls `_is_harmful()` which calls `_prepare_aligned_frames()`, then `_should_stop_trials_early()` calls `_prepare_aligned_frames()` again -- identical data lookup + alignment running twice.
Fix: override `should_stop_trials_early` to call `_default_objective_and_direction()` and `_prepare_aligned_frames()` once, passing results as explicit keyword arguments (`metric_signature`, `minimize`, `aligned_frames`) to both `_is_harmful` and `_should_stop_trials_early`. Each defaults to `None` and falls back to computing from scratch when not provided, preserving backward compatibility for subclasses.
**3. Eliminate the `Data` round-trip in `_lookup_and_validate` (base.py, threshold.py)**
`_lookup_and_validate` (formerly `_lookup_and_validate_data`) used to wrap its filtered DataFrame back into `Data(df=filtered_df)` at the end, purely to satisfy its `Data | None` return type. The sole consumer (`_prepare_aligned_frames`) immediately called `.full_df` to get the DataFrame back. This triggered a pointless df-to-DataRow-to-df round-trip on every call:
```
DataFrame -> itertuples -> list[DataRow] -> from_records -> regex sort -> cast -> DataFrame
```
Profiling shows this round-trip is **100% overhead** at every scale:
| Scale | Rows | Round-trip cost | Replacement (df.copy) |
|-----------------|---------|----------------|-----------------------|
| tiny (5x10) | 50 | 4ms | <0.1ms |
| typical (20x100)| 2,000 | 15ms | <0.1ms |
| large (50x200) | 10,000 | 60ms | <0.1ms |
| xlarge (100x200)| 20,000 | 160ms | <0.1ms |
| huge (200x500) | 100,000 | 733ms | <0.1ms |
The cost comes from `Data.__init__` iterating every row via `itertuples()` to build `list[DataRow]` (~35ms at 10k rows), then `Data.full_df` reconstructing the DataFrame via `from_records()` (~12ms) and running regex-based arm name parsing in `sort_by_trial_index_and_arm_name()` (~19ms) -- none of which is needed since we already had the DataFrame.
Fix: change `_lookup_and_validate` to return `pd.DataFrame | None` directly. Update `ModelBasedEarlyStoppingStrategy` and `ThresholdEarlyStoppingStrategy` accordingly.
End-to-end profiling of `should_stop_trials_early` at the 50x200 scale (10k rows) shows a ~19% speedup (323ms -> ~263ms), with the benefit growing at larger scales (up to ~41% at 100k rows).
**4. Naming cleanup**
Renamed methods and variables to disambiguate between `Data` objects and `pd.DataFrame`:
- `_lookup_and_validate_data` -> `_lookup_and_validate` (returns `pd.DataFrame | None`)
- `_prepare_aligned_data` -> `_prepare_aligned_frames` (returns a tuple of DataFrames)
- Variables holding `_lookup_and_validate` results: `data`/`map_data`/`data_lookup` -> `df`
- Parameter `aligned_data` -> `aligned_frames`
**5. Tests**
- Removed tests for moved/removed `align_partial_results` validations.
- Added test for the arm-to-trial uniqueness check in `_lookup_and_validate`.
- Added profiling notebook at `ax/early_stopping/profiling.ipynb`.
Differential Revision: D985448351 parent f5976b8 commit fb5afd8
5 files changed
Lines changed: 206 additions & 172 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
18 | | - | |
| 18 | + | |
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| |||
217 | 217 | | |
218 | 218 | | |
219 | 219 | | |
220 | | - | |
| 220 | + | |
221 | 221 | | |
222 | | - | |
223 | | - | |
224 | | - | |
225 | | - | |
226 | | - | |
| 222 | + | |
| 223 | + | |
227 | 224 | | |
228 | 225 | | |
229 | 226 | | |
| |||
250 | 247 | | |
251 | 248 | | |
252 | 249 | | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
253 | 261 | | |
254 | 262 | | |
255 | 263 | | |
| |||
264 | 272 | | |
265 | 273 | | |
266 | 274 | | |
267 | | - | |
| 275 | + | |
268 | 276 | | |
269 | 277 | | |
270 | 278 | | |
| |||
547 | 555 | | |
548 | 556 | | |
549 | 557 | | |
550 | | - | |
| 558 | + | |
551 | 559 | | |
552 | 560 | | |
553 | 561 | | |
| |||
564 | 572 | | |
565 | 573 | | |
566 | 574 | | |
567 | | - | |
| 575 | + | |
568 | 576 | | |
569 | 577 | | |
570 | | - | |
| 578 | + | |
571 | 579 | | |
572 | 580 | | |
573 | 581 | | |
574 | 582 | | |
575 | | - | |
| 583 | + | |
576 | 584 | | |
577 | 585 | | |
578 | 586 | | |
| |||
651 | 659 | | |
652 | 660 | | |
653 | 661 | | |
654 | | - | |
| 662 | + | |
655 | 663 | | |
656 | | - | |
657 | | - | |
658 | | - | |
659 | | - | |
| 664 | + | |
| 665 | + | |
| 666 | + | |
660 | 667 | | |
661 | | - | |
| 668 | + | |
662 | 669 | | |
663 | 670 | | |
664 | | - | |
665 | | - | |
666 | | - | |
667 | | - | |
668 | | - | |
| 671 | + | |
| 672 | + | |
| 673 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
124 | 124 | | |
125 | 125 | | |
126 | 126 | | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
127 | 177 | | |
128 | 178 | | |
129 | 179 | | |
130 | 180 | | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
131 | 184 | | |
132 | 185 | | |
133 | 186 | | |
| |||
139 | 192 | | |
140 | 193 | | |
141 | 194 | | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
142 | 202 | | |
143 | 203 | | |
144 | 204 | | |
145 | 205 | | |
146 | 206 | | |
147 | | - | |
148 | | - | |
149 | | - | |
150 | | - | |
151 | | - | |
152 | | - | |
153 | | - | |
154 | | - | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
155 | 217 | | |
156 | | - | |
| 218 | + | |
157 | 219 | | |
158 | 220 | | |
159 | 221 | | |
| |||
180 | 242 | | |
181 | 243 | | |
182 | 244 | | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
183 | 248 | | |
184 | 249 | | |
185 | 250 | | |
| |||
191 | 256 | | |
192 | 257 | | |
193 | 258 | | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
194 | 266 | | |
195 | 267 | | |
196 | 268 | | |
197 | 269 | | |
198 | 270 | | |
199 | 271 | | |
200 | | - | |
201 | | - | |
202 | | - | |
203 | | - | |
204 | | - | |
205 | | - | |
206 | | - | |
207 | | - | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
208 | 282 | | |
209 | | - | |
| 283 | + | |
210 | 284 | | |
211 | 285 | | |
212 | 286 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
112 | 112 | | |
113 | 113 | | |
114 | 114 | | |
115 | | - | |
| 115 | + | |
116 | 116 | | |
117 | 117 | | |
118 | | - | |
| 118 | + | |
119 | 119 | | |
120 | 120 | | |
121 | 121 | | |
122 | | - | |
123 | | - | |
124 | 122 | | |
125 | 123 | | |
126 | 124 | | |
| |||
0 commit comments