Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated ModelMetrics merging logic into a shared merge_model_metrics(base, additional) helper to ensure consistent, non-mutating aggregation across the codebase.
Changes:
- Added
merge_model_metrics()helper to centralize model-metrics merging behavior. - Updated
parser.pyandreport.pyto delegate merging to the shared helper. - Added dedicated unit tests for
merge_model_metrics().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/copilot_usage/models.py |
Introduces merge_model_metrics() helper for non-mutating metric aggregation. |
src/copilot_usage/parser.py |
Replaces inline shutdown-cycle metric merge loop with merge_model_metrics(). |
src/copilot_usage/report.py |
Replaces _aggregate_model_metrics loop body with merge_model_metrics(). |
tests/copilot_usage/test_models.py |
Adds TestMergeModelMetrics coverage for empty/overlap/disjoint/non-mutation cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Extract duplicated ModelMetrics merging logic from parser.py and report.py into a single merge_model_metrics() function in models.py. - Add merge_model_metrics() that returns a new dict without mutating either input - Replace inline merging in build_session_summary() (parser.py) - Replace _aggregate_model_metrics body (report.py) with calls to the shared helper - Add unit tests: empty dicts, single entry, overlapping keys, disjoint keys, all token fields accumulate, no-mutation guarantees Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces manual ModelMetrics reconstruction with model_copy(deep=True) and in-place field accumulation. Keeps copy aligned with model schema and reduces allocations for overlapping keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
263eca4 to
bc3d7da
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #47
Problem
ModelMetricsdict merging was implemented twice with divergent approaches:parser.py(build_session_summary): Creates newModelMetrics/RequestMetrics/TokenUsageobjects on every merge — safe but verbose.report.py(_aggregate_model_metrics): Usesmodel_copy()on first insertion, then mutates the copy in-place — fragile if copy is ever removed.Any future bug fix or field addition (e.g. a new token type on
TokenUsage) had to be applied in both places.Solution
Extracted a shared
merge_model_metrics(base, additional)helper inmodels.pythat:Both
parser.pyandreport.pynow delegate to this helper.Changes
src/copilot_usage/models.pymerge_model_metrics()functionsrc/copilot_usage/parser.pymerge_model_metrics()callsrc/copilot_usage/report.py_aggregate_model_metricsbody withmerge_model_metrics()calltests/copilot_usage/test_models.pyTestMergeModelMetricswith 7 test casesTests
New
TestMergeModelMetricsclass covers:baseinputadditionalinputExisting
test_parser.pyandtest_report.pytests (includingTestAggregateModelMetrics) remain unchanged and validate regression.Warning
The following domains were blocked by the firewall during workflow execution:
astral.shconda.anaconda.orgpypi.orgrepo.anaconda.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.