feat: Scaffold model invocation params form (#5040) #5045
Conversation
cephalization
force-pushed
the
cephalization/invocation-parameters-ui
branch
2 times, most recently
from
cdaaeaa
to
e78d8fd
Compare
dosubot
bot
added
the
size:L
|
I think it's a known bug but o1 fails due to lack of |
cephalization
force-pushed
the
cephalization/invocation-parameters-ui
branch
from
f7c7e0d
to
4b0c2d8
Compare
| /** | ||
| * Form field for a single invocation parameter. | ||
| * | ||
| * TODO(apowell): Should this be generic over the schema field data type? There |
There was a problem hiding this comment.
What does this comment mean exactly? If it's a real todo might add an issue and link it here. The one below seems like a real follow up so might be nice to have the issue
| ); | ||
| }); | ||
| return ( | ||
| <Flex direction="column" gap="size-200"> |
There was a problem hiding this comment.
Should this be a form? We use https://www.react-hook-form.com/ react hook form, seem slike this could be used here iwth controllers etc. might be tough though with the genericness of it, but would probably give it a try, can be afollow up
app/src/pages/playground/schemas.ts
Outdated
| .string() | ||
| .transform((s) => | ||
| z | ||
| // TODO(apowell): specify schema for parameters that we care about |
There was a problem hiding this comment.
just want to make sure that this won't fail and drop the model name if the invocation params are off, if so it might be worth parsing twice with two different schemas just so that we get as much information as we can. Let me know if this doesn't make sense and we can take a look together
There was a problem hiding this comment.
can we use your schema below with passthrough?
There was a problem hiding this comment.
Okay so this turned out to be awesome. We can now read invocation params from the span and convert them into the expected format automatically, including snake cased keys.
There was a problem hiding this comment.
still want to make sure we can get llm model name and stuff even if invocation params are busted. Can also just not use zod for model name and just traverse to it if that makes it easier
| /** | ||
| * Invocation parameters for O1 models. | ||
| */ | ||
| const o1BaseInvocationParameterSchema = invocationParameterSchema.pick({ |
There was a problem hiding this comment.
doesn't this result in all other invocation parameters being dropped?
https://zod.dev/?id=pickomit
or is that the only valid param for o1?
There was a problem hiding this comment.
This is in fact the only valid param for o1 unfortunately
| ANTHROPIC: { | ||
| default: baseInvocationParameterSchema, | ||
| }, | ||
| } satisfies Record< |
There was a problem hiding this comment.
nice, forced em to go back and read about satisfies, this is great
There was a problem hiding this comment.
It's so good once it clicks. You get inference similar to "as const" against some type without all the readonly garbage
| it("should correctly parse the invocation parameters", () => { | ||
| const span = { | ||
| ...basePlaygroundSpan, | ||
| attributes: JSON.stringify({ | ||
| ...spanAttributesWithInputMessages, | ||
| llm: { | ||
| ...spanAttributesWithInputMessages.llm, | ||
| // note that camel case keys are automatically converted to snake case | ||
| invocation_parameters: | ||
| '{"topP": 0.5, "maxTokens": 100, "seed": 12345, "stop": ["stop", "me"]}', | ||
| }, | ||
| }), | ||
| }; |
There was a problem hiding this comment.
wait which direction it goes snake to camel right? can we get a test showing that with at least one param as well, c just convert one of these params
There was a problem hiding this comment.
Updated comment and test, cursor got me mixed up 😂
cephalization
force-pushed
the
cephalization/invocation-parameters-ui
branch
from
7287cd7
to
419ff2a
Compare
invocation.params.form.mov