perf(weave): address data loading perf issue on eval compare

[chance-wnb]

Description

This addresses a part of the evaluation comparison performance issue. No the customer scenario will no longer crash.

UI wise, there is no noticeable behavior change.

Here I attach a video(internal) to explain the change.

Testing

This PR is manually tested against the customer scenario and locally.

[circle-job-mirror]

Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=e0ff91d73a74f2bddded908c73577a5c11c57ed8

[gtarpenning]

I played around with it and its really nice, I did notice one small bug which is that if you click the next example button rapidly, we get into a state where the query fires but the loading state if permanently stuck. I think this has to do with the mutation counter but i'm not sure. A very edge case, but because we don't have a way of going to a specific example (which I think we should eventually add btw), I could imagine a user spamming the next example button to get to the one they know is interesting. We might be able to get away with just loading 5 ahead or something 🤷🏻

[chance-wnb]

I played around with it and its really nice, I did notice one small bug which is that if you click the next example button rapidly, we get into a state where the query fires but the loading state if permanently stuck. I think this has to do with the mutation counter but i'm not sure. A very edge case, but because we don't have a way of going to a specific example (which I think we should eventually add btw), I could imagine a user spamming the next example button to get to the one they know is interesting. We might be able to get away with just loading 5 ahead or something 🤷🏻

Great job on discovering this bug, it is a solid bug. This is because in the loadRowDataIntoCache() I was passing cachedRowData.current which is the at-the-time value of the cache, which does not reflect the latest value during race conditions. My fix is to change the parameter to pass the refs (the boxed object), and the issue went away.

[gtarpenning]

lgtm, love the code removal. I think the cache handling is neat. I trust that the mutation counter is common practice, and after watching your video it makes sense; @tssweeney might be curious about it!

[chance-wnb]

lgtm, love the code removal. I think the cache handling is neat. I trust that the mutation counter is common practice, and after watching your video it makes sense; @tssweeney might be curious about it!

In regard to the mutation counter thingy, I now feel that semantically giving it a better name seems to justify its usage better. now I renamed it to cacheVersion.

[tssweeney]

Given this, you are no long populating inputs in

export type EvaluationComparisonResults = {
  // Inputs are the intersection of all inputs used in the evaluations.
  // Note, we are able to "merge" the same input digest even if it is
  // used in different evaluations.
  inputs: {
    [rowDigest: string]: DatasetRow;
  };

as such, I would either:
a) populate this field using other forms (ex. the predict_and_score input) or
b) remove this field from the type definition and force resolution of any typing issues caused from this

[tssweeney]

i think (b) corresponds with this PR

[chance-wnb]

Thanks for catching this. It was an oversight, it should be removed.

[tssweeney]

in @andrewtruong's upcoming PR, we can't count that the data comes from a dataset (gasp!) it might be good to at least leave a comment here as this would be a possible location to record the raw predict_and_score inputs as the presumed data row.

[tssweeney]

my reading of the implementation of useExampleCompareData is that filteredRows is only used to get the index using filteredRows[targetIndex]. It might be cleaner to just pass filteredRows[targetIndex] to useExampleCompareData, simplifying the params and internal logic of useExampleCompareData

[chance-wnb]

filteredRows should be needed to invalidate the cache in case the filter condition changes.

[tssweeney]

I am not the biggest fan of this pattern (in particular, explicitly depending on cacheVersion that is not used in the hook). But I would agree it is hard to get around this one. Approving, but do think there might be a cleaner way.