Include report averages #3053
Include report averages #3053
Conversation
Docs Preview
|
| ) | ||
| if (averages := report.averages()) is not None and averages.assertions is not None: | ||
| experiment_metadata = {'n_cases': len(self.cases), 'averages': averages} | ||
| eval_span.set_attribute('experiment.metadata', experiment_metadata) |
There was a problem hiding this comment.
Can you add a logfire. prefix here since it's not general OTel but specific to our platform?
There was a problem hiding this comment.
I'd argue it's specific to pydantic evals, rather than logfire itself?
We use the experiment.metadata field to ingest information about the test run. Are you saying we should change this to logfire.experiment.metadata ? or are you saying underneath experiment.metadata we should have a logfire field.
There was a problem hiding this comment.
@cetra3 Yeah I was suggesting (based on @alexmojaki suggestion in Slack) to change it to logfire.experiment.metadata. But I see your (implied) point that even though right now only Logfire will use these attributes, they could be useful to users sending pydantic_evals data to different platform as well.
@alexmojaki What do you think about a pydantic_evals prefix?
There was a problem hiding this comment.
Everything here is specific to pydantic evals. This is particularly specific not just to logfire but to fusionfire popped attributes. n_cases is being repeated here so that it can be displayed cheaply in the frontend. We may add or remove things here freely depending on what the frontend needs, it's not intended to be queried. I think logfire. makes sense for that.
| trace_id=trace_id, | ||
| ) | ||
| if (averages := report.averages()) is not None and averages.assertions is not None: | ||
| experiment_metadata = {'n_cases': len(self.cases), 'averages': averages} |
There was a problem hiding this comment.
From the test below, it looks like we already had n_cases in here, do we need to repeat it? Or could we drop it there and have it just under the metadata?
| 'experiment.metadata': { | ||
| 'type': 'object', | ||
| 'properties': { | ||
| 'averages': { |
There was a problem hiding this comment.
I'd expect n_cases to also be listed here
| 'logfire.msg': 'evaluate mock_async_task', | ||
| 'experiment.metadata': { | ||
| 'n_cases': 2, | ||
| 'averages': { |
There was a problem hiding this comment.
If we're going to be using this on the frontend, I don't know if it's wise to pass along the entire ReportCaseAggregate object or just the subset we need
Allows the evals page in logfire to present some nice stats in the main comparison table: