feat: Add RecordModel base and rename readers/writers to Model*#73
feat: Add RecordModel base and rename readers/writers to Model*#73msto wants to merge 3 commits into
RecordModel base and rename readers/writers to Model*#73Conversation
📝 WalkthroughWalkthroughIntroduces ChangesRecord model and serialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ModelReader
participant RecordModel
participant ModelWriter
Caller->>ModelReader: open(model_class, path)
ModelReader->>RecordModel: validate each parsed row
ModelReader-->>Caller: yield records
Caller->>ModelWriter: open(model_class, path)
ModelWriter->>RecordModel: serialize model fields
ModelWriter-->>Caller: write delimited output
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| machinery; use `Metric` when you want the batteries-included behaviors. `ModelReader` and | ||
| `ModelWriter` operate on any `RecordModel` subclass. | ||
|
|
||
| Subclasses should define their fields using Pydantic field annotations. |
There was a problem hiding this comment.
Add a note to the docstring:
T | None fields do not round-trip None: it serializes to an empty cell and reads back as "". Use Metric (or a NullSentinels mixin) if you need None preserved."
| def read( | ||
| cls, | ||
| path: Path | str, | ||
| # NB: these defaults mirror `ModelReader.open()`; keep them in sync. |
There was a problem hiding this comment.
Could you add module-level constants for these defaults?
| - Introduce `MetricReader` for iterating metrics from any text-IO source (#41) | ||
| - Add an `encoding` kwarg on `MetricReader.open` and `MetricWriter.open` (#43) | ||
| - Add `RecordModel`, a mixin-free base class providing the default tabular read/write surface without the `fgmetric.converters` mixins; `Metric` now subclasses it (#10) | ||
| - Introduce `ModelReader` for iterating records from any text-IO source (#41) |
There was a problem hiding this comment.
Consider adding **Breaking:** prefixes where relevant.
| return [ | ||
| info.serialization_alias or info.alias or name | ||
| for name, info in cls.model_fields.items() | ||
| ] |
There was a problem hiding this comment.
The _header_fieldnames method resolution order contract is enforced only by a comment. The NB block in _counter_pivot_table.py is excellent, but the invariant ("any header-overriding converter must inherit RecordModel and precede it in MRO, else super() resolves to BaseModel") is load-bearing and unguarded. If violated, super()._header_fieldnames() would raise AttributeError only when a writer is opened.
Proposed fix from Claude:
fgmetric/record_model.py— split the default header into a@finalhelper
Replace the current _header_fieldnames with a thin overridable method that delegates to a non-overridable helper:
from typing import final # add to imports
@classmethod
def _header_fieldnames(cls) -> list[str]:
"""
Return the fieldnames to use as a header row when writing records to a file.
This method is used by `ModelWriter` to construct the underlying `csv.DictWriter`.
It returns the fieldnames that will appear in serialized output, which may differ from
the model's field names when aliases are used.
Subclasses and mixins (e.g., `CounterPivotTable`) may override this method to adjust the
header to match a custom serializer. Such overrides should build on
`_default_header_fieldnames` (the plain field-per-column header) rather than `super()`,
so they do not depend on their position in the model's MRO.
Note:
This method is deliberately not used during reading/validation; see
`ModelReader` for the headerless-input path.
Returns:
The list of fieldnames to use as the header row.
"""
# TODO: support returning the set of fields that would be constructed if the class has a
# custom model serializer
return cls._default_header_fieldnames()
@final
@classmethod
def _default_header_fieldnames(cls) -> list[str]:
"""
Return the plain field-per-column header: one column per field, in declaration order.
Each field resolves to the key it serializes under (its alias, when one is set), so the
header matches the keys produced by `model_dump(by_alias=True)`. This method is `@final`
so header-rewriting mixins can call it directly instead of routing through `super()`,
which would otherwise depend on MRO ordering.
"""
return [
info.serialization_alias or info.alias or name
for name, info in cls.model_fields.items()
]fgmetric/converters/_counter_pivot_table.py— call the helper, notsuper()
In the override, change one line:
default_header = cls._default_header_fieldnames()
And trim the now-stale half of the class-level NB comment. It currently says the inheritance exists so it can "call super() to reuse the default … header" and that a future override "must … appear before it in the model's MRO (so super() resolves to the base header, not BaseModel)." After the fix that ordering trap is gone — replace with:
# NB: This mixin inherits `RecordModel` rather than `BaseModel` (unlike the other converters) so
# that `_default_header_fieldnames` is in scope for its `_header_fieldnames` override. It is the
# only converter that rewrites the on-disk column set. Any future converter that also overrides
# `_header_fieldnames` should build on `_default_header_fieldnames` (not `super()`) so the result
# does not depend on MRO ordering.
Extract a mixin-free RecordModel base out of Metric (closes #10), and rename MetricReader/MetricWriter to ModelReader/ModelWriter so the IO classes operate on any RecordModel. Metric is behaviorally unchanged: it subclasses RecordModel plus the same converter mixins in the same MRO order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to code review: consolidate CounterPivotTable._header_fieldnames onto a single _counter_fieldname guard (matching the file's assert-narrowing idiom) and document the MRO/inheritance contract for future header-overriding converters. Add a README section introducing RecordModel and when to use it over Metric. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split `RecordModel._header_fieldnames` into a thin overridable method plus a `@final` `_default_header_fieldnames` helper, so header-rewriting mixins (`CounterPivotTable`) build on the helper rather than `super()` and no longer depend on MRO ordering; the old MRO contract was enforced only by a comment. Also document that `T | None` fields do not round-trip `None` on the mixin-free base, extract the shared read-path `DEFAULT_ENCODING`, correct the misleading "mirrors ModelReader.open" comment on `read` (its "\t" delimiter default does not infer from the file extension), and mark the breaking CHANGELOG entries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fgmetric/model_writer.py (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the
RecordModelimport likemodel_reader.pydoes.This import is only used for the
[T: RecordModel]generic bound and docstrings, not at runtime.fgmetric/model_reader.pydefers the identical import (apparently behindTYPE_CHECKING), since PEP 695 type-parameter bounds are lazily evaluated and don't need the name available at class-definition time. Making this import unconditional here is inconsistent with that sibling file and sets up a latent circular-import trap ifRecordModelever grows a symmetricwrite()classmethod (mirroring its existingread()classmethod that already importsModelReader).♻️ Proposed fix
-from fgmetric.record_model import RecordModel +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from fgmetric.record_model import RecordModel🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fgmetric/model_writer.py` at line 13, Guard the RecordModel import in model_writer.py using the same TYPE_CHECKING pattern as model_reader.py. Keep RecordModel available for the generic bound and docstrings during static analysis, while preventing the unconditional runtime import and preserving existing writer behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@fgmetric/model_writer.py`:
- Line 13: Guard the RecordModel import in model_writer.py using the same
TYPE_CHECKING pattern as model_reader.py. Keep RecordModel available for the
generic bound and docstrings during static analysis, while preventing the
unconditional runtime import and preserving existing writer behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 503fcaa7-5747-4fb4-bc05-8bed9d082bb8
📒 Files selected for processing (16)
CHANGELOG.mdREADME.mdfgmetric/__init__.pyfgmetric/converters/_counter_pivot_table.pyfgmetric/metric.pyfgmetric/model_reader.pyfgmetric/model_writer.pyfgmetric/record_model.pytests/benchmarks/test_benchmark_runtime.pytests/test_collections.pytests/test_compression.pytests/test_metric.pytests/test_model_reader.pytests/test_model_writer.pytests/test_record_model.pytests/test_roundtrip.py
4 participants
Summary
Closes #10.
This PR extracts a base
RecordModelwith no default mixins, leavingMetricwith the converter behaviors enabled by default in fgpyo.The file IO classes are renamed
MetricReader/MetricWriter→ModelReader/ModelWriterand now read/write anyRecordModel. (We could add a thin alias, optionally with a deprecation warning, but I think it's cleaner to set the API now.)Summary by CodeRabbit
New Features
RecordModelfor plain tabular record parsing and serialization.ModelReaderandModelWriterfor streaming reads and writes.Breaking Changes
MetricReaderandMetricWriterwithModelReaderandModelWriter.Metric.read()now eagerly returns a list and supports string paths and encoding options.