Refresh cached plots data on every update #3532

mattseddon · 2023-03-22T09:07:03Z

4/4 `main` <- #3477 <- #3520 <- #3522 <- this

Tl;dr of the problem is that our cached plots data is actually mutable depending on the combination of revisions (and even order of those revisions) that we call the plots diff with.

This PR updates the way that we treat cached plots data in the extension. Now every time there is an update to the selected revisions we call for an update from the CLI. Please watch the demo plus read below to see the approach that I've taken.

LMK what you think.

Demo

Screen.Recording.2023-03-23.at.9.37.46.pm.mov

The way that it works.

We get a call to update the data from one of the paths available (e.g change in selected revisions or change in data in the workspace).
We send all the data that we currently have cached to the UI.
At the same time we call the CLI for an update for all of the currently selected revisions.
We send the updated data to the UI.

This approach gives us a few of things:

It gives the user something to look whilst the CLI is working.
It gives them the impression that something is happening.
In most cases the initial data send to the UI will be correct anyway.
The resultant data should always be correct.

extension/src/plots/errors/decorationProvider.ts

mattseddon · 2023-03-24T00:27:21Z

extension/src/experiments/index.ts

@@ -450,12 +450,6 @@ export class Experiments extends BaseRepository<TableData> {
    return experiment?.name || experiment?.label
  }

-  public getMutableRevisions() {


[F] No longer required as we now refresh all selected revisions all the time.

mattseddon · 2023-03-24T00:30:12Z

extension/src/plots/data/index.ts

-      this.model.getMissingRevisions(),
-      this.model.getMutableRevisions()
-    )
+    this.notifyTriggered()


[F] This is how we send the cached data before following up with a CLI update.

mattseddon · 2023-03-24T00:35:11Z

extension/src/plots/index.ts

+  private onDidTriggerDataUpdate() {
+    const sendCachedDataToWebview = () => {
+      this.plots.resetFetched()
+      this.plots.setComparisonOrder()


[F] Resetting the fetchedRevs means that we get spinners in the ribbon for all of the revisions. Setting the comparison order means we get a loading state for the comparison table:

Screen.Recording.2023-03-24.at.11.33.54.am.mov

mattseddon · 2023-03-24T00:37:04Z

extension/src/plots/model/index.ts

@@ -355,6 +336,12 @@ export class PlotsModel extends ModelWithPersistence {
    return this.experiments.getSelectedRevisions().map(({ label }) => label)
  }

+  public getSelectedOrderedCliIds() {
+    return collectOrderedRevisions(this.experiments.getSelectedRevisions()).map(


[F] This logic should give stable semantics no matter what order the user selects the experiments. We will take the most recent version of the template and try to apply that to the rest of the data.

mattseddon · 2023-03-24T00:42:26Z

extension/src/plots/model/index.ts

@@ -355,6 +336,12 @@ export class PlotsModel extends ModelWithPersistence {
    return this.experiments.getSelectedRevisions().map(({ label }) => label)
  }

+  public getSelectedOrderedCliIds() {
+    return collectOrderedRevisions(this.experiments.getSelectedRevisions()).map(


[F] This logic should give stable semantics no matter what order the user selects the experiments. We will take the most recent version of the template and try to apply that to the rest of the data.

julieg18

Great work!

codeclimate · 2023-04-02T02:23:40Z

webview/src/plots/components/customPlots/CustomPlots.tsx

@@ -23,7 +23,7 @@ interface CustomPlotsProps {

 export const CustomPlots: React.FC<CustomPlotsProps> = ({ plotsIds }) => {
  const [order, setOrder] = useState(plotsIds)
-  const { nbItemsPerRow, hasData, disabledDragPlotIds } = useSelector(
+  const { nbItemsPerRow, hasData, hasItems, disabledDragPlotIds } = useSelector(


Similar blocks of code found in 4 locations. Consider refactoring.

codeclimate · 2023-04-02T02:23:41Z

webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx

@@ -7,12 +7,9 @@ import { PlotsContainer } from '../PlotsContainer'
 import { PlotsState } from '../../store'

 export const TemplatePlotsWrapper: React.FC = () => {
-  const { nbItemsPerRow, isCollapsed, height } = useSelector(
+  const { nbItemsPerRow, isCollapsed, height, hasItems } = useSelector(


Similar blocks of code found in 4 locations. Consider refactoring.

codeclimate · 2023-04-02T02:23:43Z

Code Climate has analyzed commit 02e94b4 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.1%.

View more on Code Climate.

mattseddon added the bug Something isn't working label Mar 22, 2023

mattseddon self-assigned this Mar 22, 2023

codeclimate bot reviewed Mar 22, 2023

View reviewed changes

extension/src/plots/errors/decorationProvider.ts Outdated Show resolved Hide resolved

mattseddon force-pushed the drop-cache-on-update branch from 26cb1df to 7fe2670 Compare March 22, 2023 09:24

This was referenced Mar 22, 2023

Bump min DVC version to 2.52.0 (plots errors) #3477

Merged

Use plot errors to display correct messages for missing plots #3520

Merged

Show plots errors in plots tree #3522

Merged

mattseddon changed the base branch from main to show-tree-errors March 22, 2023 09:30

mattseddon changed the base branch from show-tree-errors to main March 23, 2023 10:15

mattseddon force-pushed the drop-cache-on-update branch from 7fe2670 to e92b897 Compare March 23, 2023 10:36

mattseddon changed the title ~~Drop cached plots data whenever dvc.yaml is changed~~ Refresh cached plots data on every update Mar 23, 2023

mattseddon changed the base branch from main to show-tree-errors March 23, 2023 10:37

mattseddon commented Mar 24, 2023

View reviewed changes

mattseddon marked this pull request as ready for review March 24, 2023 00:49

mattseddon requested review from sroy3 and julieg18 as code owners March 24, 2023 00:49

This was referenced Mar 24, 2023

Ensure unfetched image plots are on disk before providing URL to webview #3544

Merged

Fix removal of existing commit errors #3545

Merged

Group shared ErrorTooltip with Tooltip #3546

Merged

mattseddon force-pushed the show-tree-errors branch from ae197a0 to a7f586f Compare March 24, 2023 03:20

mattseddon force-pushed the drop-cache-on-update branch from 02b01cc to e1519ad Compare March 24, 2023 03:20

mattseddon force-pushed the show-tree-errors branch from a7f586f to e560f67 Compare March 24, 2023 03:32

mattseddon force-pushed the drop-cache-on-update branch from e1519ad to b0dd757 Compare March 24, 2023 03:34

This was referenced Mar 24, 2023

Account for data key being optional in plots diff output #3547

Merged

Consolidate tree view error decoration providers #3548

Merged

sroy3 approved these changes Mar 24, 2023

View reviewed changes

julieg18 approved these changes Mar 24, 2023

View reviewed changes

This was referenced Mar 26, 2023

Remove code that refreshes a single plot revision (no longer possible) #3555

Merged

Ensure that data for broken revisions is dropped #3576

Merged

mattseddon force-pushed the show-tree-errors branch from e560f67 to 82fa5b4 Compare April 2, 2023 01:50

Base automatically changed from show-tree-errors to main April 2, 2023 02:08

mattseddon force-pushed the drop-cache-on-update branch from b0dd757 to 72be7b6 Compare April 2, 2023 02:14

mattseddon added 3 commits April 2, 2023 12:15

Refresh all cached plots data on every update

9d46fd1

Add tests

2f337d3

Self review

02e94b4

mattseddon force-pushed the drop-cache-on-update branch from 72be7b6 to 02e94b4 Compare April 2, 2023 02:15

mattseddon enabled auto-merge (squash) April 2, 2023 02:20

codeclimate bot reviewed Apr 2, 2023

View reviewed changes

mattseddon merged commit 6bdd98c into main Apr 2, 2023

mattseddon deleted the drop-cache-on-update branch April 2, 2023 02:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh cached plots data on every update #3532

Refresh cached plots data on every update #3532

mattseddon commented Mar 22, 2023 •

edited

Loading

mattseddon Mar 24, 2023 •

edited

Loading

mattseddon Mar 24, 2023

mattseddon Mar 24, 2023

mattseddon Mar 24, 2023

mattseddon Mar 24, 2023

julieg18 left a comment

codeclimate bot Apr 2, 2023

codeclimate bot Apr 2, 2023

codeclimate bot commented Apr 2, 2023

Refresh cached plots data on every update #3532

Refresh cached plots data on every update #3532

Conversation

mattseddon commented Mar 22, 2023 • edited Loading

4/4 main <- #3477 <- #3520 <- #3522 <- this

Demo

mattseddon Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

mattseddon Mar 24, 2023

Choose a reason for hiding this comment

mattseddon Mar 24, 2023

Choose a reason for hiding this comment

mattseddon Mar 24, 2023

Choose a reason for hiding this comment

mattseddon Mar 24, 2023

Choose a reason for hiding this comment

julieg18 left a comment

Choose a reason for hiding this comment

codeclimate bot Apr 2, 2023

Choose a reason for hiding this comment

codeclimate bot Apr 2, 2023

Choose a reason for hiding this comment

codeclimate bot commented Apr 2, 2023

mattseddon commented Mar 22, 2023 •

edited

Loading

4/4 `main` <- #3477 <- #3520 <- #3522 <- this

mattseddon Mar 24, 2023 •

edited

Loading