Repository navigation
Fix collect_data; route kind:benchmarks sections - #156
Conversation
…ist, add prefix coverage test
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues were identified.
Pull request overview
Updates collect_data to support kind: "benchmarks" sections, broader report formats, and partial-failure handling.
Changes:
- Filters benchmark report filenames.
- Expands metric mapping and validation.
- Routes structured benchmark data through the detailed mapper.
- Tracks failures without blocking report uploads.
File summaries
| File | Summary |
|---|---|
.github/actions/collect_data/test/test_benchmark_mapper.py |
Adds coverage for routing, metrics, discovery, and failure handling. |
.github/actions/collect_data/src/shared.py |
Adds failure-state reset support. |
.github/actions/collect_data/src/generate_data.py |
Resets and reports partial collection failures. |
.github/actions/collect_data/src/benchmark.py |
Implements report filtering, routing, mapping, and validation. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d5c2ca3 to
19d04ef
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate issues affect report determinism and migration correctness.
Review details
Suppressed comments (1)
.github/actions/collect_data/src/benchmark.py:70
- When both
model_spec_*.jsonandruntime_model_spec_*.jsonexist for one job, this whitelist puts both paths into the same report group, butcreate_json_from_reportselects whichever path happens to come first and parses the other as a benchmark report. Becauseos.walkdoes not guarantee that order, the model identity/config can vary between runs; select a canonical spec deterministically and exclude all other spec files from report processing.
"model_spec_",
"runtime_model_spec_",
- Files reviewed: 4/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Deterministically select one model-spec file and skip all model-spec files during benchmark report iteration.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Lite
|
Please fill in the PR description with what is changed, why and how, few sentences is enough, thanks! |
Update benchmark collection to support tt-inference-server’s report format introduced at the end of July, while preserving legacy reports.