-
Notifications
You must be signed in to change notification settings - Fork 285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(playground): Extract and display variables from all message templates as "inputs" #4994
Conversation
@@ -123,6 +123,7 @@ const playgroundInputOutputPanelContentCSS = css` | |||
|
|||
function PlaygroundContent() { | |||
const instances = usePlaygroundContext((state) => state.instances); | |||
const inputs = usePlaygroundContext((state) => state.input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect there will be a better way to extract this data given that the shape of the input object changes based on whether we are in dataset or manual mode.
<View padding="size-200"> | ||
<pre> | ||
{JSON.stringify( | ||
"variables" in inputs ? inputs.variables : "inputs go here", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is temporary, just to show that the store is working
@@ -192,6 +207,7 @@ export const createPlaygroundStore = ( | |||
return instance; | |||
}), | |||
}); | |||
get().calculateVariables(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be heavy handed, but with the size of our data I don't see any performance problems with this.
There may be a better place to hook this logic into however that I'm not familiar with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh this is basically calculating variables everytime the instance changes?
I was just thinking over whether it's worth it to have some abstractions like updateMessage and stuff as opposed to everything going through updateInstance. Doesn't seem necessary now, but if we did this could go there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was going back and forth on that, I wasn't sure what all of the operations were that flow through updateInstance.
Rather than writing bespoke abstractions for each update operation and making sure we calculate variables in all of the correct abstractions, I figured this would catch everything for not much perf cost.
Otherwise yeah I agree with you. Additionally we could store variables per-instance and merge them all together before display so that we only have to calculate variables in a single instance at a time.
2693d01
to
393deaa
Compare
FString: "f-string", // {variable} | ||
Mustache: "mustache", // {{variable}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
know this was pre-existing but could add to a doc comment with in an examples block for the two variable types so we can see this elsewhere - feel like you had that on some of the other stuff so maybe not necessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
format: ({ | ||
text, | ||
variables, | ||
}: { | ||
text: string; | ||
variables: Record<string, string | number | boolean | undefined>; | ||
}) => string; | ||
extractVariables: (template: string) => string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might pull these out into some top level types instead of inline
eg:
type ExtractVariablesFn = (template: string) => string[];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
input: { variables: {} }, | ||
input: { | ||
// TODO(apowell): When implementing variable forms, we should maintain a separate | ||
// map of variableName to variableValue. This will allow us to "cache" variable values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, so values don't get reset if you switch back and forth or accidentally type one extra character or something. Also raises a question for user defined variables in the following flow:
- add a variable called question_1 to a message
- set some value to that variable
- change that variable to be question_2
Don't need to handle this but similar case. Also this will all be a bit different with datasets where there will be a set of potential variables (names + values) coming from the dataset that should be available in the playground
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wondering about that. We can evict the cache of unused variable values on like prompt run or something like that.
As for the incoming variables from dataset type playground, I will have to play with that to see how it works and where to best parse out the variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i will put in some variable parsing logic from spans, so that might inform some things but don't need to worry now
if (instance.template.__type === "chat") { | ||
// for each chat message in the instance | ||
instance.template.messages.forEach((message) => { | ||
// extract variables from the message content | ||
const extractedVariables = utils.extractVariables(message.content); | ||
extractedVariables.forEach((variable) => { | ||
variables[variable] = ""; | ||
}); | ||
}); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small thing here since we're basically not supporting completion templates but it would be pretty easy to add an exhaustive switch on the template here and parse variables from the completion templates as well since they are just something like: {content: string } if i remember correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Merging to add a toggle |
template.calc.mov