-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(ui): bulk annotate traces and spans from the selection bar #7940
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import { useMutation, useQueryClient } from "@tanstack/react-query"; | ||
| import chunk from "lodash/chunk"; | ||
| import { AxiosError } from "axios"; | ||
|
|
||
| import api, { | ||
| COMPARE_EXPERIMENTS_KEY, | ||
| SPANS_KEY, | ||
| SPANS_REST_ENDPOINT, | ||
| TRACE_KEY, | ||
| TRACES_KEY, | ||
| } from "@/api/api"; | ||
| import { FEEDBACK_SCORE_TYPE } from "@/types/traces"; | ||
| import { useToast } from "@/ui/use-toast"; | ||
| import { extractErrorMessage } from "@/lib/errors"; | ||
| import { | ||
| FeedbackScoreBatchEntry, | ||
| MAX_FEEDBACK_SCORES_PER_BATCH, | ||
| } from "@/lib/feedback-scores"; | ||
|
|
||
| type UseSpanFeedbackScoreBatchSetMutationParams = { | ||
| projectName: string; | ||
| scores: FeedbackScoreBatchEntry[]; | ||
| }; | ||
|
|
||
| const useSpanFeedbackScoreBatchSetMutation = () => { | ||
| const queryClient = useQueryClient(); | ||
| const { toast } = useToast(); | ||
|
|
||
| return useMutation({ | ||
| mutationFn: async ({ | ||
| projectName, | ||
| scores, | ||
| }: UseSpanFeedbackScoreBatchSetMutationParams) => { | ||
| for (const scoresChunk of chunk(scores, MAX_FEEDBACK_SCORES_PER_BATCH)) { | ||
| await api.put(`${SPANS_REST_ENDPOINT}feedback-scores`, { | ||
| scores: scoresChunk.map((score) => ({ | ||
| id: score.id, | ||
| // The backend groups the batch by project name and derives project_id from it, | ||
| // falling back to the default project when it is blank, so it has to be sent. | ||
| project_name: projectName, | ||
| name: score.name, | ||
| category_name: score.categoryName, | ||
| value: score.value, | ||
| reason: score.reason, | ||
| source: FEEDBACK_SCORE_TYPE.ui, | ||
| })), | ||
| }); | ||
| } | ||
| }, | ||
| onError: (error: AxiosError) => { | ||
| toast({ | ||
| title: "Error", | ||
| description: extractErrorMessage(error), | ||
| variant: "destructive", | ||
| }); | ||
| }, | ||
| onSettled: async () => { | ||
| // Span scores also roll up onto traces and experiment views — same keys as | ||
| // useTraceFeedbackScoreSetMutation's span branch + its always-on set. | ||
| await queryClient.invalidateQueries({ queryKey: [SPANS_KEY] }); | ||
| await queryClient.invalidateQueries({ queryKey: ["spans-columns"] }); | ||
| await queryClient.invalidateQueries({ queryKey: ["spans-statistic"] }); | ||
| await queryClient.invalidateQueries({ queryKey: [TRACE_KEY] }); | ||
| await queryClient.invalidateQueries({ queryKey: [TRACES_KEY] }); | ||
| await queryClient.invalidateQueries({ queryKey: ["traces-columns"] }); | ||
| await queryClient.invalidateQueries({ queryKey: ["traces-statistic"] }); | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: ["experiment-items-statistic"], | ||
| }); | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: ["experiments-columns"], | ||
|
Comment on lines
+60
to
+71
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-entity invalidation lacks coverageThe dialog test mocks both hooks, so regressions in broad span-batch invalidation or per-ID Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by Other fix methodsPrompt for AI Agents |
||
| }); | ||
| await queryClient.invalidateQueries({ queryKey: ["experiment"] }); | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: [COMPARE_EXPERIMENTS_KEY], | ||
| }); | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| export default useSpanFeedbackScoreBatchSetMutation; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import { useMutation, useQueryClient } from "@tanstack/react-query"; | ||
| import chunk from "lodash/chunk"; | ||
| import { AxiosError } from "axios"; | ||
|
|
||
| import api, { | ||
| COMPARE_EXPERIMENTS_KEY, | ||
| TRACE_KEY, | ||
| TRACES_KEY, | ||
| TRACES_REST_ENDPOINT, | ||
| } from "@/api/api"; | ||
| import { FEEDBACK_SCORE_TYPE } from "@/types/traces"; | ||
| import { useToast } from "@/ui/use-toast"; | ||
| import { extractErrorMessage } from "@/lib/errors"; | ||
| import { | ||
| FeedbackScoreBatchEntry, | ||
| MAX_FEEDBACK_SCORES_PER_BATCH, | ||
| } from "@/lib/feedback-scores"; | ||
|
|
||
| type UseTraceFeedbackScoreBatchSetMutationParams = { | ||
| projectName: string; | ||
| scores: FeedbackScoreBatchEntry[]; | ||
| }; | ||
|
|
||
| const useTraceFeedbackScoreBatchSetMutation = () => { | ||
| const queryClient = useQueryClient(); | ||
| const { toast } = useToast(); | ||
|
|
||
| return useMutation({ | ||
| mutationFn: async ({ | ||
| projectName, | ||
| scores, | ||
| }: UseTraceFeedbackScoreBatchSetMutationParams) => { | ||
| for (const scoresChunk of chunk(scores, MAX_FEEDBACK_SCORES_PER_BATCH)) { | ||
| await api.put(`${TRACES_REST_ENDPOINT}feedback-scores`, { | ||
| scores: scoresChunk.map((score) => ({ | ||
| id: score.id, | ||
| // The backend groups the batch by project name and derives project_id from it, | ||
| // falling back to the default project when it is blank, so it has to be sent. | ||
| project_name: projectName, | ||
| name: score.name, | ||
|
Comment on lines
+34
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-project feedback scores are persistedEach chunk sends Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents |
||
| category_name: score.categoryName, | ||
| value: score.value, | ||
| reason: score.reason, | ||
| source: FEEDBACK_SCORE_TYPE.ui, | ||
| })), | ||
| }); | ||
| } | ||
| }, | ||
| onError: (error: AxiosError) => { | ||
| toast({ | ||
| title: "Error", | ||
| description: extractErrorMessage(error), | ||
| variant: "destructive", | ||
| }); | ||
| }, | ||
| onSettled: async (data, error, variables) => { | ||
| // Same invalidation set as useTraceFeedbackScoreSetMutation. | ||
| await queryClient.invalidateQueries({ queryKey: [TRACES_KEY] }); | ||
| await queryClient.invalidateQueries({ queryKey: ["traces-columns"] }); | ||
| await queryClient.invalidateQueries({ queryKey: ["traces-statistic"] }); | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: ["experiment-items-statistic"], | ||
| }); | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: ["experiments-columns"], | ||
| }); | ||
| await queryClient.invalidateQueries({ queryKey: ["experiment"] }); | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: [COMPARE_EXPERIMENTS_KEY], | ||
| }); | ||
|
|
||
| const traceIds = [...new Set(variables.scores.map((score) => score.id))]; | ||
| await Promise.all( | ||
| traceIds.map((traceId) => | ||
| queryClient.invalidateQueries({ | ||
| queryKey: [TRACE_KEY, { traceId }], | ||
| }), | ||
| ), | ||
| ); | ||
|
Comment on lines
+58
to
+79
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bulk annotations leave dependent views stale
Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents |
||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| export default useTraceFeedbackScoreBatchSetMutation; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -637,3 +637,18 @@ export const combineExperimentScoresAsMap = (row: { | |
|
|
||
| return result; | ||
| }; | ||
|
|
||
| // A single entry of the traces/spans batch feedback score payload: the entity id plus the | ||
| // score to set on it. | ||
| export type FeedbackScoreBatchEntry = { | ||
| id: string; | ||
| name: string; | ||
| value: number; | ||
| categoryName?: string; | ||
| reason?: string; | ||
|
Comment on lines
+643
to
+648
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bulk annotation rejects valid decimal scores
Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents |
||
| }; | ||
|
|
||
| // The backend caps a feedback score batch at 1000 items | ||
| // (FeedbackScoreBatchContainer: `@Size(min = 1, max = 1000)`), and annotating a selection | ||
| // produces `rows * scores` entries, so larger payloads have to be split. | ||
| export const MAX_FEEDBACK_SCORES_PER_BATCH = 1000; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { render, screen, fireEvent, waitFor } from "@testing-library/react"; | ||
| import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; | ||
| import { ReactNode } from "react"; | ||
| import { TooltipProvider } from "@/ui/tooltip"; | ||
| import { TRACE_DATA_TYPE } from "@/hooks/useTracesOrSpansList"; | ||
| import { Span, Trace } from "@/types/traces"; | ||
| import { UpdateFeedbackScoreData } from "@/v2/pages-shared/traces/TraceDetailsPanel/TraceAnnotateViewer/types"; | ||
| import AddAnnotationDialog from "./AddAnnotationDialog"; | ||
|
|
||
| const mockSetTraceFeedbackScores = vi.fn(); | ||
| const mockSetSpanFeedbackScores = vi.fn(); | ||
|
|
||
| vi.mock("@/api/traces/useTraceFeedbackScoreBatchSetMutation", () => ({ | ||
| default: () => ({ | ||
| mutateAsync: mockSetTraceFeedbackScores, | ||
| isPending: false, | ||
| }), | ||
| })); | ||
| vi.mock("@/api/traces/useSpanFeedbackScoreBatchSetMutation", () => ({ | ||
| default: () => ({ | ||
| mutateAsync: mockSetSpanFeedbackScores, | ||
| isPending: false, | ||
| }), | ||
| })); | ||
|
|
||
| vi.mock("@/ui/use-toast", () => ({ | ||
| useToast: () => ({ toast: vi.fn() }), | ||
| })); | ||
|
|
||
| type StubFeedbackScoresEditorProps = { | ||
| onUpdateFeedbackScore: (update: UpdateFeedbackScoreData) => void; | ||
| onDeleteFeedbackScore: (name: string) => void; | ||
| }; | ||
|
|
||
| vi.mock( | ||
| "@/v2/pages-shared/traces/FeedbackScoresEditor/FeedbackScoresEditor", | ||
| () => { | ||
| const Stub = ({ | ||
| onUpdateFeedbackScore, | ||
| onDeleteFeedbackScore, | ||
| }: StubFeedbackScoresEditorProps) => ( | ||
| <div data-testid="feedback-scores-editor"> | ||
| <button | ||
| onClick={() => | ||
| onUpdateFeedbackScore({ | ||
| name: "Relevance", | ||
| value: 1, | ||
| categoryName: "Yes", | ||
| reason: "looks good", | ||
| }) | ||
| } | ||
| > | ||
| set-score | ||
| </button> | ||
| <button onClick={() => onDeleteFeedbackScore("Relevance")}> | ||
| clear-score | ||
| </button> | ||
| </div> | ||
| ); | ||
| Stub.displayName = "FeedbackScoresEditorStub"; | ||
| const StubHeader = () => null; | ||
| StubHeader.displayName = "FeedbackScoresEditorHeaderStub"; | ||
| const StubFooter = () => null; | ||
| StubFooter.displayName = "FeedbackScoresEditorFooterStub"; | ||
| Stub.Header = StubHeader; | ||
| Stub.Footer = StubFooter; | ||
| return { default: Stub }; | ||
| }, | ||
| ); | ||
|
|
||
| describe("AddAnnotationDialog", () => { | ||
| let queryClient: QueryClient; | ||
|
|
||
| beforeEach(() => { | ||
| queryClient = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { retry: false }, | ||
| mutations: { retry: false }, | ||
| }, | ||
| }); | ||
| vi.clearAllMocks(); | ||
| mockSetTraceFeedbackScores.mockResolvedValue(undefined); | ||
| mockSetSpanFeedbackScores.mockResolvedValue(undefined); | ||
| }); | ||
|
|
||
| const wrapper = ({ children }: { children: ReactNode }) => ( | ||
| <QueryClientProvider client={queryClient}> | ||
| <TooltipProvider>{children}</TooltipProvider> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| const mockTrace: Trace = { | ||
| id: "row-1", | ||
| name: "Test Trace", | ||
| input: { prompt: "test input" }, | ||
| output: { response: "test output" }, | ||
| start_time: "2024-01-01T00:00:00Z", | ||
| end_time: "2024-01-01T00:00:01Z", | ||
| duration: 1000, | ||
| created_at: "2024-01-01T00:00:00Z", | ||
| last_updated_at: "2024-01-01T00:00:01Z", | ||
| tags: [], | ||
| metadata: {}, | ||
| feedback_scores: [], | ||
| comments: [], | ||
| project_id: "project-1", | ||
| }; | ||
|
|
||
| const ROWS: Array<Trace | Span> = [mockTrace, { ...mockTrace, id: "row-2" }]; | ||
|
|
||
| const renderDialog = (type: TRACE_DATA_TYPE) => | ||
| render( | ||
| <AddAnnotationDialog | ||
| rows={ROWS} | ||
| open | ||
| setOpen={vi.fn()} | ||
| projectName="project-name" | ||
| type={type} | ||
| />, | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| const EXPECTED_SCORES = [ | ||
| { | ||
| id: "row-1", | ||
| name: "Relevance", | ||
| value: 1, | ||
| categoryName: "Yes", | ||
| reason: "looks good", | ||
| }, | ||
| { | ||
| id: "row-2", | ||
| name: "Relevance", | ||
| value: 1, | ||
| categoryName: "Yes", | ||
| reason: "looks good", | ||
| }, | ||
| ]; | ||
|
|
||
| it("sends one batch of trace scores, carrying the project name", async () => { | ||
| renderDialog(TRACE_DATA_TYPE.traces); | ||
|
|
||
| fireEvent.click(screen.getByRole("button", { name: "set-score" })); | ||
| fireEvent.click(screen.getByTestId("apply-annotation-button")); | ||
|
|
||
| await waitFor(() => | ||
| expect(mockSetTraceFeedbackScores).toHaveBeenCalledTimes(1), | ||
| ); | ||
| expect(mockSetTraceFeedbackScores).toHaveBeenCalledWith({ | ||
| projectName: "project-name", | ||
| scores: EXPECTED_SCORES, | ||
| }); | ||
| expect(mockSetSpanFeedbackScores).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("routes span selections to the spans endpoint, not the traces one", async () => { | ||
| renderDialog(TRACE_DATA_TYPE.spans); | ||
|
|
||
| fireEvent.click(screen.getByRole("button", { name: "set-score" })); | ||
| fireEvent.click(screen.getByTestId("apply-annotation-button")); | ||
|
|
||
| await waitFor(() => | ||
| expect(mockSetSpanFeedbackScores).toHaveBeenCalledTimes(1), | ||
| ); | ||
| expect(mockSetSpanFeedbackScores).toHaveBeenCalledWith({ | ||
| projectName: "project-name", | ||
| scores: EXPECTED_SCORES, | ||
| }); | ||
| expect(mockSetTraceFeedbackScores).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("enables apply only while at least one score is set", () => { | ||
| renderDialog(TRACE_DATA_TYPE.traces); | ||
|
|
||
| expect(screen.getByTestId("apply-annotation-button")).toBeDisabled(); | ||
|
|
||
| fireEvent.click(screen.getByRole("button", { name: "set-score" })); | ||
| expect(screen.getByTestId("apply-annotation-button")).toBeEnabled(); | ||
|
|
||
| fireEvent.click(screen.getByRole("button", { name: "clear-score" })); | ||
| expect(screen.getByTestId("apply-annotation-button")).toBeDisabled(); | ||
| }); | ||
| }); |
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.
Duplicated batch mutation logic drifts
This hook duplicates the chunking and
FeedbackScoreBatchEntry/FEEDBACK_SCORE_TYPE.uipayload mapping fromuseTraceFeedbackScoreBatchSetMutation, so future batching or payload changes can make trace and span updates diverge — should we extract a shared helper in the traces API package, parameterized by endpoint while preserving each hook's cache invalidation?Want Baz to fix this for you? Activate Fixer