-
Notifications
You must be signed in to change notification settings - Fork 305
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
[OPIK-655]: finish initial dataset UI #1016
Conversation
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.
Great work! I added some suggestions and improvements in the code and logic, overall it looks great, nice management of async queues, streaming and performance
Let's address the changes in the next PR (logic changes + design feedback), thanks!
const flattenedDatasetItem = flattenObject(datasetItem); | ||
|
||
const notDefinedVariables = messageTags.filter((tag) => | ||
isUndefined(flattenedDatasetItem[tag]), | ||
); |
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.
Maybe better to replace the flattenObject
with the lodash get
instead?
|
||
return { | ||
role: message.role, | ||
content: mustache.render(message.content, datasetItem), |
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.
Probably we need extra logic here in case we pass an object since mustache is using toString
instead of JSON.stringify
for replacing the values
async (task) => { | ||
await createTraceSpan(task); | ||
}, |
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.
Just passing createTraceSpan
should be okay, right?
(combination: DatasetItemPromptCombination, callback) => { | ||
processCombination(combination, callback); | ||
}, |
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 don't believe the callback
here is actually needed if you just return the Promise from processCombination
like:
asyncLib.mapLimit(
combinations,
LIMIT_STREAMING_CALLS,
processCombination,
() => {
setIsRunning(false);
isToStopRef.current = false;
abortControllersOngoingRef.current.clear();
},
);
And removing the processCallback(null)
calls
useEffect(() => { | ||
// stop streaming whenever the location changes | ||
return () => stopAll(); | ||
}, [location, stopAll]); |
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'd remove the location
dependency since it's prone to error in case in the future we introduce search/hash locations in the Playground, just removing the dependency should do the trick and it'll be called on unmount the component
Details
Core features:
zustand
in order to avoid re-rendering components. Each output and prompt is re-rendered only when it is changed now. The store is connected to the localStorage. The key has changed, so all previous playgrounds will be reset.async
libraryIssues
Resolves #
Testing
Documentation