Allow for bisecting by pystats #459
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for bisecting using pystats by updating the benchmark collection functions and integrating new command-line flags to drive the bisect process. Key changes include:
- Updating type hints and default parameters in the collect_pystats function.
- Renaming and adjusting functions in bisect.py to include new pystats and invert parameters.
- Modifying the argument parsing logic in bisect.py to support the new bisect mode.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| bench_runner/scripts/run_benchmarks.py | Updates to type hints and logic in collect_pystats to run summarize stats only when both fork and ref are provided. |
| bench_runner/scripts/bisect.py | Introduction of new functions and parameters (pystats, invert) to handle pystats bisecting; modification of command-line arguments and function calls to support new behavior. |
Comments suppressed due to low confidence (1)
bench_runner/scripts/bisect.py:257
- Using a string argument to handle a boolean value can be error-prone; consider using an action such as 'store_true' for the --invert flag to simplify the interface.
parser.add_argument("invert", type=str, choices=["True", "False"])
| pass | ||
| else: | ||
| if individual: | ||
| if individual and fork is not None and ref is not None: |
There was a problem hiding this comment.
Ensure that the new condition correctly captures all intended cases. If the bisect operation requires both fork and ref to be set, consider documenting the expected behavior to avoid ambiguity.
| if individual and fork is not None and ref is not None: | |
| # Ensure both fork and ref are set and valid before proceeding. | |
| if individual and isinstance(fork, str) and fork.strip() and isinstance(ref, str) and ref.strip(): | |
| # This ensures that the operation can correctly summarize stats for the given benchmark. | |
| if individual and isinstance(fork, str) and fork.strip() and isinstance(ref, str) and ref.strip(): |
| benchmark_links = [] | ||
|
|
||
| run_summarize_stats(python, fork, ref, "all", benchmark_links, flags=flags) | ||
| if fork is not None and ref is not None: |
There was a problem hiding this comment.
[nitpick] This check duplicates the condition logic from earlier; consider centralizing the validation of fork and ref to improve maintainability and reduce potential inconsistencies.
1 participant
No description provided.