Feat: Add in ability to delete a single output by prompt name in editor #1465

justinkchen · 2024-03-17T01:11:32Z

Adds the ability to delete a single output in the config editor

saqadri

Looks great overall!! Nice first diff 😃 -- just one suggestion about the button moved to https://github.com/lastmile-ai/aiconfig/blob/main/python/src/aiconfig/editor/client/src/components/prompt/PromptMenuButton.tsx

saqadri · 2024-03-17T14:56:42Z

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

              <PromptOutputBar />
+              {!readOnly && prompt.outputs?.length && deleteOutput ? (


Instead of adding the button here, can we add it to the prompt menu?

https://github.com/lastmile-ai/aiconfig/blob/main/python/src/aiconfig/editor/client/src/components/prompt/PromptMenuButton.tsx

I imagine we will have other capabilities like Copy/Duplicate prompt which will also be added in the future, so it will prevent too many buttons everywhere

saqadri

Ship it! 🚀🚀🚀

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

rossdanlm · 2024-03-18T15:02:41Z

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

@@ -66,8 +68,10 @@ export default memo(function PromptsContainer(props: Props) {
            <Flex mt="md">
              {!readOnly && (
                <PromptMenuButton
+                  prompt={prompt}
                  promptId={prompt._ui.id}


nit: we don't need to pass in promptId anymore if passing in prompt

Made a comment above -- we shouldn't pass prompt and we can actually just pass callbacks (and a new showDeleteOutput or canDeleteOutput prop)

rossdanlm · 2024-03-18T15:04:34Z

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

+        if (prompt._ui.id === action.id) {
+          return {
+            ...prompt,
+            outputs: undefined,
+          };
+        } else {
+          return prompt;
+        }


We have a utils reducer method called reduceReplacePrompt(). Would prefer to use that to keep things cleaner and more abstracted

+1, it will handle selecting the correct prompt by id

rossdanlm · 2024-03-18T15:08:52Z

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

+  const deleteOutput = useCallback(
+    async () => await onDeleteOutput(promptId),
+    [promptId, onDeleteOutput]
+  );
+
+  const deletePrompt = useCallback(
+    async () => await onDeletePrompt(promptId),
+    [promptId, onDeletePrompt]
+  );


This is a front-end thing that Ryan taught me. Since PromptMenuButton is already wrapped with a memo component, it's actually more overhead for us to wrap these sub-functions in a useCallback() hook. Here useCallback() checks for changes to any dependencies, but in our case that's simply changes to the props, which is already memoized in the PromptMenuButton component itself, so in this case it adds overhead. Would be better to either:

Keep inlined

Keep separate but don't wrap with useCallback()

Oh just noticed as well but we should just construct the onDeletePrompt and onDeleteOutput callbacks in the parent, since the promptId is known there. Then promptId wouldn't even need to be passed, just the callbacks. Similarly that would make PromptMenuButton memoization actually work as intended since the inline function currently prevents memoization from working (new function prop each time). So in PromptsContainer we could have

const promptId = prompt._ui.id; const onDeletePrompt = props.onDeletePrompt; const onDeleteOutput = props.onDeleteOutput; const deletePrompt = useCallback(() => onDeletePrompt(promptId), [onDeletePrompt, promptId]); const deleteOutput = useCallback(() => onDeleteOutput(promptId), [onDeleteOutput, promptId]);

And they can be passed through the props to PromptMenuButton

rossdanlm · 2024-03-18T15:09:24Z

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

+          <Menu.Item
+            icon={<IconEraser size={16} />}
+            color="white"
+            onClick={deleteOutput}
+          >


cc @rholinshead for whether this would work with light mode theming

Good catch. I think we can remove color here and it will have the correct color we want. To test this, you can set mode="gradio" and themeMode="dark" and themeMode="light" props for AIConfigEditor in the LocalEditor component

rossdanlm · 2024-03-18T15:11:35Z

python/src/aiconfig/editor/server/server.py

@@ -811,9 +812,32 @@ def _op(

    signature: dict[str, Type[Any]] = {}
    return run_aiconfig_operation_with_request_json(
-        aiconfig, request_json, f"method_", _op, signature
+        aiconfig, request_json, f"method_{method_name}", _op, signature


nice thanks!

Actually upon further inspection, I think this should just be method_name, not f"method_{method_name}", don't think we did this right the first time:

aiconfig/python/src/aiconfig/editor/server/server_utils.py

Lines 501 to 503 in f0f0522

return run_aiconfig_operation_with_op_args(

aiconfig, operation_name, operation, Ok(op_args_ok)

)

rossdanlm · 2024-03-18T15:21:13Z

python/src/aiconfig/editor/server/server.py

+        ).to_flask_format()
+
+    signature: dict[str, Type[Any]] = {"prompt_name": str}
+    operation = make_op_run_method(method_name)


Oh cool, TIL that we already have existing API for this:

aiconfig/python/src/aiconfig/schema.py

Line 1005 in f0f0522

def delete_output(self, prompt_name: str):

. Thought it was for deleting entire outputs only. We can probly clean this up and delete the line existing_outputs = prompt.outputs. cc @Ankush-lastmile do you remember why this line might have been needed?

rossdanlm · 2024-03-18T15:22:12Z

python/src/aiconfig/editor/server/server.py

+    """
+    Clears a single outputs in the server state's AIConfig based on prompt name.
+    """
+    method_name = MethodName("delete_output")


nit: Based on comment above, we probably don't need to inline this since it's only used once in operation = ... line, but not a blocker

rossdanlm

Besides the useCallbacks and potential light mode issue, there are no functional blockers I see on my end. I'm happy to unblock and land for now and we can fix lingering nits in a followup! Let me know whether you prefer to fix these small things in this PR (will wait for you) or do a followup (will merge this for now)

rossdanlm · 2024-03-18T15:29:07Z

Btw for failing blockers (on autoformatting), you can use either of these commands to reformat and get unblocked (I think, not 100% sure):

black --exclude '/\.sl\b' . --line-length=79
fd --glob '*.py' python/src/aiconfig/editor | xargs python -m 'scripts.lint' --mode=fix --files

rholinshead

Overall this looks great, thanks! Just requesting changes for some small but important changes in the comments

rholinshead · 2024-03-18T15:23:55Z

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

@@ -190,7 +190,9 @@ export default memo(function PromptContainer({

          {prompt.outputs && prompt.outputs.length > 0 && (
            <>
+            <Flex justify="space-between" direction="column">


What does this look like / is this needed now that the button is moved to the menu?

rholinshead · 2024-03-18T15:26:39Z

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

  onDeletePrompt,
 }: {
  promptId: string;
+  prompt: ClientPrompt;


Passing prompt through just to access the output length will mean this component is re-rendered any time any part of the prompt changes (e.g. if typing in the input). That will result in the React memo constantly re-calculating the diff to check if it should re-render, and also re-rendering unnecessarily. Instead, it would be better to have a prop showDeleteOutput which is set to true if the prompt.outputs.length > 0 in the parent component

rholinshead · 2024-03-18T15:34:04Z

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

+  const deleteOutput = useCallback(
+    async () => await onDeleteOutput(promptId),
+    [promptId, onDeleteOutput]
+  );
+
+  const deletePrompt = useCallback(
+    async () => await onDeletePrompt(promptId),
+    [promptId, onDeletePrompt]
+  );


Oh just noticed as well but we should just construct the onDeletePrompt and onDeleteOutput callbacks in the parent, since the promptId is known there. Then promptId wouldn't even need to be passed, just the callbacks. Similarly that would make PromptMenuButton memoization actually work as intended since the inline function currently prevents memoization from working (new function prop each time). So in PromptsContainer we could have

const promptId = prompt._ui.id; const onDeletePrompt = props.onDeletePrompt; const onDeleteOutput = props.onDeleteOutput; const deletePrompt = useCallback(() => onDeletePrompt(promptId), [onDeletePrompt, promptId]); const deleteOutput = useCallback(() => onDeleteOutput(promptId), [onDeleteOutput, promptId]);

And they can be passed through the props to PromptMenuButton

rholinshead · 2024-03-18T15:36:03Z

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

+          <Menu.Item
+            icon={<IconEraser size={16} />}
+            color="white"
+            onClick={deleteOutput}
+          >


Good catch. I think we can remove color here and it will have the correct color we want. To test this, you can set mode="gradio" and themeMode="dark" and themeMode="light" props for AIConfigEditor in the LocalEditor component

rholinshead · 2024-03-18T15:40:25Z

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

@@ -34,6 +35,7 @@ type Props = {
  ) => Promise<void>;
  prompts: ClientPrompt[];
  runningPromptId?: string;
+  readOnly?: boolean;


This can be removed as unused, since it's already obtained from context.

rholinshead · 2024-03-18T15:42:11Z

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

@@ -66,8 +68,10 @@ export default memo(function PromptsContainer(props: Props) {
            <Flex mt="md">
              {!readOnly && (
                <PromptMenuButton
+                  prompt={prompt}
                  promptId={prompt._ui.id}


Made a comment above -- we shouldn't pass prompt and we can actually just pass callbacks (and a new showDeleteOutput or canDeleteOutput prop)

rholinshead · 2024-03-18T15:43:44Z

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

+        if (prompt._ui.id === action.id) {
+          return {
+            ...prompt,
+            outputs: undefined,
+          };
+        } else {
+          return prompt;
+        }


+1, it will handle selecting the correct prompt by id

…r cleanliness and reusability

rossdanlm · 2024-03-19T05:25:58Z

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

+    <>
+      {!readOnly && (
+        <PromptMenuButton
+          showDeleteOutput={!!prompt.outputs?.length}


nit: feel prompt.outputs?.length > 0 is easier to read and I don't really like JS implicitly "falsey-ness" but don't really care

it actually complains if you use this syntax unelss you do a (prompt.outputs?.length || 0) > 0 it seems, so thats why i kept it with this way

rossdanlm · 2024-03-19T05:30:59Z

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

-              {!readOnly && (
-                <PromptMenuButton
-                  promptId={prompt._ui.id}
-                  onDeletePrompt={() => props.onDeletePrompt(prompt._ui.id)}
-                />
-              )}


omg this had me confused so hard wondering wtf was going on, but this is the PromptsContainer (with an s), not PromptContainer :S

I like this change, I feel it makes sense to put with the individual prompt component anyways

rossdanlm · 2024-03-19T05:33:18Z

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

+                onDeletePrompt={props.onDeletePrompt}
+                onDeleteOutput={props.onDeleteOutput}


rossdanlm · 2024-03-19T05:35:53Z

python/src/aiconfig/editor/client/src/shared/types.ts

@@ -66,6 +66,7 @@ export function aiConfigToClientConfig(aiconfig: AIConfig): ClientAIConfig {
 export type LogEvent =
  | "ADD_PROMPT"
  | "CLEAR_OUTPUTS"
+  | "DELETE_OUTPUT"


nit: Would like to use wording "CLEAR_OUTPUT" to match with our UI, but again not a blocker or a big deal

i think maybe as a bigger refactor of the api? i made it clear output at first but then found that all the apis reference delete output so wanted to stay consistent there.

rossdanlm

Nice, I think this is great! Will let @rholinshead do a final pass before merging!

rholinshead

Very nice - thanks!

rholinshead · 2024-03-19T15:35:52Z

Ah, looks like the formatting lint is still failing. I'll override to merge to unblock and then I can do a quick PR with the formatting. I'd recommend setting up black formatter extension in vs code (if you use it) so that it formats automatically on save instead of needing the command

#1474) # Remove Unused os Import + Run Black Format + Add Missing Dep & Rebuild Following up with minor changes from #1465 (comment) Mainly, removed unused `os` import and save `server.py` so that black formatter runs to pass the lint. While testing, noticed lint in `LocalEditor` for missing dep so added that as well. Just for good measure, rebuilt the client. Test Plan: - Clear output works - Lint passes on PR

justinkchen requested review from saqadri, rholinshead, suyoglastmileai, Ankush-lastmile, jonathanlastmileai and rossdanlm as code owners March 17, 2024 01:11

Feat: Add in ability to delete a single output by prompt name in editor

0c0f78d

justinkchen force-pushed the justin_chen-delete-single-prompt-output branch from 8ae319f to 0c0f78d Compare March 17, 2024 01:55

saqadri reviewed Mar 17, 2024

View reviewed changes

justinkchen added 2 commits March 17, 2024 14:25

Update to move delete output button to context menu

c7c2d17

Fix client to pass in prompt name

c958a1d

saqadri approved these changes Mar 17, 2024

View reviewed changes

python/src/aiconfig/editor/client/src/components/prompt/PromptMenuButton.tsx Outdated Show resolved Hide resolved

Update styling

8b4b559

rossdanlm reviewed Mar 18, 2024

View reviewed changes

rossdanlm approved these changes Mar 18, 2024

View reviewed changes

rholinshead requested changes Mar 18, 2024

View reviewed changes

justinkchen added 2 commits March 19, 2024 00:03

Update client changes and server code changes based on PR comments fo…

cf0cf99

…r cleanliness and reusability

Style tweaks

c299c4b

rossdanlm reviewed Mar 19, 2024

View reviewed changes

rossdanlm approved these changes Mar 19, 2024

View reviewed changes

rholinshead approved these changes Mar 19, 2024

View reviewed changes

rholinshead merged commit d57f077 into lastmile-ai:main Mar 19, 2024
1 of 2 checks passed

rholinshead mentioned this pull request Mar 19, 2024

Remove Unused os Import + Run Black Format + Add Missing Dep & Rebuild #1474

Merged

		<PromptOutputBar />
		{!readOnly && prompt.outputs?.length && deleteOutput ? (

	return run_aiconfig_operation_with_op_args(
	aiconfig, operation_name, operation, Ok(op_args_ok)
	)

		onDeletePrompt={props.onDeletePrompt}
		onDeleteOutput={props.onDeleteOutput}

Feat: Add in ability to delete a single output by prompt name in editor #1465

Feat: Add in ability to delete a single output by prompt name in editor #1465

Conversation

justinkchen commented Mar 17, 2024 • edited Loading

saqadri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saqadri left a comment

Choose a reason for hiding this comment

rossdanlm Mar 18, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm Mar 18, 2024 • edited Loading

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

rossdanlm commented Mar 18, 2024

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

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 Mar 19, 2024 • 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 left a comment

Choose a reason for hiding this comment

rholinshead left a comment

Choose a reason for hiding this comment

rholinshead commented Mar 19, 2024

justinkchen commented Mar 17, 2024 •

edited

Loading

rossdanlm Mar 18, 2024 •

edited

Loading

rossdanlm Mar 18, 2024 •

edited

Loading

rholinshead left a comment •

edited

Loading

rossdanlm Mar 19, 2024 •

edited

Loading