Skip to content

Resolve BENCH_DIR at call time in the benchmark runner's main() - #2563

Open
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:bench-runner-main-late-binding
Open

Resolve BENCH_DIR at call time in the benchmark runner's main()#2563
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:bench-runner-main-late-binding

Conversation

@LeSingh1

@LeSingh1 LeSingh1 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

discover_benchmarks() goes out of its way to avoid def-time binding, and says why:

    # 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, module_name_prefix=module_name_prefix)

Because main() always passes a non-None bench_dir down, the sentinel branch in
discover_benchmarks() can never be taken on this path — so 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
(test_runner.py:124-127), which is why the gap is invisible today.

Fix

Apply the same None sentinel to both parameters of main(). Explicit keyword arguments
keep 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_dir patches BENCH_DIR to a tmp dir holding one
bench_*.py and drives main() with --list, asserting the patched benchmark is the only
one listed.

Fails on main: the real repo benchmarks are listed instead. Pure Python — the
existing load_runner_main() helper already stubs pyperf via sys.modules, so no CUDA,
no GPU, no pyperf install.

Verified locally: 3 passed before, 4 passed after; the new test fails against
upstream/main's runner/main.py. ruff check and ruff format --check are clean.

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).
@copy-pr-bot

copy-pr-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

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.

1 participant