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.

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

[OPIK-82] Dot character not allowed on feedback definition values #314

Merged
merged 2 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { useEffect, useMemo, useState } from "react";
import sortBy from "lodash/sortBy";
import isNumber from "lodash/isNumber";
import { Plus, X } from "lucide-react";

import { Button } from "@/components/ui/button";
Expand All @@ -13,23 +14,30 @@ type CategoricalFeedbackDefinitionDetailsProps = {
};

type CategoryField = {
name?: string;
value?: number;
name: string;
value: number | "";
};

function categoryFieldsToDetails(
const generateEmptyCategory = (): CategoryField => {
return {
name: "",
value: "",
};
};

const categoryFieldsToDetails = (
categoryFields: CategoryField[],
): CategoricalFeedbackDefinition["details"] {
): CategoricalFeedbackDefinition["details"] => {
const categories = {} as Record<string, number>;

categoryFields.forEach((categoryField) => {
if (categoryField.name && categoryField.value !== undefined) {
if (categoryField.name && isNumber(categoryField.value)) {
categories[categoryField.name] = categoryField.value;
}
});

return { categories };
}
};

const CategoricalFeedbackDefinitionDetails: React.FunctionComponent<
CategoricalFeedbackDefinitionDetailsProps
Expand All @@ -43,7 +51,7 @@ const CategoricalFeedbackDefinitionDetails: React.FunctionComponent<
})),
"name",
)
: [{}, {}],
: [generateEmptyCategory(), generateEmptyCategory()],
);

const categoricalDetails = useMemo(
Expand Down Expand Up @@ -82,16 +90,22 @@ const CategoricalFeedbackDefinitionDetails: React.FunctionComponent<
placeholder="0.0"
value={category.value}
type="number"
step="any"
onChange={(event) => {
setCategories((prevCategories) => {
return prevCategories.map((prevCategory) => {
if (prevCategory !== category) {
return prevCategory;
}

const value =
event.target.value === ""
? ""
: Number(event.target.value);

return {
...prevCategory,
value: Number(event.target.value),
value,
};
});
});
Expand Down Expand Up @@ -121,7 +135,10 @@ const CategoricalFeedbackDefinitionDetails: React.FunctionComponent<
variant="secondary"
size="sm"
onClick={() => {
setCategories((categories) => [...categories, {}]);
setCategories((categories) => [
...categories,
generateEmptyCategory(),
]);
}}
>
<Plus className="mr-2 size-4" /> Add category
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
import React, { useEffect, useState } from "react";
import isNumber from "lodash/isNumber";

import { Input } from "@/components/ui/input";
import { Label } from "@/components/ui/label";
import { NumericalFeedbackDefinition } from "@/types/feedback-definitions";

const INVALID_DETAILS_VALUE: NumericalFeedbackDefinition["details"] = {
min: 0,
max: 0,
};

type NumericalFeedbackDefinitionDetails = {
min: number | "";
max: number | "";
};

type NumericalFeedbackDefinitionDetailsProps = {
onChange: (details: NumericalFeedbackDefinition["details"]) => void;
details?: NumericalFeedbackDefinition["details"];
Expand All @@ -12,12 +23,18 @@ type NumericalFeedbackDefinitionDetailsProps = {
const NumericalFeedbackDefinitionDetails: React.FunctionComponent<
NumericalFeedbackDefinitionDetailsProps
> = ({ onChange, details }) => {
const [numericalDetails, setNumericalDetails] = useState<
NumericalFeedbackDefinition["details"]
>(details ?? { min: 0, max: 1 });
const [numericalDetails, setNumericalDetails] =
useState<NumericalFeedbackDefinitionDetails>(details ?? { min: 0, max: 1 });

useEffect(() => {
onChange(numericalDetails);
const isValid =
isNumber(numericalDetails.min) && isNumber(numericalDetails.max);

onChange(
isValid
? (numericalDetails as NumericalFeedbackDefinition["details"])
: INVALID_DETAILS_VALUE,
);
}, [numericalDetails, onChange]);

return (
Expand All @@ -29,10 +46,11 @@ const NumericalFeedbackDefinitionDetails: React.FunctionComponent<
placeholder="Min"
value={numericalDetails.min}
type="number"
step="any"
onChange={(event) =>
setNumericalDetails((details) => ({
...details,
min: Number(event.target.value),
min: event.target.value === "" ? "" : Number(event.target.value),
}))
}
/>
Expand All @@ -45,10 +63,11 @@ const NumericalFeedbackDefinitionDetails: React.FunctionComponent<
placeholder="Max"
value={numericalDetails.max}
type="number"
step="any"
onChange={(event) =>
setNumericalDetails((details) => ({
...details,
max: Number(event.target.value),
max: event.target.value === "" ? "" : Number(event.target.value),
}))
}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const AddFeedbackScorePopover: React.FunctionComponent<

const [activeFeedbackDefinition, setActiveFeedbackDefinition] =
useState<FeedbackDefinition>();
const [value, setValue] = useState<number>();
const [value, setValue] = useState<number | "">("");

const workspaceName = useAppStore((state) => state.activeWorkspaceName);
const { data: feedbackDefinitionsData } = useFeedbackDefinitionsList({
Expand All @@ -53,7 +53,7 @@ const AddFeedbackScorePopover: React.FunctionComponent<
const resetState = () => {
setSearch("");
setActiveFeedbackDefinition(undefined);
setValue(undefined);
setValue("");
};

const feedbackDefinitions: FeedbackDefinition[] = useMemo(
Expand Down Expand Up @@ -131,10 +131,15 @@ const AddFeedbackScorePopover: React.FunctionComponent<
<Input
max={activeFeedbackDefinition.details.max}
min={activeFeedbackDefinition.details.min}
onChange={(event) => setValue(Number(event.target.value))}
onChange={(event) =>
setValue(
event.target.value === "" ? "" : Number(event.target.value),
)
}
placeholder="Value"
type="number"
value={String(value)}
step="any"
value={value}
/>
</div>
<div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import React, { useCallback, useEffect, useMemo, useState } from "react";
import debounce from "lodash/debounce";
import React, { useCallback, useEffect, useState } from "react";
import isUndefined from "lodash/isUndefined";
import isNumber from "lodash/isNumber";
import sortBy from "lodash/sortBy";
import { X } from "lucide-react";

import useTraceFeedbackScoreSetMutation from "@/api/traces/useTraceFeedbackScoreSetMutation";
import useTraceFeedbackScoreDeleteMutation from "@/api/traces/useTraceFeedbackScoreDeleteMutation";
import { Input } from "@/components/ui/input";
import { ToggleGroup, ToggleGroupItem } from "@/components/ui/toggle-group";
import DebounceInput from "@/components/shared/DebounceInput/DebounceInput";
import ColoredTag from "@/components/shared/ColoredTag/ColoredTag";
import { ToggleGroup, ToggleGroupItem } from "@/components/ui/toggle-group";
import {
FEEDBACK_DEFINITION_TYPE,
FeedbackDefinition,
Expand Down Expand Up @@ -40,9 +40,11 @@ const AnnotateRow: React.FunctionComponent<AnnotateRowProps> = ({
setCategoryName(feedbackScore?.category_name);
}, [feedbackScore?.category_name, traceId, spanId]);

const [value, setValue] = useState(feedbackScore?.value);
const [value, setValue] = useState<number | "">(
isNumber(feedbackScore?.value) ? feedbackScore?.value : "",
);
useEffect(() => {
setValue(feedbackScore?.value);
setValue(isNumber(feedbackScore?.value) ? feedbackScore?.value : "");
}, [feedbackScore?.value, spanId, traceId]);

const feedbackScoreDeleteMutation = useTraceFeedbackScoreDeleteMutation();
Expand All @@ -58,38 +60,46 @@ const AnnotateRow: React.FunctionComponent<AnnotateRowProps> = ({

const setTraceFeedbackScoreMutation = useTraceFeedbackScoreSetMutation();

const handleChangeValue = useMemo(() => {
return debounce((value: number, categoryName?: string) => {
const handleChangeValue = useCallback(
(value: number, categoryName?: string) => {
setTraceFeedbackScoreMutation.mutate({
categoryName,
name,
spanId,
traceId,
value,
});
}, SET_VALUE_DEBOUNCE_DELAY);

},
// setTraceFeedbackScoreMutation re triggers this memo when it should not
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [name, spanId, traceId]);
[name, spanId, traceId],
);

const renderOptions = (feedbackDefinition: FeedbackDefinition) => {
if (feedbackDefinition.type === FEEDBACK_DEFINITION_TYPE.numerical) {
return (
<Input
<DebounceInput
className="min-w-[100px]"
max={feedbackDefinition.details.max}
min={feedbackDefinition.details.min}
step="any"
dimension="sm"
onChange={(event) => {
const newValue = Number(event.target.value);
delay={SET_VALUE_DEBOUNCE_DELAY}
onChangeValue={(value) => {
const newValue = value === "" ? "" : Number(value);

setValue(newValue);
handleChangeValue(newValue);
if (
isNumber(newValue) &&
newValue >= feedbackDefinition.details.min &&
newValue <= feedbackDefinition.details.max
) {
handleChangeValue(newValue);
}
}}
placeholder="Score"
type="number"
value={String(value)}
value={value}
/>
);
}
Expand Down
2 changes: 1 addition & 1 deletion apps/opik-frontend/src/components/ui/input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const inputVariants = cva(
variants: {
variant: {
default:
"border border-input hover:shadow-sm focus-visible:border-primary hover:disabled:shadow-none",
"border border-input invalid:border-warning hover:shadow-sm focus-visible:border-primary focus-visible:invalid:border-warning hover:disabled:shadow-none",
ghost: "",
},
dimension: {
Expand Down
2 changes: 1 addition & 1 deletion apps/opik-frontend/src/components/ui/textarea.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const Textarea = React.forwardRef<HTMLTextAreaElement, TextareaProps>(
return (
<textarea
className={cn(
"flex min-h-44 w-full rounded-md border border-input bg-background px-3 py-2 text-sm placeholder:text-light-slate hover:shadow-sm hover:disabled:shadow-none focus-visible:outline-none focus-visible:border-primary disabled:cursor-not-allowed disabled:text-muted-gray disabled:bg-muted-disabled disabled:placeholder:text-muted-gray",
"flex min-h-44 w-full rounded-md border border-input bg-background px-3 py-2 text-sm placeholder:text-light-slate invalid:border-warning hover:shadow-sm focus-visible:border-primary focus-visible:outline-none focus-visible:invalid:border-warning disabled:cursor-not-allowed disabled:bg-muted-disabled disabled:text-muted-gray disabled:placeholder:text-muted-gray hover:disabled:shadow-none",
className,
)}
ref={ref}
Expand Down
6 changes: 3 additions & 3 deletions apps/opik-frontend/src/lib/traces.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import md5 from "md5";
import get from "lodash/get";
import isUndefined from "lodash/isUndefined";
import isNumber from "lodash/isNumber";
import { TAG_VARIANTS } from "@/components/ui/tag";
import { ExperimentItem } from "@/types/datasets";

Expand All @@ -14,8 +14,8 @@ export const isObjectSpan = (object: object) => get(object, "trace_id", false);

export const isNumericFeedbackScoreValid = (
{ min, max }: { min: number; max: number },
value?: number,
) => !isUndefined(value) && value >= min && value <= max;
value?: number | "",
) => isNumber(value) && value >= min && value <= max;

export const traceExist = (item: ExperimentItem) =>
item.output || item.input || item.feedback_scores;
Loading