Skip to content

Fix stale API calls in the asv benchmark suite - #740

Open
s-sasaki-earthsea-wizard wants to merge 1 commit into
isce-framework:mainfrom
s-sasaki-earthsea-wizard:fix/asv-benchmark-suite
Open

Fix stale API calls in the asv benchmark suite#740
s-sasaki-earthsea-wizard wants to merge 1 commit into
isce-framework:mainfrom
s-sasaki-earthsea-wizard:fix/asv-benchmark-suite

Conversation

@s-sasaki-earthsea-wizard

Copy link
Copy Markdown

Both ShpBenchmark and SingleMinistackBenchmark fail on main. Each broke when the API it calls changed and the benchmark wasn't updated with it — one in early 2024, the other in late 2024.

Reproduce

On main, with vmtouch installed:

$ asv run --python=same --quick -b "ShpBenchmark|SingleMinistackBenchmark" --show-stderr
[ 25.00%] ··· benchmarks.ShpBenchmark.time_estimate_neighbors             failed
              TypeError: tuple indices must be integers or slices, not str
[ 50.00%] ··· ...ngleMinistackBenchmark.time_single_ministack             failed
              TypeError: run_wrapped_phase_sequential() missing 2 required
                         keyword-only arguments: 'slc_vrt_stack' and 'output_folder'

CovarianceBenchmark and PhaseLinkingBenchmark are unaffected.

What broke

ShpBenchmarkHALF_WINDOW stopped being a dict in 2de180f ("Convert phase linking to use jax", #203, 2024-01-31):

-HALF_WINDOW = {"x": 11, "y": 5}
-STRIDES = {"x": 6, "y": 3}
+HALF_WINDOW = HalfWindow(11, 5)
+STRIDES = Strides(3, 6)
+HALF_WINDOW_DICT = {"x": HALF_WINDOW.x, "y": HALF_WINDOW.y}
+STRIDES_DICT = {"x": STRIDES.x, "y": STRIDES.y}

benchmarks.py:134 still subscripted it with strings.

SingleMinistackBenchmarkrun_wrapped_phase_sequential changed its signature in 78e6375 ("Add logic for manually specifying reference dates mid-stack", #334, 2024-09-14): slc_vrt_file / ministack_planner became slc_vrt_stack / output_folder, and the MiniStackPlanner is now built inside the function. The benchmark still passed the old arguments and hand-built a planner.

Fixing the signature exposes a second layer: workflows/single.py:76-77 does strides["y"] and half_window["y"], so the sequential path still wants dicts, while the benchmark passed the HalfWindow / Strides NamedTuples. That is what HALF_WINDOW_DICT and STRIDES_DICT are for — added in #203, never referenced until this PR.

Why it went unnoticed

.github/workflows/benchmark.yml runs only on the benchmark label or workflow_dispatch, so the suite isn't part of ordinary CI. Since the workflow does grep "Traceback \|failed" benchmarks.log && exit 1, and an asv run against main produces both strings today, I'd expect a label-triggered run to go red regardless of what the PR changes. (I reproduced the asv run output locally; I have not triggered the workflow itself.)

After

$ asv run --python=same -b "ShpBenchmark|SingleMinistackBenchmark" --show-stderr
[ 50.00%] ··· Setting up benchmarks:176                                       ok
[ 75.00%] ··· benchmarks.ShpBenchmark.time_estimate_neighbors           751±70μs
[100.00%] ··· ...ngleMinistackBenchmark.time_single_ministack         2.03±0.04s

Python 3.14.6, asv 0.6.6 / asv_runner 0.3.1, jax 0.11.0 (GPU backend). SingleMinistackBenchmark writes and reads its own rasters, so its absolute number is I/O-dependent and machine-specific.

One note on asv_runner: versions 0.3.0 and earlier run a class's bound setup(self) before setup_cache(), which makes SingleMinistackBenchmark die in setup_cache before reaching the TypeError above (airspeed-velocity/asv_runner#52, fixed in 0.3.1). If you reproduce on 0.3.0 you'll see AssertionError: No SLC files found: [] instead.

Left alone

self.dates in SingleMinistackBenchmark.setup is now unused — nothing reads it once the planner is gone. I left it in place since setup isn't timed and removing it is a separate concern, but happy to drop it if you'd prefer.

Unrelated question, happy to split this out

Not a crash, so I'd rather ask than assume: #203 converted STRIDES faithfully ({"x": 6, "y": 3}Strides(3, 6), and Strides is (y, x)), but HALF_WINDOW went {"x": 11, "y": 5}HalfWindow(11, 5), which with HalfWindow being (y, x) means y=11, x=5 — the transpose of the original, and the opposite aspect ratio to the config default (half_window: y=7, x=14). So the covariance and phase-linking benchmarks have been running a tall window rather than a wide one since 2024-01-31. If that was unintentional, HalfWindow(5, 11) restores the original, at the cost of making results across that change non-comparable. If it was deliberate, ignore this half.


Disclosure: AI tools assisted with research, implementation, and drafting. I verified the technical claims and take responsibility for the change.

ShpBenchmark and SingleMinistackBenchmark have both been failing on main.
Neither is exercised by ordinary CI (benchmark.yml runs only on the
`benchmark` label or workflow_dispatch), so the breakage went unnoticed.

ShpBenchmark: HALF_WINDOW stopped being a dict in isce-framework#203 (2024-01-31) when it
became a HalfWindow NamedTuple, but line 134 still subscripted it with
strings.

SingleMinistackBenchmark: run_wrapped_phase_sequential switched from
slc_vrt_file/ministack_planner to slc_vrt_stack/output_folder in isce-framework#334
(2024-09-14), and now builds the MiniStackPlanner internally. The half
window and strides also have to be passed as dicts, since workflows/single.py
still indexes them by key -- which is what the HALF_WINDOW_DICT and
STRIDES_DICT constants added in isce-framework#203 were for; they had been unused until
now.
@scottstanie

Copy link
Copy Markdown
Contributor

The ASV setup was an aspirational goal to check for regressions in runtime as we added complexity. But, in the end, we never cared about many of the runtimes on the tiny machines github would provide for CI runners, and (more importantly) we could never tell that they would be free of other users, meaning it was not a good way to benchmark meaningful long running workflows. we should probably cut it out now, but hadn't checked on it in many months.

@s-sasaki-earthsea-wizard

Copy link
Copy Markdown
Author

No strong feelings about this PR on my end, so feel free to close it unmerged if removal is the way to go. If it helps, I'm happy to open an issue or PR removing the ASV setup (benchmarks/, asv.conf.json, .github/workflows/benchmark.yml, and the two references in pyproject.toml) — just say the word.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants