Implement project.parameters.create and loadParameters APIs#1298
Implement project.parameters.create and loadParameters APIs#1298joshuawootonn merged 23 commits intomainfrom
project.parameters.create and loadParameters APIs#1298Conversation
|
small nit: should the PR be titled "Implement |
loadParameter API loadParameters API
|
Yes it should be. Thank you! |
56870fb to
1a3f392
Compare
js/src/framework.ts
Outdated
| // todo(josh): at this point, I have a JSON schema, but I don't have something to use that JSON schema to validate my data. | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| const loadedData = | ||
| resolvedEvaluatorParams.data as unknown as InferParameters<EvalParameters>; | ||
| if (parameters && Object.keys(parameters).length > 0) { | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| parameters = { | ||
| ...loadedData, | ||
| ...parameters, | ||
| } as unknown as InferParameters<EvalParameters>; | ||
| } else { | ||
| parameters = loadedData; | ||
| } | ||
| } else if (resolvedEvaluatorParams) { | ||
| parameters = validateParameters( | ||
| parameters ?? {}, | ||
| resolvedEvaluatorParams, | ||
| ); |
There was a problem hiding this comment.
I could use some advice here. In the previous implementation, we always have the schema passed directly to the Eval. This meant we could use that schema to validate parameters, as you can see in the second branch of this if statement.
The schema is now passed to create.
export const myParameters = project.parameters.create({
name: "My Parameters",
slug: "my-parameters",
schema: {
numSamples: z
.number()
.default(100)
.describe("Number of samples to take from the dataset"),
datasetName: z
.string()
.default("Animals")
.describe("Name of the dataset to sample from"),
},
});We no longer have the zod schema when we loadParameters and therefore we don't have it in the actual eval function context either.
Eval(PROJECT_NAME, {
experimentName: "My Eval",
data: /* ... */
task: async (input, { parameters }) => {
console.log("datasetName", parameters.datasetName);
/* ... */
},
scores: /* ... */,
parameters: loadParameters({
projectName: "My Project",
slug: "my-parameters",
}),
});
We do have a JSON schema, but this SDK doesn't have a JSON schema parsing library installed currently, i'm not sure we want to install one. Do you have any suggestions on how I can validate this data?
I could see us requiring the schema for loadParameters, so that Eval could get the zod schema directly. That kinda sucks, but it might be inevitable
There was a problem hiding this comment.
i think the type param you introduced is inevitable
There was a problem hiding this comment.
Options in my mind:
- Add
ajvor similar and use the json schema to validate - Add
[json-schema-to-zod](https://www.npmjs.com/package/json-schema-to-zod)or something similar and use the users zod to validate
In my mind option 1 doesn't sound too bad since we can lazy import, but let me know.
There was a problem hiding this comment.
Yes this is a tricky problem.
I am not opposed to adding ajv as a dependency to the sdk and doing a best effort to validate against the json schema. That would be at least a bit of validation for the user.
Validating against the actual zod would be tricky without converting/adding some library. json-schema-to-zod is planned to be deprecated soon but will still work for zod3 schemas.
People using our SDK use it with both zod 3 and zod 4 at the moment.
Users using the sdk with zod4 would be able to use toJSONSchema and fromJSONSchema on their schemas.
We are still on zod 3 at the moment for our backend and internal schemas (which create the generated_types files) and is why all our sdk imports from zod needs to export from the /v3 path.
| return ret; | ||
| } | ||
|
|
||
| export function makeEvalParametersSchema( |
There was a problem hiding this comment.
I moved out of server so I could use this within the CLI.
| @@ -0,0 +1,69 @@ | |||
| import { RemoteEvalParameters } from "../logger"; | |||
There was a problem hiding this comment.
This is a copy of prompts-cache.ts but for the parameter.
This might not be necessary. I got a bit confused because it seems like in some places prompt is used interchangeably for function.
There was a problem hiding this comment.
yeah i'm not sure about this one. cc @manugoyal would know, i think
| function_type: "parameters", | ||
| function_data: { | ||
| type: "parameters", | ||
| data: {}, |
There was a problem hiding this comment.
I'm initializing data to an empty object. Could also initialize it to null. It needs to be serializable though, so undefined doesn't work even thought it's the most accurate representation
| import { z } from "zod/v3"; | ||
| import { | ||
| ToolFunctionDefinition as toolFunctionDefinitionSchema, | ||
| type ToolFunctionDefinitionType as ToolFunctionDefinition, | ||
| ChatCompletionMessageParam as chatCompletionMessageParamSchema, | ||
| ModelParams as modelParamsSchema, | ||
| type PromptBlockDataType as PromptBlockData, | ||
| type PromptDataType as PromptData, | ||
| } from "./generated_types"; | ||
|
|
||
| // This roughly maps to promptBlockDataSchema, but is more ergonomic for the user. | ||
| export const promptContentsSchema = z.union([ | ||
| z.object({ | ||
| prompt: z.string(), | ||
| }), | ||
| z.object({ | ||
| messages: z.array(chatCompletionMessageParamSchema), | ||
| }), | ||
| ]); | ||
|
|
||
| export type PromptContents = z.infer<typeof promptContentsSchema>; | ||
|
|
||
| export const promptDefinitionSchema = promptContentsSchema.and( | ||
| z.object({ | ||
| model: z.string(), | ||
| params: modelParamsSchema.optional(), | ||
| templateFormat: z.enum(["mustache", "nunjucks", "none"]).optional(), | ||
| }), | ||
| ); | ||
|
|
||
| export type PromptDefinition = z.infer<typeof promptDefinitionSchema>; | ||
|
|
||
| export const promptDefinitionWithToolsSchema = promptDefinitionSchema.and( | ||
| z.object({ | ||
| tools: z.array(toolFunctionDefinitionSchema).optional(), | ||
| }), | ||
| ); | ||
|
|
||
| export type PromptDefinitionWithTools = z.infer< | ||
| typeof promptDefinitionWithToolsSchema | ||
| >; | ||
|
|
||
| export function promptDefinitionToPromptData( | ||
| promptDefinition: PromptDefinition, | ||
| rawTools?: ToolFunctionDefinition[], | ||
| ): PromptData { | ||
| const promptBlock: PromptBlockData = | ||
| "messages" in promptDefinition | ||
| ? { | ||
| type: "chat", | ||
| messages: promptDefinition.messages, | ||
| tools: | ||
| rawTools && rawTools.length > 0 | ||
| ? JSON.stringify(rawTools) | ||
| : undefined, | ||
| } | ||
| : { | ||
| type: "completion", | ||
| content: promptDefinition.prompt, | ||
| }; | ||
|
|
||
| return { | ||
| prompt: promptBlock, | ||
| options: { | ||
| model: promptDefinition.model, | ||
| params: promptDefinition.params, | ||
| }, | ||
| ...(promptDefinition.templateFormat | ||
| ? { template_format: promptDefinition.templateFormat } | ||
| : {}), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Move this to a central location to avoid circular dependencies error in the zod tests
| __schema: FunctionDataFunctionData4Schema | ||
| """ | ||
| JSON Schema describing the structure and types of the parameters data | ||
| """ |
There was a problem hiding this comment.
Not sure how this regenerated. Open to dropping it.
There was a problem hiding this comment.
The makefile target for regenerate-sdk-types currently generates backend types for both the typescript and the python sdk at the same time.
This should be good to leave in.
loadParameters API project.parameters.create and loadParameters API
project.parameters.create and loadParameters API project.parameters.create and loadParameters APIs
Why API compatibility is failing:
Essentially, the type was widened on parameters. There's a new parameter property on the evaluator file. I'm skeptical that we actually want to drop a major version for this diff, but let me know and I'll update the PR 🙏🏼 |
|
@colsondonohue and @j13huang eden mentioned you both as good reviewers for these changes. If there is someone on the SDK team that would be better for review lemme know 🙏🏼 |
2aa4e47 to
8658092
Compare
|
One thing I want to bring up is: With this new API your schema is no longer passed into the export const myParameters = project.parameters.create({
name: "My Parameters",
slug: "my-parameters",
schema: {
numSamples: z
.number()
.default(100)
.describe("Number of samples to take from the dataset"),
datasetName: z
.string()
.default("Animals")
.describe("Name of the dataset to sample from"),
},
});In practice, this means a user has to manually pass the type of the schema. Curious how this feels. Am I over thinking it this? Or do we need to change this before launch? Eval(PROJECT_NAME, {
experimentName: "My Eval",
data: /* ... */
task: async (input, { parameters }) => {
console.log("datasetName", parameters.datasetName);
/* ... */
},
scores: /* ... */,
parameters: loadParameters<typeof myParameters>({
projectName: "My Project",
slug: "my-parameters",
}),
}); |
This is a good question/point. I think it's a classic problem with database/code integrations. Supabase for example has a build step that fetches database schemas and updates the types accordingly. I'm not sure there is fundamentally a workaround for it. Or in other words, I think you do need to let people provide the type themselves, and anything we do on top of that is just helpful sugar that reduces the effort required. (I might be missing something, though). |
Great point! Ok good to know. I hadn't really connected the dots that a build step would be the workaround. Unless there is a build step like this in place, I think building something like that can be apart of the next "chunk" of work on this API. |
colsondonohue
left a comment
There was a problem hiding this comment.
seems reasonable but i haven't worked in the sdk all that much. adding @ibolmo for a more expert opinion
| @@ -0,0 +1,69 @@ | |||
| import { RemoteEvalParameters } from "../logger"; | |||
There was a problem hiding this comment.
yeah i'm not sure about this one. cc @manugoyal would know, i think
js/src/framework.ts
Outdated
| // todo(josh): at this point, I have a JSON schema, but I don't have something to use that JSON schema to validate my data. | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| const loadedData = | ||
| resolvedEvaluatorParams.data as unknown as InferParameters<EvalParameters>; | ||
| if (parameters && Object.keys(parameters).length > 0) { | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| parameters = { | ||
| ...loadedData, | ||
| ...parameters, | ||
| } as unknown as InferParameters<EvalParameters>; | ||
| } else { | ||
| parameters = loadedData; | ||
| } | ||
| } else if (resolvedEvaluatorParams) { | ||
| parameters = validateParameters( | ||
| parameters ?? {}, | ||
| resolvedEvaluatorParams, | ||
| ); |
There was a problem hiding this comment.
i think the type param you introduced is inevitable
js/src/logger.ts
Outdated
| * @param options.slug The slug of the parameters to load. | ||
| * @param options.version An optional version of the parameters (to read). If not specified, the latest version will be used. | ||
| * @param options.environment Fetch the version of the parameters assigned to the specified environment (e.g. "production", "staging"). Cannot be specified at the same time as `version`. | ||
| * @param options.id The id of specific parameters to load. If specified, this takes precedence over all other parameters (project, slug, version). |
There was a problem hiding this comment.
version doesn't conflict with id, right? i guess you're saying if we provide a version that belongs to another prompt, we'll use the latest version for the provided id, rather than retrieving the prompt with the given version?
There was a problem hiding this comment.
This comment is misleading. Claude modeled it after the existing comment for loadPrompt, which is also misleading. I'll update both.
For both loadPrompt and loadParameter, when you pass both id and version it by id and filters by version. So if you pass both id and version, it will return that id @ that version or throw if it doesn't exist.
js/src/logger.ts
Outdated
| fetch, | ||
| forceLogin, | ||
| state: stateArg, | ||
| }: LoadParametersOptions): Promise< |
There was a problem hiding this comment.
would be nice to use function overloads to enforce the mutual exclusivity of different params at the type level
There was a problem hiding this comment.
I agree.
I think the main thing I want to prevent is someone passing version and environment. Based on your earlier comment (https://github.com/braintrustdata/braintrust-sdk/pull/1298/changes/BASE..c935b1e5dee6cda9e9a0d1ca408af4454844ad08#r2729064591) it should be fine to have version + id and environment + id.
There was a problem hiding this comment.
After further review I can also make projectName + slug, projectId + slug, and id exclusive.
These overloads have been added.
joshuawootonn
left a comment
There was a problem hiding this comment.
Thanks for taking a look @colsondonohue 🙌🏼
js/src/logger.ts
Outdated
| * @param options.slug The slug of the parameters to load. | ||
| * @param options.version An optional version of the parameters (to read). If not specified, the latest version will be used. | ||
| * @param options.environment Fetch the version of the parameters assigned to the specified environment (e.g. "production", "staging"). Cannot be specified at the same time as `version`. | ||
| * @param options.id The id of specific parameters to load. If specified, this takes precedence over all other parameters (project, slug, version). |
There was a problem hiding this comment.
This comment is misleading. Claude modeled it after the existing comment for loadPrompt, which is also misleading. I'll update both.
For both loadPrompt and loadParameter, when you pass both id and version it by id and filters by version. So if you pass both id and version, it will return that id @ that version or throw if it doesn't exist.
js/src/logger.ts
Outdated
| fetch, | ||
| forceLogin, | ||
| state: stateArg, | ||
| }: LoadParametersOptions): Promise< |
There was a problem hiding this comment.
I agree.
I think the main thing I want to prevent is someone passing version and environment. Based on your earlier comment (https://github.com/braintrustdata/braintrust-sdk/pull/1298/changes/BASE..c935b1e5dee6cda9e9a0d1ca408af4454844ad08#r2729064591) it should be fine to have version + id and environment + id.
| __schema: FunctionDataFunctionData4Schema | ||
| """ | ||
| JSON Schema describing the structure and types of the parameters data | ||
| """ |
There was a problem hiding this comment.
The makefile target for regenerate-sdk-types currently generates backend types for both the typescript and the python sdk at the same time.
This should be good to leave in.
| return `parameters:${prefix}:${key.slug}:${key.version ?? "latest"}`; | ||
| } | ||
|
|
||
| export class ParametersCache { |
There was a problem hiding this comment.
I think this makes sense if I am understanding correctly.
Now parameters are stored separately than prompts. In that case when we cache the prompt it may not have the parameters so cache the parameters separately.
Let me know if I understand this correctly. If so I think having a cache on the parameters you are using with the prompt makes sense unless the parameters are also stored with the prompt itself(?)
js/src/framework.ts
Outdated
| // todo(josh): at this point, I have a JSON schema, but I don't have something to use that JSON schema to validate my data. | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| const loadedData = | ||
| resolvedEvaluatorParams.data as unknown as InferParameters<EvalParameters>; | ||
| if (parameters && Object.keys(parameters).length > 0) { | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| parameters = { | ||
| ...loadedData, | ||
| ...parameters, | ||
| } as unknown as InferParameters<EvalParameters>; | ||
| } else { | ||
| parameters = loadedData; | ||
| } | ||
| } else if (resolvedEvaluatorParams) { | ||
| parameters = validateParameters( | ||
| parameters ?? {}, | ||
| resolvedEvaluatorParams, | ||
| ); |
There was a problem hiding this comment.
Yes this is a tricky problem.
I am not opposed to adding ajv as a dependency to the sdk and doing a best effort to validate against the json schema. That would be at least a bit of validation for the user.
Validating against the actual zod would be tricky without converting/adding some library. json-schema-to-zod is planned to be deprecated soon but will still work for zod3 schemas.
People using our SDK use it with both zod 3 and zod 4 at the moment.
Users using the sdk with zod4 would be able to use toJSONSchema and fromJSONSchema on their schemas.
We are still on zod 3 at the moment for our backend and internal schemas (which create the generated_types files) and is why all our sdk imports from zod needs to export from the /v3 path.
@ibolmo I know we discussed previously allowing a bypass of the api-compatibility, do you think we should add that now? I do kind of think this change should at least be a minor due to the additional parameter for the evaluator file parameters but major does seem a bit drastic in this case. I know we are about to release the 3.0 version. @joshuawootonn is there any way we could make the parameters param optional? |
js/src/framework.ts
Outdated
| GenericFunction<unknown, unknown> | ||
| >[]; | ||
| prompts: CodePrompt[]; | ||
| parameters: CodeParameters[]; |
There was a problem hiding this comment.
@joshuawootonn this is the parameter I am talking about that became required.
There was a problem hiding this comment.
yeah we should try to make this optional/nullable. score one for the api compat test
There was a problem hiding this comment.
Just pushed up make it optional, and I'm initializing it if it's optional too
f230038 to
ec2b8e7
Compare
| z.null(), | ||
| ]); | ||
| export type ObjectReferenceNullishType = z.infer<typeof ObjectReferenceNullish>; | ||
| export const SavedFunctionId = z.union([ |
There was a problem hiding this comment.
hmm a little worried of these reverts
There was a problem hiding this comment.
Thanks for checking this. Dropping my merge commit and rebasing on main fixed the issue.
| audit_data: z.union([z.array(z.unknown()), z.null()]).optional(), | ||
| _async_scoring_state: z.unknown().optional(), | ||
| facets: z.union([z.object({}).partial().passthrough(), z.null()]).optional(), | ||
| classifications: z |
There was a problem hiding this comment.
looks like the we are missing this. i'd make sure your branch(es) are on the latest and see if regenerating brings this back
There was a problem hiding this comment.
Thanks for checking this. Same as above
| class ExtendedSavedFunctionIdExtendedSavedFunctionId(TypedDict): | ||
| type: Literal['function'] | ||
| id: str | ||
| version: NotRequired[str | None] |
There was a problem hiding this comment.
Thanks for checking this. Same as above
…moving prompt schema stuff to it's own file
…with TS utility type
…lts` handles this for us
929c8a5 to
05c782c
Compare
|
|
||
|
|
||
| class GitMetadataSettings(TypedDict): | ||
| class GitMetadataSettings(TypedDict, closed=True): |
There was a problem hiding this comment.
this one is interesting. double check that you're using python 3.11. i think this is a python 3.15 only feature?
ibolmo
left a comment
There was a problem hiding this comment.
sorry one more round to double check the closed=True (appears in python 3.15)

Related: https://github.com/braintrustdata/braintrust/pull/10207
Current State:
This PR is ready to go except for the question I have posted here. This is the first time, to my knowledge, that we are using a json schema to validate data, and I could use some advice on how to do that.
Demo:
CleanShot.2026-01-23.at.17.17.47.mp4