-
Notifications
You must be signed in to change notification settings - Fork 77
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
[editor] Remount Model Settings Renderer on Model Change #1257
Conversation
@@ -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 |
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.
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?
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'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
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.
multiple props where we re-use the default model
Sorry typo, meant to say "multiple prompts", so you answered it with the last paragraph
@@ -107,6 +110,7 @@ export default memo(function PromptActionBar({ | |||
style={{ overflowY: "auto" }} | |||
> | |||
<ModelSettingsRenderer | |||
model={getPromptModelName(prompt, defaultConfigModelName)} |
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 looked at the getPromptModelName
method in
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?
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.
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)
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 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
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.
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.
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.
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)
[editor] Remount Model Settings Renderer on Model Change
Currently, the model settings state is retained when changing models in the selector instead of updating to the new defaults:
Screen.Recording.2024-02-15.at.1.55.23.PM.mov
To fix, we can force the model settings renderer to remount when the prompt model value changes by passing the model name as the
key
prop to the component. Specifically, this handles the scenario outlined in discussion here: #1256 (comment)Screen.Recording.2024-02-15.at.6.18.51.PM.mov