fix: prevent multiple calls to time-series on compare view select (#9805

)

(cherry picked from commit f7e18fc)

webui/react/src/components/CompareHyperparameters.test.mock.tsx

@@ -108,20 +108,6 @@ export const METRIC_DATA: RunMetricData = {
     },
   ],
   scale: 'linear',
-  selectedMetrics: [
-    {
-      group: 'training',
-      name: 'loss',
-    },
-    {
-      group: 'validation',
-      name: 'accuracy',
-    },
-    {
-      group: 'validation',
-      name: 'validation_loss',
-    },
-  ],
   setScale: (): Scale => {
     return Scale.Linear;
   },

webui/react/src/components/CompareMetrics.tsx

@@ -1,6 +1,5 @@
 import { ChartGrid, ChartsProps } from 'hew/LineChart';
 import { Loadable, Loaded, NotLoaded } from 'hew/utils/loadable';
-import _ from 'lodash';
 import React, { useCallback, useMemo, useState } from 'react';
 
 import MetricBadgeTag from 'components/MetricBadgeTag';
@@ -116,7 +115,7 @@ const CompareMetrics: React.FC<Props> = ({
   );
 
   const chartsProps: Loadable<ChartsProps> = useMemo(() => {
-    const { metricHasData, metrics, isLoaded, selectedMetrics } = metricData;
+    const { metricHasData, metrics, isLoaded } = metricData;
     const { chartProps, chartedMetrics } = selectedRuns
       ? calculateRunsChartProps(metricData, selectedRuns, xAxis, colorMap)
       : calculateExperimentChartProps(metricData, selectedExperiments, trials, xAxis, colorMap);
@@ -134,11 +133,7 @@ const CompareMetrics: React.FC<Props> = ({
     if (!isLoaded) {
       // When trial metrics hasn't loaded metric names or individual trial metrics.
       return NotLoaded;
-    } else if (!chartDataIsLoaded || !_.isEqual(selectedMetrics, metrics)) {
-      // In some cases the selectedMetrics returned may not be up to date
-      // with the metrics selected by the user. In this case we want to
-      // show a loading state until the metrics match.
-
+    } else if (!chartDataIsLoaded) {
       // returns the chartProps with a NotLoaded series which enables
       // the ChartGrid to show a spinner for the loading charts.
       return Loaded(chartProps.map((chartProps) => ({ ...chartProps, series: NotLoaded })));

webui/react/src/components/ComparisonView.test.mock.tsx

@@ -57,20 +57,6 @@ export const METRIC_DATA: RunMetricData = {
     },
   ],
   scale: 'linear',
-  selectedMetrics: [
-    {
-      group: 'training',
-      name: 'loss',
-    },
-    {
-      group: 'validation',
-      name: 'accuracy',
-    },
-    {
-      group: 'validation',
-      name: 'validation_loss',
-    },
-  ],
   setScale: (): Scale => {
     return Scale.Linear;
   },

webui/react/src/components/ComparisonView.tsx

@@ -40,6 +40,88 @@ type Props = XOR<{ experimentSelection: SelectionType }, { runSelection: Selecti
 
 const SELECTION_LIMIT = 50;
 
+interface TabsProps {
+  colorMap: MapOfIdsToColors;
+  loadableSelectedExperiments: Loadable<ExperimentWithTrial[]>;
+  loadableSelectedRuns: Loadable<FlatRun[]>;
+  projectId: number;
+}
+
+const Tabs = ({
+  colorMap,
+  loadableSelectedExperiments,
+  loadableSelectedRuns,
+  projectId,
+}: TabsProps) => {
+  const selectedExperiments: ExperimentWithTrial[] | undefined = Loadable.getOrElse(
+    undefined,
+    loadableSelectedExperiments,
+  );
+
+  const selectedRuns: FlatRun[] | undefined = Loadable.getOrElse(undefined, loadableSelectedRuns);
+
+  const trials = useMemo(() => {
+    return selectedExperiments?.flatMap((exp) => (exp.bestTrial ? [exp.bestTrial] : [])) ?? [];
+  }, [selectedExperiments]);
+
+  const experiments = useMemo(
+    () => selectedExperiments?.map((exp) => exp.experiment) ?? [],
+    [selectedExperiments],
+  );
+
+  const metricData = useMetrics(selectedRuns ?? trials ?? []);
+
+  const tabs: PivotProps['items'] = useMemo(() => {
+    return [
+      {
+        children: selectedRuns ? (
+          <CompareMetrics colorMap={colorMap} metricData={metricData} selectedRuns={selectedRuns} />
+        ) : (
+          <CompareMetrics
+            colorMap={colorMap}
+            metricData={metricData}
+            selectedExperiments={selectedExperiments ?? []}
+            trials={trials}
+          />
+        ),
+        key: 'metrics',
+        label: 'Metrics',
+      },
+      {
+        children: selectedRuns ? (
+          <CompareHyperparameters
+            colorMap={colorMap}
+            metricData={metricData}
+            projectId={projectId}
+            selectedRuns={selectedRuns}
+          />
+        ) : (
+          <CompareHyperparameters
+            colorMap={colorMap}
+            metricData={metricData}
+            projectId={projectId}
+            selectedExperiments={selectedExperiments ?? []}
+            trials={trials}
+          />
+        ),
+        key: 'hyperparameters',
+        label: 'Hyperparameters',
+      },
+      {
+        children: selectedRuns ? (
+          <TrialsComparisonTable runs={selectedRuns} />
+        ) : (
+          <TrialsComparisonTable experiment={experiments} trials={trials} />
+        ),
+        key: 'details',
+        label: 'Details',
+      },
+    ];
+  }, [selectedRuns, metricData, selectedExperiments, trials, colorMap, projectId, experiments]);
+
+  return <Pivot items={tabs} />;
+};
+
 const ComparisonView: React.FC<Props> = ({
   children,
   colorMap,
@@ -82,11 +164,6 @@ const ComparisonView: React.FC<Props> = ({
     }
   }, [experimentSelection, open]);
 
-  const selectedExperiments: ExperimentWithTrial[] | undefined = Loadable.getOrElse(
-    undefined,
-    loadableSelectedExperiments,
-  );
-
   const loadableSelectedRuns = useAsync(async () => {
     if (
       !open ||
@@ -112,71 +189,10 @@ const ComparisonView: React.FC<Props> = ({
     }
   }, [open, runSelection]);
 
-  const selectedRuns: FlatRun[] | undefined = Loadable.getOrElse(undefined, loadableSelectedRuns);
-
   const minWidths: [number, number] = useMemo(() => {
     return [fixedColumnsCount * MIN_COLUMN_WIDTH + scrollbarWidth, 100];
   }, [fixedColumnsCount, scrollbarWidth]);
 
-  const trials = useMemo(() => {
-    return selectedExperiments?.flatMap((exp) => (exp.bestTrial ? [exp.bestTrial] : [])) ?? [];
-  }, [selectedExperiments]);
-
-  const experiments = useMemo(
-    () => selectedExperiments?.map((exp) => exp.experiment) ?? [],
-    [selectedExperiments],
-  );
-
-  const metricData = useMetrics(selectedRuns ?? trials ?? []);
-
-  const tabs: PivotProps['items'] = useMemo(() => {
-    return [
-      {
-        children: selectedRuns ? (
-          <CompareMetrics colorMap={colorMap} metricData={metricData} selectedRuns={selectedRuns} />
-        ) : (
-          <CompareMetrics
-            colorMap={colorMap}
-            metricData={metricData}
-            selectedExperiments={selectedExperiments ?? []}
-            trials={trials}
-          />
-        ),
-        key: 'metrics',
-        label: 'Metrics',
-      },
-      {
-        children: selectedRuns ? (
-          <CompareHyperparameters
-            colorMap={colorMap}
-            metricData={metricData}
-            projectId={projectId}
-            selectedRuns={selectedRuns}
-          />
-        ) : (
-          <CompareHyperparameters
-            colorMap={colorMap}
-            metricData={metricData}
-            projectId={projectId}
-            selectedExperiments={selectedExperiments ?? []}
-            trials={trials}
-          />
-        ),
-        key: 'hyperparameters',
-        label: 'Hyperparameters',
-      },
-      {
-        children: selectedRuns ? (
-          <TrialsComparisonTable runs={selectedRuns} />
-        ) : (
-          <TrialsComparisonTable experiment={experiments} trials={trials} />
-        ),
-        key: 'details',
-        label: 'Details',
-      },
-    ];
-  }, [selectedRuns, metricData, selectedExperiments, trials, colorMap, projectId, experiments]);
-
   const leftPane =
     open && !hasPinnedColumns ? (
       <Message icon="info" title='Pin columns to see them in "Compare View"' />
@@ -191,7 +207,7 @@ const ComparisonView: React.FC<Props> = ({
           <Alert description="Select records you would like to compare." message={EMPTY_MESSAGE} />
         );
       }
-      if (selectedExperiments === undefined) {
+      if (loadableSelectedExperiments.isNotLoaded) {
         return <Spinner spinning />;
       }
     }
@@ -201,7 +217,7 @@ const ComparisonView: React.FC<Props> = ({
           <Alert description="Select records you would like to compare." message={EMPTY_MESSAGE} />
         );
       }
-      if (selectedRuns === undefined) {
+      if (loadableSelectedRuns.isNotLoaded) {
         return <Spinner spinning />;
       }
     }
@@ -210,7 +226,12 @@ const ComparisonView: React.FC<Props> = ({
         {isSelectionLimitReached && (
           <Alert message={`Only up to ${SELECTION_LIMIT} records can be compared`} />
         )}
-        <Pivot items={tabs} />
+        <Tabs
+          colorMap={colorMap}
+          loadableSelectedExperiments={loadableSelectedExperiments}
+          loadableSelectedRuns={loadableSelectedRuns}
+          projectId={projectId}
+        />
       </>
     );
   };

webui/react/src/hooks/useMetricNames.ts

@@ -1,38 +1,40 @@
 import { Loadable, Loaded, NotLoaded } from 'hew/utils/loadable';
 import _ from 'lodash';
-import { useEffect, useState } from 'react';
+import { useEffect, useMemo, useRef, useState } from 'react';
 
 import { V1ExpMetricNamesResponse } from 'services/api-ts-sdk';
 import { detApi } from 'services/apiConfig';
 import { readStream } from 'services/utils';
 import { Metric, XAxisDomain } from 'types';
 import { metricKeyToMetric, metricSorter, metricToKey } from 'utils/metric';
 
-import usePrevious from './usePrevious';
-
-// DO NOT pass a raw object for experimentIds param
-// That causes unwanted API call
 const useMetricNames = (
   experimentIds: number[],
   errorHandler?: (e: unknown) => void,
   quickPoll?: boolean,
 ): Loadable<Metric[]> => {
   const [metrics, setMetrics] = useState<Loadable<Metric[]>>(NotLoaded);
-  const previousExpIds = usePrevious(experimentIds, []);
+  // Do not replace with usePrevious here -- it restarts the stream erroneously;
+  const idsRef = useRef(experimentIds);
+  const curExperimentIds = useMemo(() => {
+    return _.isEqual(experimentIds, idsRef.current) ? idsRef.current : experimentIds;
+  }, [experimentIds]);
 
   useEffect(() => {
-    if (experimentIds.length === 0) {
+    const previousExpIds = idsRef.current;
+    if (curExperimentIds.length === 0) {
       setMetrics(Loaded([]));
       return;
     }
-    if (!_.isEqual(experimentIds, previousExpIds)) setMetrics(NotLoaded);
+    if (curExperimentIds !== previousExpIds) setMetrics(NotLoaded);
+
     const canceler = new AbortController();
 
     // We do not want to plot any x-axis metric values as y-axis data
     const xAxisMetrics = Object.values(XAxisDomain).map((v) => v.toLowerCase());
 
     readStream<V1ExpMetricNamesResponse>(
-      detApi.StreamingInternal.expMetricNames(experimentIds, quickPoll ? 5 : undefined, {
+      detApi.StreamingInternal.expMetricNames(curExperimentIds, quickPoll ? 5 : undefined, {
         signal: canceler.signal,
       }),
       (event: V1ExpMetricNamesResponse) => {
@@ -79,7 +81,10 @@ const useMetricNames = (
       errorHandler,
     );
     return () => canceler.abort();
-  }, [experimentIds, previousExpIds, errorHandler, quickPoll]);
+  }, [curExperimentIds, errorHandler, quickPoll]);
+  useEffect(() => {
+    idsRef.current = experimentIds;
+  });
 
   return metrics;
 };