Add Coherence metric

[davidgisbey]

Description

This ports the Coherence metric from the evaluation repo to our Ruby codebase.

It:

• adding the metric to lib/auto_evaluation
• adds a rake task which can be used to output an evaluation result for coherence for a given input
• adds the RunMetricEvaluation which handles, building an answer and running a metric on that answer
• adds schema validation against the tool output

Rake task

Result

Trello card

https://trello.com/c/cUIagBUx/2996-ruby-auto-eval-for-coherence-metric

[kevindew]

Looks great, mostly minor comments but with a potentially bigger suggestion on an abstraction for the Rake task

[davidgisbey]

Thanks for the review @kevindew. I made those changes. The last 2 commits implement the rake task and schema validation.

[kevindew]

Looks great, just a bit of tweaking with the code to run the task. If easier we could do that on separate PR but I think it should be ok.

[davidgisbey]

@kevindew i made those changes. I renamed the new class to AutoEvaluation::RunEvaluation but i think the naming is too generic. I wonder if coupling it to ScoreResults is the way to go. Something like:

AutoEvaluation::ScoreResultEvaluation
AutoEvaluation::AnswerScoreResultEvaluation
AutoEvaluation::GenerateAutoEvaluationScoreResult
AutoEvaluation::GenerateAutoEvaluationFromInput

Or we could go down a slightly more explicit route for what it's actually doing

AutoEvaluation::GenerateAndEvaluateAnswer
AutoEvaluation::EvaluateGeneratedAnswer

Another option is to just add a metrics directory as you previously suggested.

Then AutoEvaluation::EvaluateMetric or similar works fine.

Wdyt?

[kevindew]

Thanks, just dropping in between meetings to reply to the question.

Yeah agree it sounds quite generic. I'm not sure it needs to reference score though as I don't think there's something coupled to ScoreResults here (unless I've missed something) instead I think the identifying aspect of this is that you have an input of a question, which gets turned into a basic answer then that evaluated.

So I think it's something like EvaluateAnswerFromQuestionMessage ?

[davidgisbey]

Great i've gone with EvaluateAnswerFromQuestionMessage

[kevindew]

Just a few super minor things then good to merge.

[davidgisbey]

@kevindew thanks for that. I've made those changes in this commit.

[kevindew]

You're missing the new on this new(**attributes) so this will be just returning a hash.

While this is just minor it did make me wonder should something have failed for this to be creating the wrong class?

[davidgisbey]

oh yes sorry. Yea i would've expected some tests to fail 🤔

Btw using to_d does this

Not a big deal, but we might still want to round in the UI if it causes issues when rendering.

[davidgisbey]

Ah the test just checks that the evaluation result is returned

  let(:score_result) { build(:auto_evaluation_score_result) }

      before do
        allow(evaluation_class)
          .to receive(:call)
          .and_return(score_result)
      end


     it "returns the AutoEvaluation::ScoreResult generated by the evaluation class" do
    result = described_class.call(
      evaluation_class: evaluation_klass,
      question_message:,
    )
    expect(result).to eq(score_result)
  end

It would've blown up on any integration test. I've got some in the next PR that would've failed.

The coherence class uses a stubbed response to build the runs not an evaluation result .

[kevindew]

Great thanks for digging into that.

For the decimal concern I think when we render them they'll only come from the DB and I'd have thought (but I haven't confirmed it) that as we have the decimal type in the DB they'll also be decimals not floats, so I think we'd have this problem either way? Unless I've missed something

[kevindew]

Nice one - sorry this dragged on