fix: prevent multiple calls to time-series on compare view select #9805
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes an issue where the page could appear slower than it should on load due to too much blocking traffic. We make three requests to build the comparison view for runs, searches and experiments: 1) request to get search/run information of selected runs/search ids 2) request to get metric names from selected runs/searches 3) request to get timeseries data given selected run ids and metric names. we were attempting to make these requests simultaneously when the each request is dependent on information in the previous request. We also used `usePrevious` hooks as dependencies of `useCallback` hooks, which meant that we would erroneously update the callback reference when the previous value update was picked up on subsequent renders. this pr fixes these issues by not rendering the comparison view tabs until the request for the records in the selection has completed, then not loading the time series data until the metric names request has started to return information. We also clean up some places where effects were called due to usePrevious and the overall data flow which should prevent issues with out of date data appearing.
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9805 +/- ##
==========================================
- Coverage 54.31% 50.01% -4.31%
==========================================
Files 1261 938 -323
Lines 155639 126309 -29330
Branches 3540 3536 -4
==========================================
- Hits 84536 63171 -21365
+ Misses 70965 63000 -7965
Partials 138 138
Flags with carried forward coverage won't be shown. Click here to find out more.
|
keita-determined
approved these changes
Aug 7, 2024
There was a problem hiding this comment.
LGTM for the frontend improvement, but i think we need API improvement as well.
cc: @jesse-amano-hpe @AmanuelAaron
Screen.Recording.2024-08-07.at.11.45.53.AM.mov
ashtonG
added
the
to-cherry-pick
github-actions bot
pushed a commit
that referenced
this pull request
Aug 7, 2024
) (cherry picked from commit f7e18fc)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
ET-677
Description
This fixes an issue where the page could appear slower than it should on load due to too much blocking traffic. We make three requests to build the comparison view for runs, searches and experiments:
request to get search/run information of selected runs/search ids
request to get metric names from selected runs/searches
request to get timeseries data given selected run ids and metric names.
we were attempting to make these requests simultaneously when the each request is dependent on information in the previous request. We also used
usePrevioushooks as dependencies ofuseCallbackhooks, which meant that we would erroneously update the callback reference when the previous value update was picked up on subsequent renders.this pr fixes these issues by not rendering the comparison view tabs until the request for the records in the selection has completed, then not loading the time series data until the metric names request has started to return information. We also clean up some places where effects were called due to usePrevious and the overall data flow which should prevent issues with out of date data appearing.
Test Plan
metric-namesand one request made totime-seriesmetric-namesand one request made totime-series. the comparison view should briefly show the loading state before showing the full data state.Checklist
docs/release-notes/See Release Note for details.