- Notifications
You must be signed in to change notification settings - Fork1k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
scores: dict[str, EvaluationResult[int | float]] | ||
labels: dict[str, EvaluationResult[str]] | ||
assertions: dict[str, EvaluationResult[bool]] |
There was a problem hiding this comment.
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__
.)
Docs Preview
|
@dmontagu To not lose the 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 between |
dmontagu commentedMay 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 be If peopleare making serious use of |
5f4661f
intomainUh oh!
There was an error while loading.Please reload this page.
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 of
model_*
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.)