refactor: better organization and support for advanced executors #102
Conversation
|
|
||
| export type Environment = LocalEnvironment | RemoteEnvironment; | ||
| /** Represents a single prompt evaluation environment. */ | ||
| export class Environment { |
There was a problem hiding this comment.
This is just moving back the previous base-environment.ts into the environment.ts file
| localEnvironmentConfigSchema, | ||
| remoteEnvironmentConfigSchema, | ||
| ]); | ||
| export const environmentConfigSchema = z.object({ |
There was a problem hiding this comment.
This is the same schema as from base-environment-config.ts
| import {mcpServerOptionsSchema} from '../../codegen/llm-runner.js'; | ||
| import {getPossiblePackageManagers} from '../../configuration/environment-config.js'; | ||
|
|
||
| export const localExecutorConfigSchema = z.strictObject({ |
There was a problem hiding this comment.
This is the schema from environment-local.ts because it's now tied to the local executor directly.
| @@ -0,0 +1,204 @@ | |||
| import {ChildProcess, fork} from 'node:child_process'; | |||
There was a problem hiding this comment.
This is a copy of the previous local-gateway, with a few added things:
llmis instantiated directly in here (specific to the local executor)llmis destroyed/disposed here directly via the newdestroy()hooktryRepairfor example got renamed (a couple of APIs got renamed..)- Exposes the
configand e.g.getBuildCommand - Passes the build command, package manager etc to the Genkit codegen requests (needed for e.g. Gemini CLI runner)
| llm.startMcpServerHost(hostName, this.config.mcpServers ?? []); | ||
| } | ||
|
|
||
| async collectMcpServerLogs() { |
There was a problem hiding this comment.
The MCP logic moved directly into the local executor as it's very specific to Genkit and was previously hard-coded into generate.ts. In the future we could have a generic API as part of Executor.
| } | ||
|
|
||
| /** Request information for a file generation. */ | ||
| export interface LlmGenerateFilesRequest { |
There was a problem hiding this comment.
Unlike the genkit interface (which extends from this), normal LLM generate requests do not need e.g. buildCommand or packageManager in here. The local executor can inject this information for e.g. Gemini CLI based on the local executor configuration.
| // Needed for portability of the `PQueue` type. | ||
| export type WorkerQueueType = PQueue; | ||
|
|
||
| export const executorSchema = z.object({ |
There was a problem hiding this comment.
I'm not sure if we need a schema for the executor since it'll usually be a JS class instance. Maybe we should require users to extend some sort of base executor?
The reason why we have a schema for the environment config is that it's just an object literal.
There was a problem hiding this comment.
I think we can follow-up on this when we better support TypeScript. Right now this seems useful at least & isn't much more work/cost
runner/reporting/report-logging.ts
Outdated
| ): void { | ||
| ): Promise<void> { | ||
| const runnerInfo = | ||
| env.executor instanceof LocalExecutor ? await env.executor.getRunnerInfo() : null; |
There was a problem hiding this comment.
Instead of checking for LocalExecutor in a few places, couldn't we move these methods into the base class/interface and have them be optional?
There was a problem hiding this comment.
I think we can move the runner info out, but other methods like for the templating/source directory I'd keep specific to the local executor.
There was a problem hiding this comment.
Addressed some of this (as much as possible). Good idea
devversion
force-pushed
the
easier-local-gateway-reuse
branch
from
657aea8 to
c6ff850
Compare
* Renames `Gateway` to `Executor` with strict validation via Zod.
* Attaches executor specific configuration to the executor, instead of
letting (previous) gateways extract information through multiple layer
of indirections from the `Environment`.
* This also resulted in lots of difficulties around generics and
heritage.
* No longer passes around gateway, or executors as they can be directly
accessed from the `Environment`.
* Adds a compatibility layer for old configs where the local executor is
"assumed" and configured at the top-level environment config object.
* Updates the remote environment example with the new API.
* Improves the gateway/executor API to be better named and support async
model validation, or destroying.
devversion
force-pushed
the
easier-local-gateway-reuse
branch
from
c6ff850 to
55c54c4
Compare
2 participants
GatewaytoExecutorwith strict validation via Zod.Environment.Environment.