Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

BREAKING CHANGE: MakeEvaluationReport andReportCase into generic dataclasses#1799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
dmontagu merged 2 commits intomainfromdmontagu/make-reports-generic
Jul 8, 2025

Conversation

dmontagu
Copy link
Contributor

Right now, these types are (non-generic) BaseModels. We received a request in the public slack to have these types be generic. As I noted there, it's much more difficult to do that with BaseModel than with a dataclass, so I converted the types into dataclasses and made things generic.

I'm not100% convinced yet that we should make this change (though I'd be easily convinced if other maintainers consider the consequences and agree with the change), but the changes in this PR make these classes generic so you can have type-checked access to the report case inputs, outputs, and metadata.

The main downside (and reason this is a breaking change) is the loss ofmodel_* methods onEvaluationReport andReportCase. I introducedEvaluationReportAdapter andReportCaseAdapter to make it somewhat easier to serialize these things on your own, but either way this is definitely less ergonomic than BaseModel, it's just harder to deal with generics properly in this way with a generic BaseModel subclass. (Generic BaseModels are more designed to reduce boilerplate when making many concrete instantiations of a parametrized type, and not designed for more type-theoretic applications like what is happening here.)

t94j0 reacted with heart emojidhimmel reacted with eyes emoji
Comment on lines +63 to +65
scores: dict[str, EvaluationResult[int | float]]
labels: dict[str, EvaluationResult[str]]
assertions: dict[str, EvaluationResult[bool]]
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The= field(init=False) currently on these fields is just a straight-up mistake in the current implementation (maybe it was supposed to be there at some point, but I'm 99% sure it shouldn't be there now).

I think the only reason it didn't cause issues was because these objects are generally created by us, and in a way that it was never created without explicitly specifying the value. (Because this is currently aBaseModel, the=field(init=False) doesn't remove the field from the__init__.)

@github-actionsGitHub Actions
Copy link

Docs Preview

commit:6b499f5
Preview URL:https://de8356b4-pydantic-ai-previews.pydantic.workers.dev

@DouweM
Copy link
Contributor

@dmontagu To not lose themodel_dump[_json] methods and maintain backward compatibility (at least of theBaseModel bits we think people are likely to use), would it be crazy to implement them explicitly on the classes in question, delegating to theTypeAdapter?

I could imagine a decorator that works on any dataclass and does just that -- that may be useful more generally to get consistent validation/serialization APIs betweenBaseModels and dataclasses.

@DouweMDouweM assigneddmontagu and unassignedDouweMMay 21, 2025
@dmontagu
Copy link
ContributorAuthor

dmontagu commentedMay 21, 2025
edited
Loading

I thought about that but the reason I didn't do it is because it won't be type-aware (well, specifically, aware of the generic parameters involved; they willalways beAny), so may end up with some surprising behavior. Admittedly it would be compatible with the way things work now, but I also feel it's a bit confusing to havemodel_X methods on a dataclass, it might make you think it's not actually a dataclass. Given we aren't in v1 yet and most people aren't manually serializing the reports, my feeling is it's not unreasonable to make a breaking change here and just make it a proper dataclass now.

If peopleare making serious use ofmodel_dump etc. on the ReportCase / EvaluationReport classes then I might feel differently, but I would be surprised if that was the case and I'm not sure we have a good way to determine that anyway (short of user feedback, which I assume we won't get until breaking their code 😞).

DouweM reacted with thumbs up emoji

@dmontagudmontaguenabled auto-merge (squash)July 8, 2025 18:55
@dmontagudmontagu merged commit5f4661f intomainJul 8, 2025
16 checks passed
@dmontagudmontagu deleted the dmontagu/make-reports-generic branchJuly 8, 2025 19:01
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@DouweMDouweMDouweM approved these changes

Assignees

@dmontagudmontagu

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@dmontagu@DouweM

[8]ページ先頭

©2009-2025 Movatter.jp