Add a helper script to bisect a regression#430
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a helper script for bisecting regressions in CPython by automating the checkout, build, and benchmark run steps. Key changes include:
- Introducing a new helper function (format_seconds) for human-readable time formatting.
- Updating compile_unix to add a reconfigure flag.
- Implementing the bisect script that automates bisect operations and logs results.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| bench_runner/util.py | Added format_seconds for time formatting with a loop using magic numbers. |
| bench_runner/scripts/workflow.py | Updated compile_unix signature with a reconfigure flag to clean builds appropriately. |
| bench_runner/scripts/bisect.py | Implemented the bisect flow including result parsing, logging, and git bisect automation. |
| bench_runner/git.py | Added a simple checkout helper function. |
| bench_runner/main.py | Registered bisect as a new command in the command entry point. |
Comments suppressed due to low confidence (1)
bench_runner/scripts/bisect.py:206
- Parsing the 'pgo' argument by comparing against the string "True" can be error-prone. Consider converting the argument to a boolean explicitly during argument parsing for clarity.
timing = get_result(args.benchmark, args.pgo == "True", args.flags, cpython=cpython)
| for i in range(2, -9, -1): | ||
| if value >= 10.0**i: | ||
| break | ||
| else: | ||
| i = -9 |
There was a problem hiding this comment.
The use of magic numbers in the loop (range from 2 to -9) reduces readability. Consider defining named constants or adding inline comments to clarify the purpose of these bounds.
| for i in range(2, -9, -1): | |
| if value >= 10.0**i: | |
| break | |
| else: | |
| i = -9 | |
| MAX_POWER_OF_TEN = 2 # Maximum power of ten to check (10^2 = 100 seconds) | |
| MIN_POWER_OF_TEN = -9 # Minimum power of ten to check (10^-9 = 1 nanosecond) | |
| for i in range(MAX_POWER_OF_TEN, MIN_POWER_OF_TEN - 1, -1): | |
| if value >= 10.0**i: | |
| break | |
| else: | |
| i = MIN_POWER_OF_TEN |
|
|
||
| sys.argv = [sys.argv[0], *sys.argv[2:]] | ||
| mod = importlib.import_module(f"bench_runner.scripts.{command}") | ||
| mod = importlib.import_module(f".{command}", "bench_runner.scripts") |
There was a problem hiding this comment.
This is just due to picking the unfortunate name bisect that conflicts with the name of a stdlib module and not being careful about how things are run elsewhere.
| print("Bisect log:") | ||
|
|
||
| with get_log_file().open("r") as f: | ||
| for line in f.readlines(): |
There was a problem hiding this comment.
Is there a reason to use readlines instead of just iterating over the file?
| for line in f.readlines(): | |
| for line in f: |
| with contextlib.chdir(cpython): | ||
| subprocess.check_call(["./configure", *args], env=env) | ||
| if reconfigure: | ||
| subprocess.check_call(["./configure", *args], env=env) |
There was a problem hiding this comment.
Even if you don't explicitly run configure, the Makefile may decide to rerun configure when jumping through revisions... It's probably a good idea to pass -C (or --config-cache) to configure to significantly reduce configure time.
|
|
||
| COMMANDS = { | ||
| "backfill": "Schedule benchmarking a number of commits", | ||
| "bisect": "Run a bisect to find the commit that caused a regression", |
There was a problem hiding this comment.
Maybe add a comment that it won't work on Windows?
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| # This is the entry point when we are called from `git bisect run` itself |
There was a problem hiding this comment.
This feels a little magical and I would've probably just made this a separate script instead, but as long as it works...
A helper script to bisect a regression, usage:
Follow-on (but not part of this PR) will be to connect it to GitHub Actions so users can easily kick it off on one of the runners.