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

chore(weave): Eval Compare Perf: Split Query #3295

Merged
merged 11 commits into from
Jan 3, 2025
Merged

Conversation

tssweeney
Copy link
Collaborator

@tssweeney tssweeney commented Dec 19, 2024

Improves performance by splitting the eval page loading into two distinct steps

@circle-job-mirror
Copy link

circle-job-mirror bot commented Dec 19, 2024

@circle-job-mirror
Copy link

@@ -50,18 +58,28 @@ export const useEvaluationComparisonState = (
const orderedCallIds = useMemo(() => {
return getCallIdsOrderedForQuery(evaluationCallIds);
}, [evaluationCallIds]);
const data = useEvaluationComparisonData(entity, project, orderedCallIds);
const summaryData = useEvaluationComparisonSummary(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the major change in this PR: splits up the data fetch into 2 stages

@tssweeney tssweeney changed the title chore(weave): Make compare page faster again chore(weave): Eval Compare Perf: Split Query Dec 20, 2024
@tssweeney tssweeney marked this pull request as ready for review December 20, 2024 19:59
@tssweeney tssweeney requested review from a team as code owners December 20, 2024 19:59
);

const flattenedRows = useMemo(() => {
const rows: FlattenedRow[] = [];
Object.entries(state.data.resultRows).forEach(
Object.entries(state.results.result?.resultRows ?? {}).forEach(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is kinda weird, this used to be a real data object but now its a query response?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like, why are we storing results.result instead of result

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i modified the name to be more clear

Copy link
Member

@gtarpenning gtarpenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, one small question

@tssweeney tssweeney enabled auto-merge (squash) January 3, 2025 20:17
@tssweeney tssweeney merged commit 655cba8 into master Jan 3, 2025
118 of 120 checks passed
@tssweeney tssweeney deleted the tim/faster_compare_page branch January 3, 2025 20:24
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants