feat: Show invocation param errors within form #5559

cephalization · 2024-11-26T21:32:58Z

This involved a decently large refactor of InvocationParametersFormFields:

Remove redundant gql query, just load definitions from store after they are populated via ModelSupportedParametersFetcher
Update onChange to use referentially stable callbacks only, computations occur in store instead of inside of useCallback now
Memoize invocation parameter value mapping
Introduce react hook form, mirroring values and validating them
Wrap all form inputs in react hook form controllers, adding validation and passing through error messages
Fix controlled/uncontrolled warning in AzureOpenAiModelConfigFormField
Remove tools when tool_choice is not supported in incoming params

Resolves #5540
Resolves #5546

cephalization · 2024-11-26T21:45:41Z

app/src/pages/playground/InvocationParametersFormFields.tsx

@@ -1,22 +1,19 @@
-import React, { useCallback } from "react";
-import { graphql, useLazyLoadQuery } from "react-relay";
+import React, { useCallback, useLayoutEffect, useMemo } from "react";


Large but relevant diff

app/src/pages/playground/InvocationParametersFormFields.tsx

Parker-Stafford · 2024-12-02T19:27:26Z

app/src/pages/playground/InvocationParametersFormFields.tsx

@@ -46,7 +61,7 @@ const InvocationParameterFormField = ({
        <Slider
          label={field.label}
          isRequired={field.required}
-          value={value}
+          defaultValue={value}


why no controller around the slider?

I am just using controller to map errors to inputs, sliders can't have errors so I didn't see the point

sounds good, if it's low lift i might add it but yeah not necessary

There isn't a possible error a slider can have since the value range is always constrained. I'm not sure what we could add to it

app/src/pages/playground/InvocationParametersFormFields.tsx

Parker-Stafford · 2024-12-02T19:31:06Z

app/src/pages/playground/InvocationParametersFormFields.tsx

+        const existingParameter = instanceInvocationParameters.find((p) =>
+          areInvocationParamsEqual(p, field)
+        );
+        const value = existingParameter
+          ? getInvocationParameterValue(field, existingParameter)
+          : undefined;
+        return {
+          [field.invocationName!]: value ?? null,
+        };


does this get defaults values from the server? I was working in this before i left and was running into issues here. It looks like we won't get the default no matter what here

the default will be merged into the store whenever model/provider is chosen. The only time you will lose the default value is if you manually edit/delete it

app/src/pages/playground/ModelConfigButton.tsx

Parker-Stafford · 2024-12-02T19:42:59Z

Couple of issues i noticed

the param form doesnt show up until you start typing a deployment (model) name for azure, probably fine but the form does not actually depend on that for azure. (probably fine)
the error for the form field only shows up on blur can be a bit confusing when the other error shows up first (nice to have)

Screen.Recording.2024-12-02.at.11.35.16.AM.mov

saved config is overwritten when fields are dirty and are not - updated in this pr fix(playground): update saved model config to overwrite all model config if present #5544 to always respect saved config and defaults not dirty (debatable) and definitely not non dirty fields

Screen.Recording.2024-12-02.at.11.37.10.AM.mov

This involved a decently large refactor of InvocationParametersFormFields: - Remove redundant gql query, just load definitions from store after they are populated via ModelSupportedParametersFetcher - Update onChange to use referentially stable callbacks only, computations occur in store instead of inside of useCallback now - Memoize invocation parameter value mapping - Introduce react hook form, mirroring values and validating them - Wrap all form inputs in react hook form controllers, adding validation and passing through error messages - Fix controlled/uncontrolled warning in AzureOpenAiModelConfigFormField

…nsupported

Form values stay in-tact when model changes, unless that model has different parameter fields

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Nov 26, 2024

cephalization commented Nov 26, 2024

View reviewed changes