feat(test): evals - #1058
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Greptile SummaryThe PR adds the Skybridge eval runner and extracts the Vite integration into
Confidence Score: 5/5The PR appears safe to merge because the previously reported matcher failure is fixed and no blocking failure remains. No blocking failure remains. Reviews (19): Last reviewed commit: "feat(test): assert on what the assistant..." | Re-trigger Greptile |
7466734 to
7aada9d
Compare
34007b8 to
50c10e1
Compare
50c10e1 to
df0cfdd
Compare
| const evals = options?.evals; | ||
| if (evals !== undefined) { | ||
| plugins.push(loadEvalsPlugin(evals)); | ||
| } |
There was a problem hiding this comment.
I don't think it's a good idea to have polymorphic plugins and using skybridge/vite-plugin should "always" package both views folder for build and prepare server for unit test.
Happy to discuss split of concerns between skybridge/test and this package if you want
| * Everything the runner needs is configured here, so a scenario file imports | ||
| * nothing but `start` and its own `AppType`. | ||
| */ | ||
| export function evals(options: EvalsOptions): Plugin { |
There was a problem hiding this comment.
why not put this directly in @skybridge/vite-plugin ?
| throw new Error("The evals plugin needs either `server` or `project`"); | ||
| } | ||
|
|
||
| const server = await startServer(options.project); |
There was a problem hiding this comment.
this should be the responsability of the start method instead to avoid booting the server on test suits that don't need it
There was a problem hiding this comment.
The thing is, moving the server booting the start method means spawning a new server per worker. I agree the server shouldn't start for test suites that don't need it though — probably via a config somewhere?
There was a problem hiding this comment.
Discussed together, we keep server startup in the setup script of the plugin for now, and see how it affects vitest run performances that are not using chat assertions
| * Wires the eval runner into vitest: contributes `setupFiles`, | ||
| * `globalSetup`, `provide` and `testTimeout` from `@skybridge/test`, loaded | ||
| * lazily so production builds never touch it. Set it only in the config | ||
| * vitest uses for evals; when omitted, no test config is contributed and | ||
| * ordinary unit tests are unaffected. Requires `@skybridge/test` as a dev | ||
| * dependency. |
There was a problem hiding this comment.
this feels more like a claude comment that something to help end user understand what this config option will allow him to configure
|
|
||
| The Vite plugin behind [Skybridge](https://skybridge.tech): it discovers view | ||
| modules, serves them as virtual entries, applies the `data-llm` transform, and | ||
| tells `skybridge build` which packages to leave unbundled. |
There was a problem hiding this comment.
it also extend vitest assertions with chat-oriented APIs to assert various scenario occurring chances
fredericbarthelet
left a comment
There was a problem hiding this comment.
a few comments, I think you can greatly reduce complexity by avoiding plugin definition polymorphism based on config options and by starting the server in the start chat method itself
874c2eb to
dde647a
Compare
| fi | ||
|
|
||
| echo "version=$VERSION" >> "$GITHUB_OUTPUT" | ||
| echo "beta_version=$VERSION-beta" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
don't forget to remove those lines before merging :)
| } | ||
|
|
||
| /** The tool definitions the model was looking at, for failure reports. */ | ||
| get toolDefinitions(): ToolDefinition[] { |
There was a problem hiding this comment.
why is this part of the public API, I feel like assertions on the tool definition is like asserting that an exact line of code is written 😅
| } | ||
|
|
||
| /** Takes a turn and returns the model's answer. */ | ||
| async send(prompt: string): Promise<string> { |
There was a problem hiding this comment.
returning only the last turn text response might be a bit restrictive : it's probably better to have assertion helpers that can dig in all properties of the assistant messages to test for the presence of a response that providing it in the public API and letting people run their own assertion on the string content, WDYT?
There was a problem hiding this comment.
you agreed and resolved, but didn't change the API 😅
| "./vitest-config": { | ||
| "types": "./dist/vitest-config.d.ts", | ||
| "default": "./dist/vitest-config.js" |
There was a problem hiding this comment.
this is not used anymore
| "@ai-sdk/anthropic": "^2.0.0", | ||
| "@ai-sdk/google": "^2.0.0", | ||
| "@ai-sdk/mistral": "^2.0.0", | ||
| "@ai-sdk/openai": "^2.0.0", |
| "@ai-sdk/anthropic": { | ||
| "optional": true | ||
| }, | ||
| "@ai-sdk/google": { | ||
| "optional": true | ||
| }, | ||
| "@ai-sdk/mistral": { | ||
| "optional": true | ||
| }, | ||
| "@ai-sdk/openai": { | ||
| "optional": true | ||
| } |
There was a problem hiding this comment.
if you switch to having a model being a LanguageModel , you only need a peer dep to the core ai sdk package
There was a problem hiding this comment.
The ecosystem convention is peer + dev on ai, and no @ai-sdk/{openai,anthropic,…} at all.
Evalite uses LanguageModel interface from ai as input for their instrumentation
| ```ts | ||
| skybridge({ | ||
| evals: { | ||
| model: "anthropic/claude-sonnet-4-5", |
There was a problem hiding this comment.
this notation is usually reserved for people using the vercel ai gateway, it can be also supported in the project if you don't provide a LanguageModel
| `openai/gpt-5` also resolves through the Vercel AI Gateway if you have | ||
| `AI_GATEWAY_API_KEY` set and no provider package installed. | ||
|
|
||
| ## Models the prefixes cannot reach |
There was a problem hiding this comment.
this use case is supported by ai sdk custom provider
fredericbarthelet
left a comment
There was a problem hiding this comment.
A few comments, but it starts to take shape :)
Also, could you throw a first version of documentation for this new package in this PR?
0ad9d4b to
8a4b6f5
Compare
8a4b6f5 to
b6f5d60
Compare
b6f5d60 to
f7943e6
Compare
f7943e6 to
2cdace2
Compare
fredericbarthelet
left a comment
There was a problem hiding this comment.
LGTM :) Awesome work @harijoe !
1cfaa1d to
2cdace2
Compare
2cdace2 to
8ceb3ae
Compare
Summary
@skybridge/test: eval runner answering whether a prompt reaches the right tool with the right arguments.skybridge({ evals }); scenarios use vitest's ownit/expectplusstart<AppType>().provider/model-idstring, and only the provider named gets imported.defineEvalModel()registers a model from a setup file.@skybridge/vite-plugin.skybridge/viteis removed; importskybridgefrom@skybridge/vite-plugininstead.server.run()now reports its bound port over IPC, letting the runner use an ephemeral port with no bind race.Implements SKY-546. API validated on the POC (#1052) and the design review thread.