[editor] Add 'Metadata' Section Under Settings Tab #1205

rholinshead · 2024-02-09T01:11:34Z

[editor] Add 'Metadata' Section Under Settings Tab

Currently the editor does not provide a way to modify general prompt metadata, outside of model settings and parameters. For some of our chat parsers, this means the user cannot modify the 'remember_chat_context' setting. This PR implements the UX for modifying this metadata, adding it below the model settings in the settings tab. For now, only doing so if the prompt schema specifies prompt_metadata since I have some concerns about this UX for general metadata (without prompt schema it just renders as JSON editor below the settings and that's kind of confusing; it may be improved by having metadata under an accordion fold but still not sure just yet how we can have some metadata above that fold and some not).

Screen.Recording.2024-02-15.at.11.56.55.AM.mov

Stack created with Sapling. Best reviewed with ReviewStack.

rholinshead · 2024-02-09T17:31:44Z

python/src/aiconfig/editor/server/static/asset-manifest.json

Ugh, sorry, just ignore this build stuff here and below. I had run yarn build accidentally and forgot to undo. It's ok to land the build changes since it needs to be done before publishing the aiconfig package anyway

Ankush-lastmile · 2024-02-09T19:37:37Z

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

+        callbacks: {
+          onSuccess?: (aiconfigRes: AIConfig) => void;
+          onError?: (err: unknown) => void;
+        }


what's the rationale to make this an object vs keep the callbacks defined?

Using onSuccess and onError is a very common frontend pattern. I think Ryan is just following conventions?

Ya, for js usually it's common to use an object when props can be undefined rather than having literal 'undefined' prop before a defined one

Ankush-lastmile · 2024-02-09T19:37:42Z

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

+          ...action.metadata,
+          // Keep existing model and parameters metadata (managed by separate actions)
+          model: prompt.metadata?.model,
+          parameters: prompt.metadata?.parameters,


could this just be

metadata: { action.metadata? ...prompt.metadata }

instead?

Following up discussion offline w. Ankush so that others are aware.

He's ensuring that whatever is defined in action.metadata (ex: "test": "Test value here") gets properly set through the action and not overridden by what was previously in the prompt.metadata field. We also can't just do

metadata: { ...prompt.metadata action.metadata? }

Because action is a generic JSONObject and Ryan is safe-guarding to ensure we can't possibly override the model and parameters unsafely through this UPDATE_PROMPT_METADATA action

Ankush-lastmile

lgtm, I had 2 questions

rossdanlm · 2024-02-09T19:59:17Z

[UX] cc @zakariassoul Why not make other stuff like stream setting part of metadata as well, since that's not actually "model settings"?

rossdanlm · 2024-02-09T20:06:01Z

python/src/aiconfig/editor/client/src/reducers/actions.ts

+  | UpdatePromptMetadataAction
  | UpdatePromptModelAction
  | UpdatePromptModelSettingsAction
+  | UpdatePromptNameAction


Nice, alphabetical!

rossdanlm · 2024-02-09T20:07:26Z

...iconfig/editor/client/src/components/prompt/prompt_metadata/PromptMetadataSchemaRenderer.tsx

+    () =>
+      debounce(
+        (newMetadata: JSONObject) => onUpdatePromptMetadata(newMetadata),
+        250


Don't we have a constant val for this in utils?

Whoops, yep

rossdanlm · 2024-02-09T20:16:13Z

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

+      const onError = (err: unknown) => {
+        const message = (err as RequestCallbackError).message ?? null;
+        showNotification({
+          title: "Error updating prompt name",


Typo: should be metadata instead of name

Thanks, good catch

rossdanlm · 2024-02-09T20:17:10Z

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

+        // Just no-op if no callback specified. We could technically perform
+        // client-side updates but that might be confusing


We could technically perform client-side updates but that might be confusing

Nah this is a good call

rossdanlm

K yo next time pls split this into small er diffs, would've been much easier to review. Ex:

UI for showing tabs + JSON editor (no callback implementations)
Implementation (getting model settings + schema renderer)
Implementation (writing model settings, updating the reducer, etc)
Other refactoring work (ex: onSuccess + onError into single callbacks object

zakariassoul · 2024-02-09T20:29:46Z

@rossdanlm my overall feedback:

1- Displaying information/Actions needs to be contextual. Settings is global tab that enables a user to pick a model, and tweak it's behavior in one go.

2- Let's preserve the integrity of the UI. Simplicity is critical for adoption, and we need to say no to adding everything as a button or a tab even if we strongly feel it's useful.

rholinshead · 2024-02-09T20:30:52Z

[UX] cc @zakariassoul Why not make other stuff like stream setting part of metadata as well, since that's not actually "model settings"?

stream IS a model setting for some model parsers (e.g. openai)

zakariassoul · 2024-02-09T20:32:54Z

@rossdanlm, @rholinshead, & @Ankush-lastmile i suggest the following:

1- Put remember context back in the settings tab
2- If and only If function calling needs it's own tab to reduce the clutter when setting it up, then repurpose the 3rd tab for function calling.

rholinshead · 2024-02-09T20:35:43Z

K yo next time pls split this into small er diffs, would've been much easier to review. Ex:

UI for showing tabs + JSON editor (no callback implementations)

Implementation (getting model settings + schema renderer)

Implementation (writing model settings, updating the reducer, etc)

Other refactoring work (ex: onSuccess + onError into single callbacks object

Ya, sorry. I had crunched through this last night and meant to spend some time cleaning it up this morning but kinda decided to put it up as-is and move to other priorities. Will be sure to do that next time

rholinshead · 2024-02-09T20:37:45Z

@rossdanlm, @rholinshead, & @Ankush-lastmile i suggest the following:

1- Put remember context back in the settings tab 2- If and only If function calling needs it's own tab to reduce the clutter when setting it up, then repurpose the 3rd tab for function calling.

@zakariassoul remember_chat_context wasn't in settings. It's a top-level metadata (i.e. prompt.metadata.remember_chat_context) for the prompt in the config. Settings tab is specifically prompt.metadata.model.settings in the config. The UI needs to dynamically render based on the metadata of the config (you can add any metadata in the prompt and it will show in this tab, previously we never showed it in the config), IMO we don't want to make our UI be coupled to these generic keys right now because we have nothing at the SDK level that currently dictates what these keys are.

In the future we CAN update the sdk spec so that remember_chat_context is an explicit key in the prompt metadata type somewhere (e.g. X parser implements ChatModelParser) and then we can leverage that for the UI, but we need to update the config types to ensure all supported chat models do it that way

rholinshead · 2024-02-12T21:10:15Z

Synced with @zakariassoul offline. Instead of its own tab, we can have the metadata below model settings (starting with remember_chat_context). Long-term, we can integrate an accordian-type UX for different "categories", e.g model metadata, lower-pri settings separate from hi-pri ones (will need schema changes). Caveat is we would want remember_chat_context to be part of the hi-pri settings section and not nested under metadata section

# [editor] Add 'Metadata' Tab to Prompt Side Pane Currently the editor does not provide a way to modify general prompt metadata, outside of model settings and parameters. For some of our chat parsers, this means the user cannot modify the 'remember_chat_context' setting. This PR implements the UX for modifying this metadata, adding it as a tab in the side panel and associated with prompt_metadata in the prompt schema: https://github.com/lastmile-ai/aiconfig/assets/5060851/353e2e47-2210-4351-96ef-81eea03c2aab ![Screenshot 2024-02-09 at 11 03 06 AM](https://github.com/lastmile-ai/aiconfig/assets/5060851/cd371457-9fd1-45d4-996e-df470e048196) ![Screenshot 2024-02-09 at 11 03 11 AM](https://github.com/lastmile-ai/aiconfig/assets/5060851/82d646e5-e96a-427c-902e-319f16580b71) Make sure generic metadata can be set as well: <img width="437" alt="Screenshot 2024-02-09 at 11 22 55 AM" src="https://github.com/lastmile-ai/aiconfig/assets/5060851/6e4fbe08-280c-4fa2-9610-4ee86e1cfc2d">

rholinshead · 2024-02-15T17:02:20Z

Ok, updated per discussion with @zakariassoul. Moved the metadata below the settings (if schema exists, for now). The only new change since last PR is this placement (see change in PromptActionBar component) -- cc @rossdanlm for re-review when you have a minute

[editor] Fix Model Parsers prompt_metadata remember_chat_context # [editor] Fix Model Parsers prompt_metadata remember_chat_context Update the relevant model parsers to have default and description for remember_chat_context. Also, remove it from the ones that don't actually support it in their parser --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/1209). * __->__ #1209 * #1208 * #1205

rholinshead force-pushed the pr1205 branch from 2305f5e to b7eeecc Compare February 9, 2024 17:16

This was referenced Feb 9, 2024

[editor][ez] Only Render SidePanel Tab Content When Active Tab #1208

Merged

[editor] Fix Model Parsers prompt_metadata remember_chat_context #1209

Merged

rholinshead marked this pull request as ready for review February 9, 2024 17:29

rholinshead requested review from saqadri, suyoglastmileai, Ankush-lastmile, jonathanlastmileai and rossdanlm as code owners February 9, 2024 17:29

rholinshead commented Feb 9, 2024

View reviewed changes

Ankush-lastmile reviewed Feb 9, 2024

View reviewed changes

Ankush-lastmile approved these changes Feb 9, 2024

View reviewed changes

rossdanlm reviewed Feb 9, 2024

View reviewed changes

rossdanlm approved these changes Feb 9, 2024

View reviewed changes

rholinshead marked this pull request as draft February 12, 2024 21:15

rholinshead force-pushed the pr1205 branch from b7eeecc to 2398cca Compare February 15, 2024 16:27

rholinshead force-pushed the pr1205 branch from 2398cca to a142c18 Compare February 15, 2024 16:40

rholinshead changed the title ~~[editor] Add 'Metadata' Tab to Prompt Side Pane~~ [editor] Add 'Metadata' Section Under Settings Tab Feb 15, 2024

rholinshead marked this pull request as ready for review February 15, 2024 17:02

rholinshead requested review from Ankush-lastmile and rossdanlm February 15, 2024 17:06

saqadri approved these changes Feb 15, 2024

View reviewed changes

rholinshead merged commit a142c18 into main Feb 15, 2024
2 checks passed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[editor] Add 'Metadata' Section Under Settings Tab #1205

[editor] Add 'Metadata' Section Under Settings Tab #1205

rholinshead commented Feb 9, 2024 •

edited

Loading

rholinshead Feb 9, 2024

Ankush-lastmile Feb 9, 2024

rossdanlm Feb 9, 2024

rholinshead Feb 9, 2024

Ankush-lastmile Feb 9, 2024 •

edited

Loading

rossdanlm Feb 9, 2024

Ankush-lastmile left a comment

rossdanlm commented Feb 9, 2024

rossdanlm Feb 9, 2024

rossdanlm Feb 9, 2024

rholinshead Feb 9, 2024

rossdanlm Feb 9, 2024

rholinshead Feb 9, 2024

rossdanlm Feb 9, 2024

rossdanlm left a comment

zakariassoul commented Feb 9, 2024

rholinshead commented Feb 9, 2024

zakariassoul commented Feb 9, 2024

rholinshead commented Feb 9, 2024

rholinshead commented Feb 9, 2024 •

edited

Loading

rholinshead commented Feb 12, 2024

rholinshead commented Feb 15, 2024 •

edited

Loading

		// Just no-op if no callback specified. We could technically perform
		// client-side updates but that might be confusing

[editor] Add 'Metadata' Section Under Settings Tab #1205

[editor] Add 'Metadata' Section Under Settings Tab #1205

Conversation

rholinshead commented Feb 9, 2024 • edited Loading

[editor] Add 'Metadata' Section Under Settings Tab

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

Ankush-lastmile Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankush-lastmile left a comment

Choose a reason for hiding this comment

rossdanlm commented Feb 9, 2024

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 left a comment

Choose a reason for hiding this comment

zakariassoul commented Feb 9, 2024

rholinshead commented Feb 9, 2024

zakariassoul commented Feb 9, 2024

rholinshead commented Feb 9, 2024

rholinshead commented Feb 9, 2024 • edited Loading

rholinshead commented Feb 12, 2024

rholinshead commented Feb 15, 2024 • edited Loading

rholinshead commented Feb 9, 2024 •

edited

Loading

Ankush-lastmile Feb 9, 2024 •

edited

Loading

rholinshead commented Feb 9, 2024 •

edited

Loading

rholinshead commented Feb 15, 2024 •

edited

Loading