Repository navigation
Strip model name prefix for all model types - #142
vmaksimovicTT wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes model-name normalization in the benchmark data mappers so that ml_model_name consistently stores the short catalog name (stripping the leading org/ segment) across all run types (benchmarks, summaries, evals, acceptance_criteria, and forge).
Changes:
- Introduce a shared
_format_model_namehelper on the base mapper and route additional run-type paths through it. - Update Shield mapper paths (benchmarks, benchmarks_summary, evals, acceptance_criteria) and Forge mapper to store the normalized model name.
- Expand/adjust unit tests to pin model-name normalization behavior across run types and edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/actions/collect_data/src/benchmark.py | Adds a base model-name normalizer and uses it across mappers/run types (including Forge). |
| .github/actions/collect_data/test/test_benchmark_mapper.py | Updates existing tests and adds regression coverage to ensure all run types store the normalized (short) model name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/actions/collect_data/src/benchmark.py:213
- ForgeBenchmarkDataMapper.map_benchmark_data is annotated as returning a single CompleteBenchmarkRun, but the implementation returns a list (see the
return [benchmark_run]). This mismatch can confuse callers and breaks static typing; update the return annotation to a list to match actual behavior.
class ForgeBenchmarkDataMapper(_BenchmarkDataMapper):
def map_benchmark_data(self, pipeline, job_id, report_data, model_spec_data=None) -> CompleteBenchmarkRun | None:
job = next((job for job in pipeline.jobs if job.github_job_id == job_id), None)
map_benchmark_data returns a list of CompleteBenchmarkRun (or None), but the base class, ForgeBenchmarkDataMapper, and ShieldBenchmarkDataMapper annotated it as `CompleteBenchmarkRun | None`. Align all signatures with the actual contract (and with VllmBenchmarkDataMapper / GuideLLMBenchmarkDataMapper) as `List[CompleteBenchmarkRun] | None`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@vmaksimovicTT I suppose this PR should be discarded per latest comments on #tt-inference-server channel |
Thanks for pointing that out, im closing it |
There is a function which strips a prefix from model's name, but it was used only for some models/reports. This PR expands the usage of the
_format_model_name