fix: prevent extra initial calls to search endpoints #9782

ashtonG · 2024-08-01T20:05:39Z

Ticket

Description

this fixes an issue where the new table experience tables would call their search endpoints multiple times on mount, possibly causing server load issues. This happened due to two root causes:

the comparison view would call the search endpoint for user selections even when the comparison view was closed and the user had selected no experiments or runs. we now bail on the request if the comparison view is closed or the selection is empty. This may impact the user because now the comparison view will update on open instead of in the background, but I think this is a fair tradeoff.
the sort state updated as an effect of loading the settings, so the polling function would initially fire when the settings loaded, then again when the sort state updated. We now ~~derive the sort state directly from the sort string instead~~ directly update the sort state when the settings load.

Test Plan

with developer tools open to the network tab, visit a project's run list
in the request list, every call to the runs endpoint should be 5 seconds apart
when opening the comparison view, another call should be made
Repeat the above for the searches view as well as the New Experiment List page

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

this fixes an issue where the new table experience tables would call their search endpoints multiple times on mount, possibly causing server load issues. This happened due to two root causes: 1) the comparison view would call the search endpoint for user selections even when the comparison view was closed and the user had selected no experiments or runs. we now bail on the request if the comparison view is closed or the selection is empty. This may impact the user because now the comparison view will update on open instead of in the background, but I think this is a fair tradeoff. 2) the sort state updated as an effect of loading the settings, so the polling function would initially fire when the settings loaded, then again when the sort state updated. We now derive the sort state directly from the sort string instead.

netlify · 2024-08-01T20:05:57Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`da49c1f`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66b29148c94b9d00073afb6f
😎 Deploy Preview	https://deploy-preview-9782--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-08-01T20:42:11Z

Codecov Report

Attention: Patch coverage is 61.45833% with 37 lines in your changes missing coverage. Please review.

Project coverage is 49.73%. Comparing base (15226b7) to head (da49c1f).
Report is 21 commits behind head on main.

Files	Patch %	Lines
webui/react/src/pages/FlatRuns/FlatRuns.tsx	5.55%	17 Missing ⚠️
...bui/react/src/pages/F_ExpList/F_ExperimentList.tsx	0.00%	16 Missing ⚠️
webui/react/src/components/ComparisonView.tsx	90.90%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9782      +/-   ##
==========================================
- Coverage   54.05%   49.73%   -4.32%     
==========================================
  Files        1260      937     -323     
  Lines      155545   126227   -29318     
  Branches     3503     3508       +5     
==========================================
- Hits        84074    62783   -21291     
+ Misses      71325    63298    -8027     
  Partials      146      146

Flag	Coverage Δ
harness	`?`
web	`53.23% <61.45%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
webui/react/src/components/Searches/Searches.tsx	`56.90% <100.00%> (+0.34%)`	⬆️
webui/react/src/hooks/useDebouncedSettings.ts	`100.00% <100.00%> (ø)`
webui/react/src/components/ComparisonView.tsx	`91.73% <90.90%> (+1.03%)`	⬆️
...bui/react/src/pages/F_ExpList/F_ExperimentList.tsx	`0.00% <0.00%> (ø)`
webui/react/src/pages/FlatRuns/FlatRuns.tsx	`11.06% <5.55%> (-0.04%)`	⬇️

... and 323 files with indirect coverage changes

keita-determined · 2024-08-01T21:00:24Z

@EmilyBonar could you also review this since i think you are more familiar with the code

keita-determined · 2024-08-01T21:57:19Z

in the request list, every call to the runs endpoint should be 5 seconds apart

i tested it against gcloud server, and see multiple run apis. that's not what we expect, right?

Screen.Recording.2024-08-01.at.2.55.43.PM.mov

ashtonG · 2024-08-02T14:31:25Z

with the comparison view open i'd expect to see two -- one for the main list and one for the selection for comparison view.

i also noticed an issue where users aren't able to add new sorts. looking into these now.

…condition

ashtonG · 2024-08-02T18:48:12Z

webui/react/src/hooks/useDebouncedSettings.ts

@@ -31,7 +31,7 @@ export function useDebouncedSettings<T extends t.HasProps | t.ExactC<t.HasProps>
  const settingsObs = useMemo(() => userSettings.get(type, path), [type, path]);
  const [localState, updateLocalState] = useState<Loadable<T | null>>(NotLoaded);

-  useEffect(() => {
+  useLayoutEffect(() => {


saw issues where we were still seeing two requests to the search endpoint because settings was loaded but column widths were not. useObservable subscribes to the observable in useLayoutEffect instead of useEffect, so we do that here to ensure all the subscriptions fire at the same time.

EmilyBonar

Looks good, can confirm that I'm not seeing the repeated initial calls in the network tab!

EmilyBonar · 2024-08-05T15:18:38Z

webui/react/src/components/ComparisonView.tsx

+      if (response?.pagination?.total && response?.pagination?.total > SELECTION_LIMIT) {
+        setIsSelectionLimitReached(true);
+      } else {
+        setIsSelectionLimitReached(false);


nit: could be reduced to setIsSelectionLimitReached(response?.pagination?.total && response?.pagination?.total > SELECTION_LIMIT);

EmilyBonar · 2024-08-05T15:19:34Z

webui/react/src/components/ComparisonView.tsx

+      if (response?.pagination?.total && response?.pagination?.total > SELECTION_LIMIT) {
+        setIsSelectionLimitReached(true);
+      } else {
+        setIsSelectionLimitReached(false);


nit: same reduction

EmilyBonar · 2024-08-05T15:22:15Z

webui/react/src/components/Searches/Searches.tsx

-    }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [isLoadingSettings]);
+  useLayoutEffect(() => {


sugg: we're doing this same pattern 3 times, could it be extracted out into a custom hook?

i'm not sure if there's value in abstracting this pattern -- while we do handoff local/server state three times in this pr (and more when you factor in filterFormState + columnWidths), the knobs necessary to twist (observable, default value, state, etc.) mean that it'll either only be used in these three places or it'll be easier to just inline.

EmilyBonar · 2024-08-05T17:39:24Z

webui/react/src/components/Searches/Searches.tsx

+    let cleanup: () => void;
+    // eslint-disable-next-line prefer-const
+    cleanup = eagerSubscribe(projectSettingsObs, (ps, prevPs) => {


q: what's the reason for declaring the variable separately from initializing it?

ran into an issue where cleanup wasn't defined in scope when the module was parsed. when we used this pattern for filterformstate, cleanup was defined and set at runtime, so i think it's just an odd issue. I'm going to take another look and see if it still makes sense.

(cherry picked from commit 9702d22)

ashtonG requested a review from a team as a code owner August 1, 2024 20:05

ashtonG requested a review from keita-determined August 1, 2024 20:05

cla-bot bot added the cla-signed label Aug 1, 2024

include defaults

3ab7173

keita-determined requested a review from EmilyBonar August 1, 2024 21:00

hand off settings.sortstring to sorts properly/fix columnWidths race …

f8f8d99

…condition

ashtonG commented Aug 2, 2024

View reviewed changes

ashtonG self-assigned this Aug 5, 2024

ashtonG added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Aug 5, 2024

EmilyBonar approved these changes Aug 5, 2024

View reviewed changes

direct set boolean

da49c1f

ashtonG merged commit 9702d22 into main Aug 7, 2024
82 of 95 checks passed

ashtonG deleted the bug/multiple-search-calls branch August 7, 2024 14:47

github-actions bot pushed a commit that referenced this pull request Aug 7, 2024

fix: prevent extra initial calls to search endpoints (#9782)

da58c92

(cherry picked from commit 9702d22)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent extra initial calls to search endpoints #9782

fix: prevent extra initial calls to search endpoints #9782

ashtonG commented Aug 1, 2024 •

edited

Loading

netlify bot commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

keita-determined commented Aug 1, 2024

keita-determined commented Aug 1, 2024

ashtonG commented Aug 2, 2024

ashtonG Aug 2, 2024

EmilyBonar left a comment

EmilyBonar Aug 5, 2024

EmilyBonar Aug 5, 2024

EmilyBonar Aug 5, 2024

ashtonG Aug 5, 2024

EmilyBonar Aug 5, 2024

ashtonG Aug 6, 2024

fix: prevent extra initial calls to search endpoints #9782

fix: prevent extra initial calls to search endpoints #9782

Conversation

ashtonG commented Aug 1, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Aug 1, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

keita-determined commented Aug 1, 2024

keita-determined commented Aug 1, 2024

ashtonG commented Aug 2, 2024

ashtonG Aug 2, 2024

Choose a reason for hiding this comment

EmilyBonar left a comment

Choose a reason for hiding this comment

EmilyBonar Aug 5, 2024

Choose a reason for hiding this comment

EmilyBonar Aug 5, 2024

Choose a reason for hiding this comment

EmilyBonar Aug 5, 2024

Choose a reason for hiding this comment

ashtonG Aug 5, 2024

Choose a reason for hiding this comment

EmilyBonar Aug 5, 2024

Choose a reason for hiding this comment

ashtonG Aug 6, 2024

Choose a reason for hiding this comment

ashtonG commented Aug 1, 2024 •

edited

Loading

netlify bot commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading