feat(perf): standardize workload profiles and stabilize run policy - #16
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not has_any: | ||
| lines.append("| small | — | — |") | ||
| lines.append("| medium | — | — |") | ||
| lines.append("| large | — | — |") |
There was a problem hiding this comment.
Table rows already added in loop (lines 310-317), causing duplicates when has_any=False
| if not has_any: | |
| lines.append("| small | — | — |") | |
| lines.append("| medium | — | — |") | |
| lines.append("| large | — | — |") | |
| if not has_any: | |
| return [] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/excelbench/results/dashboard.py
Line: 319:322
Comment:
Table rows already added in loop (lines 310-317), causing duplicates when `has_any=False`
```suggestion
if not has_any:
return []
```
How can I resolve this? If you propose a fix, please make it concise.| if not isinstance(iteration_policy, str): | ||
| iteration_policy = "fixed" |
There was a problem hiding this comment.
Unnecessary type check - iteration_policy parameter has type annotation str in typer.Option, so will always be a string
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/excelbench/cli.py
Line: 273:274
Comment:
Unnecessary type check - `iteration_policy` parameter has type annotation `str` in typer.Option, so will always be a string
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f6ab30379
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if workload is not None: | ||
| try: | ||
| op_count = len(_cells_from_range(str(workload["range"]))) | ||
| except (TypeError, ValueError, KeyError): |
There was a problem hiding this comment.
Workload size is derived from len(_cells_from_range(...)), but write workloads with op == "bulk_write_grid" can drastically reduce effective operations via sparse_every (see _bench_write_workload), so these runs are bucketed into the wrong size tier. This skews the new “Best Adapter by Workload Profile” aggregation because rates are computed from reduced op_count while size is computed from the full dense range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR enhances ExcelBench’s performance results by adding iteration policy metadata, standardizing workload size classification, capturing coarse phase attribution for perf operations, and expanding the combined dashboard to highlight best adapters per workload size.
Changes:
- Add
iteration_policyto perf configuration and surface it in CLI output, markdown, and HTML dashboards. - Add
workload_sizeto perf result rows and compute it from workload ranges / test cases. - Add coarse
phase_attribution_msto perf op results, and add a dashboard section for “Best Adapter by Workload Profile”, with new tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/excelbench/perf/runner.py |
Adds iteration_policy, workload_size, phase_attribution_ms, and new helpers for workload sizing + phase attribution. |
src/excelbench/cli.py |
Adds --iteration-policy flag and passes it through to run_perf. |
src/excelbench/perf/renderer.py |
Includes iteration_policy in perf markdown output config line. |
src/excelbench/results/html_dashboard.py |
Displays iteration_policy in the perf meta bar. |
src/excelbench/results/dashboard.py |
Adds “Best Adapter by Workload Profile” section based on perf results. |
tests/test_perf_cli.py |
Asserts iteration_policy is present in emitted perf metadata. |
tests/test_perf_workloads.py |
Adds coverage for workload size standardization + phase attribution presence. |
tests/test_dashboard.py |
Adds coverage for the new dashboard section rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| op_count = len(_cells_from_range(str(workload["range"]))) | ||
| except (TypeError, ValueError, KeyError): | ||
| op_count = 0 |
There was a problem hiding this comment.
_standardize_workload_size computes op_count via len(_cells_from_range(...)), which materializes a list of every cell address. For large ranges this adds significant CPU/memory overhead outside the timed benchmark. Consider computing the cell count from the start/end coords (rows*cols) without enumerating all cells, and when range parsing fails fall back to test_case count (or an explicit "unknown" size) instead of treating it as 0 (=> "small").
| op_count = len(_cells_from_range(str(workload["range"]))) | |
| except (TypeError, ValueError, KeyError): | |
| op_count = 0 | |
| range_str = str(workload["range"]) | |
| start_cell, end_cell = _split_range(range_str) | |
| start_row, start_col = _cell_to_coord(start_cell) | |
| end_row, end_col = _cell_to_coord(end_cell) | |
| row_count = abs(end_row - start_row) + 1 | |
| col_count = abs(end_col - start_col) + 1 | |
| op_count = row_count * col_count | |
| except (TypeError, ValueError, KeyError): | |
| case_count = len(getattr(test_file, "test_cases", []) or []) | |
| op_count = case_count |
| } | ||
|
|
||
| for entry in results: | ||
| size = str(entry.get("workload_size") or "small").strip().lower() |
There was a problem hiding this comment.
Defaulting missing workload_size to "small" will misclassify older perf/results.json files (or other producers) that don't include this new field, which can make the "Best Adapter by Workload Profile" table misleading. Consider skipping entries without workload_size or inferring size from op_count/range/feature instead of forcing them into "small".
| size = str(entry.get("workload_size") or "small").strip().lower() | |
| raw_size = entry.get("workload_size") | |
| if not raw_size: | |
| # Skip entries without an explicit workload_size to avoid | |
| # misclassifying older perf/results.json files. | |
| continue | |
| size = str(raw_size).strip().lower() |
| def run_perf( | ||
| test_dir: Path, | ||
| *, | ||
| adapters: list[Any] | None = None, | ||
| features: list[str] | None = None, | ||
| profile: str = "xlsx", | ||
| warmup: int = 3, | ||
| iters: int = 25, | ||
| iteration_policy: str = "fixed", | ||
| breakdown: bool = False, | ||
| ) -> PerfResults: |
There was a problem hiding this comment.
The PR description is currently a placeholder and doesn’t describe the intended behavior/user impact of adding iteration_policy, workload_size, phase attribution, and the new dashboard section. Please update the PR description with a brief summary and any compatibility notes (e.g., how older perf results without workload_size are handled).
wolfiesch
changed the title
Codex generated this pull request, but encountered an unexpected error after generation. This is a placeholder PR message.
Codex Task
Greptile Overview
Greptile Summary
Adds workload profiling and phase attribution to performance benchmarks. Introduces
--iteration-policyCLI option (currently only supports "fixed"), classifies workloads into small/medium/large based on cell count, and tracks phase-level attribution (parse/write/verify) for operations.Major changes:
workload_sizefield onPerfFeatureResultwith small/medium/large classificationphase_attribution_msfield tracking parse/write/verify breakdownIssues found:
Confidence Score: 3/5
src/excelbench/results/dashboard.py- fix the duplicate row bug before mergingImportant Files Changed
--iteration-policyCLI option with validation; minor style issue with redundant type checkiteration_policyto markdown config output with safe default valueiteration_policyto HTML dashboard metadata displayiteration_policyis persisted in output JSONFlowchart
flowchart TD A[CLI: perf command] -->|iteration_policy param| B[run_perf] B -->|validate policy| C{policy == 'fixed'?} C -->|no| D[ValueError] C -->|yes| E[Execute benchmarks] E -->|for each workload| F[_standardize_workload_size] F -->|count cells| G{Classify size} G -->|<= 1000| H[small] G -->|<= 10000| I[medium] G -->|> 10000| J[large] E -->|for each iteration| K[_measure_*_iteration] K -->|breakdown data| L[_phase_attribution_from_measurement] L -->|read op| M[parse + verify phases] L -->|write op| N[write phase] E -->|aggregate| O[PerfFeatureResult with workload_size + phase_attribution_ms] O -->|results| P[render_perf_results] P -->|generate| Q[Dashboard with workload profile table] Q -->|display| R[Best adapter by size]Last reviewed commit: 4f6ab30