[editor] Model Selector #662

rholinshead · 2023-12-29T01:29:25Z

[editor] Model Selector

Implement the model selector and add a placeholder call to the future api endpoint for updating the model.

Screen.Recording.2023-12-28.at.8.29.42.PM.mov

Stack created with Sapling. Best reviewed with ReviewStack.

rholinshead · 2023-12-29T01:33:56Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

  const onUpdatePromptModelSettings = useCallback(
    async (promptIndex: number, newModelSettings: any) => {
      dispatch({
        type: "UPDATE_PROMPT_MODEL_SETTINGS",
        index: promptIndex,
        modelSettings: newModelSettings,
      });
-      // TODO: Call server-side endpoint to update model settings
+      // TODO: Call server-side endpoint to update model


Not setting this up just yet (unlike model name below) since there is a higher potential for bugs in managing the settings object & don't want to add friction to whomever is adding the server-side endpoint. I'll complete this once the endpoint is implemented

rossdanlm · 2023-12-29T14:19:08Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+  updateModel: (
+    promptName?: string,
+    modelData?: string | ModelMetadata
+  ) => Promise<void /*{ aiconfig: AIConfig }*/>;


Why not return aiconfig?

It will once the endpoint is implemented -- right now there's no updated aiconfig to return.

rossdanlm · 2023-12-29T14:19:57Z

python/src/aiconfig/editor/client/src/Editor.tsx

+      // return await ufetch.post(ROUTE_TABLE.UPDATE_MODEL,
+      //   prompt_name: promptName,
+      //   model_data: modelData,
+      // });


Are you commenting this out to add it later once the update_model() api is built on backend?

rossdanlm · 2023-12-29T14:22:59Z

Btw not sure if you're aware, but I refactored the update_model() python SDK to take in model_name and model_settings as two separate function args: https://github.com/lastmile-ai/aiconfig/blob/ca59d2c1657fe6e2655f1a0c25180f9407b97cab/python/src/aiconfig/schema.py#L492C34-L492C34

PR for full details: #507

rossdanlm · 2023-12-29T14:27:43Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+      debounce(
+        (promptName?: string, modelMetadata?: string | ModelMetadata) =>
+          callbacks.updateModel(promptName, modelMetadata),
+        250


Should we define this as a const somewhere? I see it's also 250 for addPrompt

I can pull it out to a const for now in a separate PR, but it could technically be dependent on where it's used

rossdanlm · 2023-12-29T14:38:32Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+          callbacks.updateModel(promptName, modelMetadata),
+        250
+      ),
+    [callbacks.updatePrompt]


Should this be .updateModel?

Oh, yep, thanks

rossdanlm · 2023-12-29T14:39:31Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+        );
+        const currentModel = prompt.metadata?.model;
+        let modelData: string | ModelMetadata | undefined = newModel;
+        if (newModel && currentModel && typeof currentModel !== "string") {


I think with the new SDK you won't have to do individual checks because we're passing model_name and settings as two separate args, hopefully this makes it easier for you

Ah, ok - for the updated config it seems like we can't support the following scenario:
Change model name for a prompt and don't set any settings (i.e. set model to the string name instead of {name: <name>, settings: {}}). This is also another case where we have multiple ways of representing the same thing in aiconfig, and might be annoying to set the model name and clear the settings but always have settings: {} set by the editor instead of just having model: name in the config?

On top of that, if we change the model and it did already have settings set, we would ideally want to maintain only the settings that are supported by the new model (e.g. say temperature, topK are used by both models) as defined in the prompt schema that is associated with the model provider, and drop anything not supported. This might need to wait until post-MVP, though. For now we can either clear the settings or keep them entirely... I was doing the former here, but open to opinions

rossdanlm · 2023-12-29T14:40:15Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+        // TODO: Consolidate
+      } catch (err: any) {
+        showNotification({
+          title: "Error updating prompt model",


Would it be possible to read the http message that we return and we display it? Can be done future PR

rossdanlm · 2023-12-29T14:40:27Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+      } catch (err: any) {
+        showNotification({
+          title: "Error updating prompt model",
+          message: err.message,


Oh I guess it's done here?

rossdanlm · 2023-12-29T14:41:37Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+                onUpdateModel={onUpdatePromptModel}
                onUpdateModelSettings={onUpdatePromptModelSettings}


Oh ok cool, so we are keeping them as separate actions?

Ya, we should keep them separate since there's different logic for them:

update settings --> keep same model name but new model settings object

update model --> change model name and either keep settings or clear them

rossdanlm · 2023-12-29T14:42:18Z

python/src/aiconfig/editor/client/src/components/aiconfigReducer.ts

@@ -37,6 +38,12 @@ export type UpdatePromptNameAction = {
  name: string;
 };

+export type UpdatePromptModelAction = {


Is this being used anywhere in this PR? Does the displatch type being set to "onUpdatePromptModel" mean that it's automatically mapped to this action type?

When we dispatch the "UPDATE_PROMPT_MODEL" action, that action is implicitly typed as this UpdatePromptModelAction type. We add it to MutateAIConfigAction above and that does two things:

Makes it part of the AIConfigReducerAction via the union for that type, which means our reducer below will require a case "UPDATE_PROMPT_MODEL" to handle it to ensure the state is update -- this is the automatic mapping you're referring to

Allows us to dispatch CONSOLIDATE_AICONFIG after we get the response, in order to update the client state with response state (not done here because we don't have an aiconfig response to do that with just yet)

Nice, thanks for the explanation!

rossdanlm · 2023-12-29T14:43:45Z

python/src/aiconfig/editor/client/src/components/aiconfigReducer.ts

+          model: action.modelName
+            ? {
+                name: action.modelName,
+                // TODO: Consolidate settings based on schema union
+              }
+            : undefined,


Hopefully don't need to do this logic with updated SDK

I'll revisit this once the endpoint is implemented. We still need some UI state change here (this happens before the request is sent and before the response is returned to ensure responsive UI). Probably what will need to be done is:

this action sets just the name

aiconfig response comes back with full metadata, either cleared settings or updated settings to match supported settings based on the schema

from aiconfig response, we consolidate the UI state model metadata with updated settings

rossdanlm · 2023-12-29T14:44:32Z

python/src/aiconfig/editor/client/src/components/prompt/AddPromptButton.tsx

-import { memo, useCallback, useEffect, useState } from "react";
-import { showNotification } from "@mantine/notifications";
+import { memo, useCallback, useState } from "react";
+import useLoadModels from "../../hooks/useLoadModels";


Nice, much cleaner

rossdanlm · 2023-12-29T14:46:48Z

python/src/aiconfig/editor/client/src/components/prompt/AddPromptButton.tsx

@@ -41,30 +41,14 @@ function ModelMenuItems({

 export default memo(function AddPromptButton({ addPrompt, getModels }: Props) {
  const [modelSearch, setModelSearch] = useState("");


Can you clarify what modelSearch string supposed to represent?

There's a search input and modelSearch is the text of that input

rossdanlm · 2023-12-29T14:52:33Z

python/src/aiconfig/editor/client/src/components/prompt/ModelSelector.tsx

+      filter={(searchValue: string, item: AutocompleteItem) => {
+        if (showAll) {
+          return true;
+        }
+
+        const modelName: string = item.value;
+        return modelName
+          .toLocaleLowerCase()
+          .includes(searchValue.toLocaleLowerCase().trim());


Alright so what's the difference between this vs. the functionality in getModels callback:

aiconfig/python/src/aiconfig/editor/client/src/Editor.tsx

Lines 31 to 40 in 9ac3b2e

const getModels = useCallback(async (search: string) => {

// For now, rely on caching and handle client-side search filtering

// We will use server-side search filtering for Gradio

const res = await ufetch.get(ROUTE_TABLE.LIST_MODELS);

const models = res.data;

if (search && search.length > 0) {

return models.filter((model: string) => model.indexOf(search) >= 0);

}

return models;

}, []);

This is specific to this AutoComplete UI component because needs to handle the search/filtering of the provided data on its own

rossdanlm · 2023-12-29T14:59:25Z

python/src/aiconfig/editor/client/src/hooks/useLoadModels.ts

+
+  useEffect(() => {
+    loadModels(modelSearch);
+  }, [loadModels, modelSearch]);


How come we need to pass in modelSearch as a dependency here but not getModels as a dependency on L21? Is it because we're implicitly assuming that getModels should only be defined once and never change?

Ah, good catch. getModels should be added above. Something must be up with my linter because it's missing these deps :(

rossdanlm · 2023-12-29T15:13:08Z

python/src/aiconfig/editor/client/src/components/prompt/ModelSelector.tsx

+        onSelectModel(value);
+        models.some((model) => {
+          if (model === value) {
+            setShowAll(true);


Ok just to clarify, this onChange only occurs when a value is selected from the autocomplete options? Does this also occur when user changes the search string? If this occurs when user changes the text and it matches a model name, we wouldn't want to have setShowAll to true right? I noticed that even when we enter gpt-4 in your test plan, at the bottom are still some model names that don't match like dall-e-3

This is mainly copied from our model selector in Workbooks and is sort of a workaround to match our desired behaviour. By default, the component will only ever show results in the dropdown that match the text in the input box. So, if the aiconfig has model set to gpt-4 when you load it, the dropdown would only ever show models with gpt-4 substring.
This isn't what we want, though, since users may not know that there are other non gpt-4 models to use then. The behaviour we want is:

if the user is typing to 'search' for a model, then the dropdown should filter on that search substring

otherwise, if the model is selected from the list / defaulted from the config then we want to show all the model options to change to

Oh, I generally assumed that the user would just click the x button on the right to see everything, hmmmm. I don't think it's a blocker but feels a bit strange to me that auto-completions don't remain filtered after selected. We can do this as P2 someday

rossdanlm

This generally looks good to me. Biggest point is my comment about the SDK being refactored to take in name + settings separately, but we can fix forward instead of worrying about merge conflict. Also have question about the autocompletion filtering.

This'll also be easier for you to test once I add this endpoint on the backend for you

# [editor] Model Selector Implement the model selector and add a placeholder call to the future api endpoint for updating the model. https://github.com/lastmile-ai/aiconfig/assets/5060851/812f3a10-aaac-4ec3-9f1f-4d21ebc99e9f

[editor] Support Deleting Prompt # [editor] Support Deleting Prompt Add the UI side for deleting a prompt, with placeholder to call the server endpoint once it's implemented https://github.com/lastmile-ai/aiconfig/assets/5060851/b3e682dd-a3ed-45c1-b153-b4b6e2094616 --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/665). * #667 * #666 * __->__ #665 * #662

[editor] Run Prompt (non-streaming) # [editor] Run Prompt (non-streaming) With non-streaming /run endpoint properly returning the response now, we can update the client config with the resulting output. We can also leverage the client-side `_ui` state for maintaining 'isRunning' state to show a spinner / disable the execute button for now (TODO: we may need to handle canceling execution as well). https://github.com/lastmile-ai/aiconfig/assets/5060851/67fa793a-9a60-4285-a7d1-69fff633d6a6 --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/667). * __->__ #667 * #666 * #665 * #662

rholinshead commented Dec 29, 2023

View reviewed changes

rholinshead marked this pull request as ready for review December 29, 2023 01:34

rholinshead requested review from saqadri, suyoglastmileai, Ankush-lastmile, jonathanlastmileai and rossdanlm as code owners December 29, 2023 01:34

rholinshead changed the title ~~Model Selector~~ [editor] Model Selector Dec 29, 2023

rholinshead force-pushed the pr662 branch from 434a192 to 924a65f Compare December 29, 2023 02:33

This was referenced Dec 29, 2023

[editor] Support Deleting Prompt #665

Merged

[editor] Use Prompt ID Instead of Index for Client-side State Actions #666

Merged

rholinshead force-pushed the pr662 branch from 924a65f to 9ac3b2e Compare December 29, 2023 03:09

rholinshead mentioned this pull request Dec 29, 2023

[editor] Run Prompt (non-streaming) #667

Merged

rossdanlm reviewed Dec 29, 2023

View reviewed changes

rossdanlm approved these changes Dec 29, 2023

View reviewed changes

[editor] Model Selector

2bda80c

# [editor] Model Selector Implement the model selector and add a placeholder call to the future api endpoint for updating the model. https://github.com/lastmile-ai/aiconfig/assets/5060851/812f3a10-aaac-4ec3-9f1f-4d21ebc99e9f

rholinshead force-pushed the pr662 branch from 9ac3b2e to 2bda80c Compare December 29, 2023 15:52

rholinshead merged commit 2bda80c into main Dec 29, 2023
2 checks passed

rossdanlm deleted the pr662 branch December 29, 2023 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[editor] Model Selector #662

[editor] Model Selector #662

rholinshead commented Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023 •

edited

Loading

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm commented Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rossdanlm Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rossdanlm Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023 •

edited

Loading

rossdanlm Dec 29, 2023

rossdanlm left a comment

		onUpdateModel={onUpdatePromptModel}
		onUpdateModelSettings={onUpdatePromptModelSettings}

		@@ -41,30 +41,14 @@ function ModelMenuItems({

		export default memo(function AddPromptButton({ addPrompt, getModels }: Props) {
		const [modelSearch, setModelSearch] = useState("");

	const getModels = useCallback(async (search: string) => {
	// For now, rely on caching and handle client-side search filtering
	// We will use server-side search filtering for Gradio
	const res = await ufetch.get(ROUTE_TABLE.LIST_MODELS);
	const models = res.data;
	if (search && search.length > 0) {
	return models.filter((model: string) => model.indexOf(search) >= 0);
	}
	return models;
	}, []);

[editor] Model Selector #662

[editor] Model Selector #662

Conversation

rholinshead commented Dec 29, 2023 • edited Loading

[editor] Model Selector

rholinshead Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm commented Dec 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

rholinshead Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm left a comment

Choose a reason for hiding this comment

rholinshead commented Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023 •

edited

Loading

rossdanlm Dec 29, 2023 •

edited

Loading

rossdanlm Dec 29, 2023 •

edited

Loading

rossdanlm Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023 •

edited

Loading