Resolve BENCH_DIR at call time in the benchmark runner's main() - #2563
Open
LeSingh1 wants to merge 1 commit into
Open
Resolve BENCH_DIR at call time in the benchmark runner's main()#2563LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
discover_benchmarks() goes out of its way to avoid def-time binding, and
says so:
# Resolve the default inside the call so tests (and embedders) can
# monkeypatch ``BENCH_DIR`` at the module level - Python binds default
# args at def-time, so a literal default would ignore later patches.
if bench_dir is None:
bench_dir = BENCH_DIR
main() then reintroduces exactly that binding:
def main(
*,
bench_dir: Path = BENCH_DIR,
default_output: Path = DEFAULT_OUTPUT,
...
registry = discover_benchmarks(bench_dir=bench_dir, ...)
Because main() always passes a non-None bench_dir down, the sentinel branch
in discover_benchmarks() can never be taken on this path, and patching
runner.main.BENCH_DIR - the documented mechanism - has no effect on main().
Same for DEFAULT_OUTPUT.
run_pyperf.py calls main() with no arguments, so this is the production
path. The existing tests patch BENCH_DIR and call discover_benchmarks()
directly, which is why the gap is invisible today.
Apply the same sentinel to both parameters. Explicit arguments keep working
unchanged, so the embedder API is unaffected.
Adds test_main_honors_a_monkeypatched_bench_dir, which patches BENCH_DIR to a
tmp dir holding one bench_*.py and drives main() with --list. It fails before
this change (main() lists the repo's real benchmarks instead).
Contributor
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.
discover_benchmarks()goes out of its way to avoid def-time binding, and says why:main()then reintroduces exactly that binding:Because
main()always passes a non-Nonebench_dirdown, the sentinel branch indiscover_benchmarks()can never be taken on this path — so patchingrunner.main.BENCH_DIR, the documented mechanism, has no effect onmain(). Same forDEFAULT_OUTPUT.run_pyperf.pycallsmain()with no arguments, so this is the production path. Theexisting tests patch
BENCH_DIRand calldiscover_benchmarks()directly(
test_runner.py:124-127), which is why the gap is invisible today.Fix
Apply the same
Nonesentinel to both parameters ofmain(). Explicit keyword argumentskeep working unchanged, so the embedder API is unaffected — this only restores the
late-binding the module already intends.
Test
test_main_honors_a_monkeypatched_bench_dirpatchesBENCH_DIRto a tmp dir holding onebench_*.pyand drivesmain()with--list, asserting the patched benchmark is the onlyone listed.
Fails on
main: the real repo benchmarks are listed instead. Pure Python — theexisting
load_runner_main()helper already stubspyperfviasys.modules, so no CUDA,no GPU, no pyperf install.
Verified locally: 3 passed before, 4 passed after; the new test fails against
upstream/main'srunner/main.py.ruff checkandruff format --checkare clean.