Skip to content

Commit

Permalink
Fix bug in evaluation report (#303)
Browse files Browse the repository at this point in the history
CellsMatchResult can be nil. I think this is the result of default
values not being serialized in the protojson.

Update the results manager to include default values when serializing to
JSON. This should make SQL queries easier.
  • Loading branch information
jlewi authored Oct 16, 2024
1 parent 6f3a967 commit 83f7712
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 3 deletions.
14 changes: 12 additions & 2 deletions app/pkg/eval/evaluator.go
Original file line number Diff line number Diff line change
Expand Up @@ -640,9 +640,19 @@ func (e *Evaluator) buildExperimentReport(ctx context.Context, name string, mana
r.CellsMatchCounts = make(map[string]int32)

for _, c := range counts {
if c.MatchResult == nil {
// N.B. I think for unknown it ends up being a nil value. I suspect this is because default values are
// elided when marshalling to JSON. We should fix that by changing the JSON serialization.
key := v1alpha1.CellsMatchResult_UNKNOWN_CellsMatchResult.String()
if _, ok := r.CellsMatchCounts[key]; !ok {
r.CellsMatchCounts[key] = 0
}
r.CellsMatchCounts[key] = r.CellsMatchCounts[key] + int32(c.Count)
continue
}
s, ok := c.MatchResult.(string)
if !ok {
return r, errors.Wrapf(err, "Failed to convert cellsMatchResult to string")
return r, errors.New("Failed to convert cellsMatchResult to string")
}
r.CellsMatchCounts[s] = int32(c.Count)
}
Expand Down Expand Up @@ -679,7 +689,7 @@ func (e *Evaluator) buildExperimentReport(ctx context.Context, name string, mana
}
}

percentiles, err := computePercentilesOfInts(generateTimes, []float64{.9, .95})
percentiles, err := computePercentilesOfInts(generateTimes, []float64{.5, .75, .9, .95})
if err != nil {
return r, errors.Wrapf(err, "Failed to compute percentiles")
}
Expand Down
6 changes: 5 additions & 1 deletion app/pkg/eval/results_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,11 @@ func (m *ResultsManager) ListResults(ctx context.Context, cursor *time.Time, pag
}

func protoToRowUpdate(result *v1alpha1.EvalResult) (*fsql.UpdateResultParams, error) {
protoJson, err := protojson.Marshal(result)
// Emit default values. Otherwise SQL queries become more complex.
opts := protojson.MarshalOptions{
EmitDefaultValues: true,
}
protoJson, err := opts.Marshal(result)
if err != nil {
return nil, errors.Wrapf(err, "Failed to serialize EvalResult to JSON")
}
Expand Down

0 comments on commit 83f7712

Please sign in to comment.