add LLMEvaluator to JS #1186
add LLMEvaluator to JS #1186
Conversation
| import * as uuid from "uuid"; | ||
| import { EvaluationResult, EvaluationResults, RunEvaluator } from "./evaluator.js"; | ||
| import type { Run, Example } from "../schemas.js"; | ||
| export class CategoricalScoreConfig { |
There was a problem hiding this comment.
We should add good typedocs :)
There was a problem hiding this comment.
tried my best, lmk what needs improving
js/src/evaluation/llm_evaluator.ts
Outdated
| properties.score = { | ||
| type: "string", | ||
| enum: scoreConfig.choices, | ||
| description: `The score for the evaluation, one of ${scoreConfig.choices.join(", ")}.`, |
There was a problem hiding this comment.
This is really confusing to me tbh.
Why do we put categorical in the numeric score field? Why call it a score? Why not call it "category" or something obvious?
Score implies sortability / comparability to me
There was a problem hiding this comment.
I changed the key to "value", but we also do this in the output parsing which I think solves this a little? not 100% sure, lmk what you think:
if ("choices" in this.scoreConfig) {
const value = output.score;
const explanation = output[this.reasoningKey];
return {
key: this.scoreConfig.key,
value,
comment: explanation,
sourceRunId: output.runId
};
}
js/package.json
Outdated
| @@ -106,14 +106,15 @@ | |||
| "p-queue": "^6.6.2", | |||
| "p-retry": "4", | |||
| "semver": "^7.6.3", | |||
| "uuid": "^10.0.0" | |||
| "uuid": "^10.0.0", | |||
| "langchain": "^0.3.3", | |||
There was a problem hiding this comment.
We can't make langchain a dependency of langsmith - it would be cyclic and not all langsmith users use langchain.
I believe we'll have to set them as peerDependencies
There was a problem hiding this comment.
Synced with Jacob to double check. Steps to handle this:
- Rm from dependencies
- Make the llm evaluator a separate entrypoint (see traceablea nd the create_entrypoints.js script)
- We should probably refrain from importing init chat model from the top since that requires langchain to be installed as well as the sub modules. Maybe accept a chat model as an argument and have them initialize, and in our docs we show using that. Core I think more OK
| "openai": "^4.67.3", | ||
| "prettier": "^2.8.8", | ||
| "ts-jest": "^29.1.0", | ||
| "ts-node": "^10.9.1", | ||
| "typescript": "^5.4.5", | ||
| "zod": "^3.23.8" | ||
| "zod": "^3.23.8", | ||
| "@langchain/core": "^0.3.14", |
There was a problem hiding this comment.
Good to keep these sorted and avoid touching unnecessary lines
| @@ -12,6 +12,7 @@ const entrypoints = { | |||
| traceable: "traceable", | |||
| evaluation: "evaluation/index", | |||
| "evaluation/langchain": "evaluation/langchain", | |||
| "evaluation/llm": "evaluation/llm_evaluator", | |||
There was a problem hiding this comment.
Naming should match filepath
So rename file below to evaluation/llm.ts
| ) { | ||
| try { | ||
| // Store the configuration | ||
| this.scoreConfig = scoreConfig; |
There was a problem hiding this comment.
More conventional to make this a constructor instead of a factory method
| return variables; | ||
| } | ||
|
|
||
| private parseOutput( |
There was a problem hiding this comment.
Prefer protected instead of private to make subclassing possible
|
|
||
| constructor() {} | ||
|
|
||
| static async create(params: LLMEvaluatorParams): Promise<LLMEvaluator> { |
There was a problem hiding this comment.
You definitely shouldn't have two methods for this, ideally you just use the constructor
| mapVariables?: (run: Run, example?: Example) => Record<string, any>; | ||
| } | ||
|
|
||
| export class LLMEvaluator implements RunEvaluator { |
|
|
||
| export class LLMEvaluator implements RunEvaluator { | ||
| prompt: any; | ||
| mapVariables?: (run: Run, example?: Example) => Record<string, any>; |
There was a problem hiding this comment.
What are mapVariables?
| max: 1, | ||
| }), | ||
| chatModel: CHAT_MODEL, | ||
| mapVariables: (run: any, _example?: any) => ({ |
There was a problem hiding this comment.
Is there a better name for this we can use? mapVariables sounds like a map of variables or something and .map() has a specific meaning in JS (and most languages?)
| interface LLMEvaluatorParams { | ||
| promptTemplate: string | [string, string][]; | ||
| scoreConfig: ScoreConfig; | ||
| chatModel: BaseLanguageModel; |
There was a problem hiding this comment.
Should be BaseChatModel
No description provided.