Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[editor] Remount Model Settings Renderer on Model Change #1257

Merged
merged 1 commit into from
Feb 16, 2024
Merged

[editor] Remount Model Settings Renderer on Model Change #1257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions python/src/aiconfig/editor/client/src/components/prompt/PromptActionBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ClientPrompt } from "../../shared/types";
import {
PromptSchema,
checkParametersSupported,
getPromptModelName,
} from "../../utils/promptUtils";
import { ActionIcon, Container, Flex, ScrollArea, Tabs } from "@mantine/core";
import { IconClearAll } from "@tabler/icons-react";
Expand All @@ -13,6 +14,7 @@ import { JSONObject } from "aiconfig";
import { PROMPT_CONTAINER_HEIGHT_MAP } from "./PromptContainer";

type Props = {
defaultConfigModelName?: string;
prompt: ClientPrompt;
promptSchema?: PromptSchema;
onUpdateModelSettings: (settings: Record<string, unknown>) => void;
Expand Down Expand Up @@ -58,6 +60,7 @@ const MIN_ACTION_BAR_HEIGHT = 300;
const ADD_PARAMETER_BOTTOM_HEIGHT = 46;

export default memo(function PromptActionBar({
defaultConfigModelName,
prompt,
promptSchema,
onUpdateModelSettings,
Expand Down Expand Up @@ -107,6 +110,7 @@ export default memo(function PromptActionBar({
style={{ overflowY: "auto" }}
>
<ModelSettingsRenderer
model={getPromptModelName(prompt, defaultConfigModelName)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the getPromptModelName method in

aiconfig/python/src/aiconfig/editor/client/src/utils/promptUtils.ts

Line 34 in 1c68007

export function getPromptModelName(

Just to sanity check, the value of this doesn't quite "matter" (since it's the Hugging Face Task ("Text Generation") and not the model itself ("mistralai/Mistral-7B-Instruct-v0.1")). All that matters is that we have changed the HF task (which for most cases will be the actual model itself like dall-e 2) and that changed value will be detected by the key and cause that component to re-render the settings right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, it's technically the model id that's registered in the ModelParserRegistry. And yep, that's correct (just nit on terminology, it would re-mount instead of re-render)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is beyond scope of this PR, just curious, what's the difference between re-mount vs. re-render and why does changing the key cause it to re-mount? Is Chat GPT wrong here? https://lastmileai.dev/workbooks/clsoufolp001pqrcsg97aj5dr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's wrong -- see https://react.dev/reference/react/useState#resetting-state-with-a-key

You can reset a component’s state by passing a different key to a component. In this example, the Reset button changes the version state variable, which we pass as a key to the Form. When the key changes, React re-creates the Form component (and all of its children) from scratch, so its state gets reset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-mount basically means the component is removed from the DOM, recreated and re-added (state, etc. is re-initialized). Re-render just means updating the existing component (state, etc. is persisted)

settings={getModelSettings(prompt)}
schema={modelSettingsSchema}
onUpdateModelSettings={onUpdateModelSettings}
Expand Down
1 change: 1 addition & 0 deletions python/src/aiconfig/editor/client/src/components/prompt/PromptContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ export default memo(function PromptContainer({
</Card>
<div className="sidePanel">
<PromptActionBar
defaultConfigModelName={defaultConfigModelName}
prompt={prompt}
promptSchema={promptSchema}
onUpdateModelSettings={updateModelSettings}
Expand Down
6 changes: 6 additions & 0 deletions ...src/aiconfig/editor/client/src/components/prompt/model_settings/ModelSettingsRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import JSONEditorToggleButton from "../../JSONEditorToggleButton";
import { ErrorBoundary, useErrorBoundary } from "react-error-boundary";

type Props = {
model?: string;
settings?: JSONObject;
schema?: GenericPropertiesSchema;
onUpdateModelSettings: (settings: Record<string, unknown>) => void;
Expand Down Expand Up @@ -52,6 +53,7 @@ function SettingsErrorFallback({
}

export default memo(function ModelSettingsRenderer({
model,
settings,
schema,
onUpdateModelSettings,
Expand All @@ -75,6 +77,8 @@ export default memo(function ModelSettingsRenderer({
{/* // Only show the toggle if there is a schema to toggle between JSON and custom schema renderer */}
{schema && rawJSONToggleButton}
<JSONRenderer
// Use the model name as the key to force re-mount / updated state when model changes
key={model}
content={settings}
onChange={(val) =>
onUpdateModelSettings(val as Record<string, unknown>)
Expand All @@ -93,6 +97,8 @@ export default memo(function ModelSettingsRenderer({
>
{rawJSONToggleButton}
<ModelSettingsSchemaRenderer
// Use the model name as the key to force re-mount / updated state when model changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this cause an issue if we have multiple props where we re-use the default model and hence have the same key for two separate ModelSettingsSchemaRenderer objects? Or is React smart enough to understand the hierarchy and know that even though we set the same keys, they're not the same objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow that flow, can you explain what you mean by 'multiple props where we re-use the default model'?

The model selector value should be either the model in the prompt metadata or the default for the aiconfig, and if they're the same then selecting that model in the selector is a no-op (because it's already selected). Also, the model -> parser mapping and model -> prompt schema mapping are both unique on model keys.

Unless you're talking between different prompts -- like prompt_1 and prompt_2 have the same model (so same key). In that case, yes it's fine because the <ModelSettingsSchemaRenderer>s aren't siblings, so React will already know the elements are different based on the DOM structure to get to them

Copy link
Contributor

@rossdanlm rossdanlm Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple props where we re-use the default model

Sorry typo, meant to say "multiple prompts", so you answered it with the last paragraph

key={model}
settings={settings}
schema={schema}
onUpdateModelSettings={onUpdateModelSettings}
Expand Down
Loading