refactor: convert ModelMetrics, RequestMetrics, TokenUsage to frozen dataclasses (#1066)#1068
refactor: convert ModelMetrics, RequestMetrics, TokenUsage to frozen dataclasses (#1066)#1068
Conversation
…dataclasses (#1066) Replace mutable Pydantic BaseModel subclasses with frozen dataclasses (frozen=True, slots=True) for TokenUsage, RequestMetrics, and ModelMetrics. These are internal accumulator structs with no Pydantic validators, making them candidates for plain frozen dataclasses per the coding guidelines. Changes: - TokenUsage, RequestMetrics, ModelMetrics: BaseModel → frozen dataclass - add_to_model_metrics: returns new ModelMetrics instead of mutating - copy_model_metrics: uses dataclasses.replace (shallow copy sufficient) - All 5 call sites updated to capture return value - Tests updated: model_fields → dataclasses.fields, model_dump → asdict - Added regression test verifying frozen instances reject mutation - Updated implementation.md docs to reflect immutable semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors internal model-metric accumulator types in copilot_usage.models from mutable Pydantic models to immutable frozen/slot dataclasses, updating accumulation helpers and downstream call sites to avoid in-place mutation (and potential cache corruption).
Changes:
- Converted
TokenUsage,RequestMetrics, andModelMetricsto@dataclasses.dataclass(frozen=True, slots=True). - Updated
add_to_model_metricsto return a newModelMetricsand adjusted call sites in parser/report to capture the return value. - Updated unit tests and implementation docs to reflect the new immutable semantics.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/copilot_usage/models.py | Converts metric structs to frozen dataclasses and makes accumulation return new instances. |
| src/copilot_usage/parser.py | Updates metric aggregation to capture add_to_model_metrics return value. |
| src/copilot_usage/report.py | Updates aggregation call site and adjusts docstring wording for immutable accumulation. |
| tests/copilot_usage/test_models.py | Migrates tests from Pydantic APIs to dataclasses utilities; adds immutability regression checks. |
| src/copilot_usage/docs/implementation.md | Updates docs to reflect non-mutating accumulation semantics. |
| @@ -112,46 +114,47 @@ class TokenUsage(BaseModel): | |||
| cacheWriteTokens: int = 0 | |||
|
|
|||
|
|
|||
| class RequestMetrics(BaseModel): | |||
| @dataclasses.dataclass(frozen=True, slots=True) | |||
| class RequestMetrics: | |||
| """Request count and cost for a single model.""" | |||
|
|
|||
| count: int = 0 | |||
| cost: int = 0 | |||
|
|
|||
|
|
|||
| class ModelMetrics(BaseModel): | |||
| @dataclasses.dataclass(frozen=True, slots=True) | |||
| class ModelMetrics: | |||
| """Combined request + usage metrics for one model.""" | |||
|
|
|||
| requests: RequestMetrics = Field(default_factory=RequestMetrics) | |||
| usage: TokenUsage = Field(default_factory=TokenUsage) | |||
| requests: RequestMetrics = dataclasses.field(default_factory=RequestMetrics) | |||
| usage: TokenUsage = dataclasses.field(default_factory=TokenUsage) | |||
There was a problem hiding this comment.
TokenUsage/RequestMetrics/ModelMetrics are no longer Pydantic models, so they no longer have .model_dump()/.model_fields/.model_copy(). There is still at least one usage of ModelMetrics.model_dump() in the test suite (tests/copilot_usage/test_report.py:5723), which will now fail. Please update those remaining call sites to use dataclasses.asdict() (or another equivalent serialization helper) instead of Pydantic APIs.
| def copy_model_metrics(mm: ModelMetrics) -> ModelMetrics: | ||
| """Create an independent copy of *mm* via explicit construction. | ||
| """Create an independent copy of *mm*. | ||
|
|
||
| Builds new ``ModelMetrics``/``RequestMetrics``/``TokenUsage`` instances | ||
| instead of using Pydantic's ``model_copy(deep=True)`` which delegates to | ||
| ``copy.deepcopy`` and is significantly slower for simple int fields. | ||
| With frozen dataclasses every instance is already immutable, so a | ||
| shallow ``dataclasses.replace`` (no field overrides) is sufficient — | ||
| the nested ``RequestMetrics`` and ``TokenUsage`` are themselves frozen | ||
| and can safely be shared. | ||
| """ |
There was a problem hiding this comment.
copy_model_metrics() now returns dataclasses.replace(mm), which creates a new ModelMetrics instance but intentionally shares the nested RequestMetrics/TokenUsage references. Since everything is frozen this is safe, but the docstring still says “independent copy”, which reads like a deep copy. Consider rewording to “shallow copy” (or explicitly mention nested objects are shared) to avoid misleading future callers.
| Each unique model name produces a single accumulated ``ModelMetrics`` | ||
| instance via immutable addition. |
There was a problem hiding this comment.
This docstring says each model name produces “a single accumulated ModelMetrics instance”. With immutable accumulation, every additional merge creates a new ModelMetrics object and reassigns the dict entry. Consider clarifying that the result dict contains one final accumulated instance per model, even though intermediate instances are created during aggregation.
| Each unique model name produces a single accumulated ``ModelMetrics`` | |
| instance via immutable addition. | |
| The returned dict contains one final accumulated ``ModelMetrics`` | |
| value per unique model name. Accumulation uses immutable addition, | |
| so intermediate merged instances may be created while aggregating. |
- Fix ruff format violations: break long lines in models.py and parser.py - Fix test_report.py: replace model_dump() with dataclasses.asdict() for ModelMetrics which is now a frozen dataclass Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI Fix AppliedFixed the following CI failures:
Warning
|
- models.py: Reword copy_model_metrics docstring from 'independent copy' to 'shallow copy' and clarify nested references are shared - report.py: Clarify _aggregate_model_metrics docstring to note intermediate instances during immutable accumulation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
microsasa
added
aw-review-response-3
|
❌ Pipeline orchestrator: review-response loop reached 5 rounds. Marking as stuck for human review. |
Closes #1066
Summary
Replace mutable Pydantic
BaseModelsubclasses with@dataclasses.dataclass(frozen=True, slots=True)forTokenUsage,RequestMetrics, andModelMetrics. These are internal accumulator structs with no Pydantic validators or special features, making them a violation of the coding guideline: "Internal intermediate state uses frozendataclasseswithslots=True."Changes
src/copilot_usage/models.pyTokenUsage,RequestMetrics,ModelMetrics:BaseModel→@dataclasses.dataclass(frozen=True, slots=True)add_to_model_metrics: Changed from in-place mutation (→ None) to returning a newModelMetricsinstance (→ ModelMetrics), preventing silent cache corruptioncopy_model_metrics: Simplified to usedataclasses.replace()— with frozen dataclasses, shallow copy is sufficient since nested types are also frozenmerge_model_metrics: Updated to capture return value fromadd_to_model_metricsCall site updates
src/copilot_usage/parser.py(line 923): Capture return valuesrc/copilot_usage/report.py(line 272): Capture return value + updated docstringTest updates (
tests/copilot_usage/test_models.py)model_fields→dataclasses.fields(),model_dump()→dataclasses.asdict()test_neither_operand_mutated: Verifies bothtargetandsourceare unchanged (regression test)test_frozen_prevents_mutation: VerifiesFrozenInstanceErroron attribute assignmenttest_accumulates_incrementally: Updated to capture return valuesfrozen=True)test_no_deep_copy_regression(removedmodel_copypatch)Documentation
implementation.mdto reflect immutable accumulation semanticsWarning
The following domains were blocked by the firewall during workflow execution:
astral.shindex.crates.iopypi.orgreleaseassets.githubusercontent.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.