ci: complete comparative benchmark contract for #309 - #373
codeforester wants to merge 2 commits into
Conversation
| f"base-cli cold invocation p95 exceeded {COLD_INVOCATION_P95_BUDGETS_MS[BENCHMARK_PLATFORM]:.0f} ms" | ||
| ) | ||
|
|
||
| for framework in FRAMEWORKS: |
There was a problem hiding this comment.
Correctness: _check_results gates ALL comparator frameworks' warm_invocation_ms p95 against WARM_INVOCATION_P95_BUDGETS_MS, not just base-cli's.
for framework in FRAMEWORKS:
warm_p95 = _metric_p95(results.get(framework, {}), "warm_invocation_ms")
if warm_p95 is not None and warm_p95 > WARM_INVOCATION_P95_BUDGETS_MS[BENCHMARK_PLATFORM]:
failures.append(...)Before this PR, only base-cli's own import/invocation metrics were checked against a budget; Click/Typer/Cyclopts were purely informational comparators. This loop now fails --check if any of those external, unpinned-minor libraries has a warm no-op p95 above 50ms/100ms — e.g. a slightly slower point release, or ordinary CI-runner noise (this same PR's docs cite a 26ms MAD on Windows for the persistence scenario, so this class of variance is real on these runners). That turns base-cli's CI red for a reason completely unrelated to any base-cli code change.
Suggest restricting this loop to framework == "base-cli" (matching how cold_import_ms/cold_invocation_ms are already scoped to base-cli only a few lines above), or dropping the loop and relying on production_warm_invocation_ms/feature-scenario checks, which already cover base-cli's own warm-path budget.
|
|
||
| command = cast(Any, app.click_command) | ||
| runner = CliRunner() | ||
| return _measure_runner(iterations, lambda: runner.invoke(command, []).exit_code) |
There was a problem hiding this comment.
Correctness (measurement isolation): _measure_lifecycle_invocations does not isolate a temp HOME/XDG dir the way _measure_production_invocations and _measure_base_cli_features do (both wrap invocations in tempfile.TemporaryDirectory() and pass home=). Here the app is invoked directly through a bare CliRunner().invoke(command, []) with the ambient process environment and CWD.
This metric feeds the tightest gate in the whole contract: LIFECYCLE_OVERHEAD_P95_BUDGETS_MS (5ms on unix/macos, 15ms on windows/wsl) via lifecycle_increment_over_click_warm_p95_ms. If base-cli's per-invocation path does any HOME/CWD-dependent work (config/history discovery, XDG lookups) even with log_to_file=False, running it against the real ambient environment (a developer's real $HOME, or the CI checkout's CWD) rather than an isolated empty temp dir can make this sample slower/noisier than in a clean environment, tripping the 5ms budget for reasons unrelated to an actual regression, and making the number non-reproducible across machines.
Worth isolating HOME here the same way the other scenarios do, unless the intent is specifically to mirror Click/Typer's own un-isolated CliRunner usage for a fair 'lifecycle overhead over Click' comparison — if so, that tradeoff is worth a one-line comment since it's inconsistent with the isolation used everywhere else in this file.
| package_root = Path(__file__).resolve().parents[1] / "lib" / "python" | ||
| environment = dict(os.environ) | ||
| existing_path = environment.get("PYTHONPATH") | ||
| environment["PYTHONPATH"] = f"{package_root}{os.pathsep}{existing_path}" if existing_path else str(package_root) |
There was a problem hiding this comment.
Cleanup (reuse): The PYTHONPATH-environment-setup block is duplicated verbatim between _measure_import (a few lines above) and _measure_cold_invocations (here):
package_root = Path(__file__).resolve().parents[1] / "lib" / "python"
environment = dict(os.environ)
existing_path = environment.get("PYTHONPATH")
environment["PYTHONPATH"] = f"{package_root}{os.pathsep}{existing_path}" if existing_path else str(package_root)Worth extracting into a small _subprocess_environment() -> dict[str, str] helper both call. As written, a future change to how the source checkout is exposed to the spawned subprocess (e.g. an extra path segment, a different env var) has to be kept in sync by hand across both copies, and a partial edit would silently desync cold-import vs cold-invocation measurement environments.
1 participant
Closes #309. This PR is stacked on #372 (
ci/312-20260918-deduplicate-invariant-gates); after #372 lands, retarget this PR tomainfor final review.\n\n### Changes\n- Measures fresh-process import, cold invocation, warm parser dispatch, and base-cli lifecycle/production paths for Click, Typer, Cyclopts, and base-cli.\n- Adds base-cli JSON success/error, diagnostics, nested command, and persistence enabled/disabled scenarios.\n- Makes missing comparators or scenarios fail; enforces per-profile p95 budgets and lifecycle delta over Click.\n- Publishes versioned JSON with source/environment/framework versions, p95/median/MAD, and lifecycle comparisons.\n- Adds public per-platform Actions summaries and retains dated JSON artifacts for 90 days, including WSL.\n- Documents scenario semantics, local commands, thresholds, and calibration evidence.\n\n### Validation\n- Full local validation: 534 tests and 193 subtests passed; 87.20% coverage; typing, lint, contracts, benchmark, and security gates passed.\n- Comparative benchmark passed with 31 samples per scenario on local macOS for all four frameworks.\n- Docs, changelog, YAML, Ruff, and benchmark self-tests passed.