From cac5266709944a166f416d940f223ffb00509ca8 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 19 Apr 2023 19:38:49 +1000 Subject: [PATCH] Trigger plot updates whenever commit data changes --- extension/src/plots/index.ts | 2 + extension/src/plots/model/collect.ts | 10 ++++ extension/src/plots/model/index.ts | 24 +++++----- extension/src/test/suite/plots/index.test.ts | 23 +++++++-- extension/src/util/object.test.ts | 49 +------------------- extension/src/util/object.ts | 15 ------ 6 files changed, 45 insertions(+), 78 deletions(-) diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 9ffca345c5..b7a97ce22f 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -225,6 +225,8 @@ export class Plots extends BaseRepository { this.dispose.untrack(waitForInitialExpData) waitForInitialExpData.dispose() this.data.setMetricFiles(experiments.getRelativeMetricsFiles()) + const collectInitialIdShas = () => this.plots.removeStaleData() + collectInitialIdShas() this.setupExperimentsListener(experiments) void this.initializeData() }) diff --git a/extension/src/plots/model/collect.ts b/extension/src/plots/model/collect.ts index 2051aea469..164deb42fa 100644 --- a/extension/src/plots/model/collect.ts +++ b/extension/src/plots/model/collect.ts @@ -468,3 +468,13 @@ export const collectImageUrl = ( return url } + +export const collectIdShas = (experiments: Experiment[]) => { + const idShas: Record = {} + for (const { id, sha } of experiments) { + if (sha) { + idShas[id] = sha + } + } + return idShas +} diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 2901aedb6e..939664c4cb 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -9,7 +9,8 @@ import { collectCustomPlots, getCustomPlotId, collectOrderedRevisions, - collectImageUrl + collectImageUrl, + collectIdShas } from './collect' import { getRevisionFirstThreeColumns } from './util' import { @@ -43,7 +44,6 @@ import { reorderObjectList, sameContents } from '../../util/array' -import { removeMissingKeysFromObject } from '../../util/object' import { TemplateOrder } from '../paths/collect' import { PersistenceKey } from '../../persistence/constants' import { ModelWithPersistence } from '../../persistence/model' @@ -66,6 +66,7 @@ export class PlotsModel extends ModelWithPersistence { private sectionCollapsed: SectionCollapsed private fetchedRevs = new Set() + private idShas: { [id: string]: string } = {} private comparisonData: ComparisonData = {} private comparisonOrder: string[] @@ -118,17 +119,18 @@ export class PlotsModel extends ModelWithPersistence { } public removeStaleData() { - const revisions = this.experiments.getRevisionIds() - - this.comparisonData = removeMissingKeysFromObject( - revisions, - this.comparisonData + const idShas = collectIdShas( + this.experiments.getWorkspaceCommitsAndExperiments() ) - this.revisionData = removeMissingKeysFromObject( - revisions, - this.revisionData - ) + for (const id of Object.keys(this.idShas)) { + if (this.idShas[id] !== idShas[id]) { + this.deleteRevisionData(id) + this.fetchedRevs.delete(id) + } + } + + this.idShas = idShas } public getCustomPlots(): CustomPlotsData | undefined { diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 63f5a1f377..5a5642aea7 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -43,6 +43,7 @@ import { isErrorItem } from '../../../tree' import { RegisteredCommands } from '../../../commands/external' import { REVISIONS } from '../../fixtures/plotsDiff' import * as FileSystem from '../../../fileSystem' +import { experimentHasError } from '../../../cli/dvc/contract' suite('Plots Test Suite', () => { const disposable = Disposable.fn() @@ -94,17 +95,31 @@ suite('Plots Test Suite', () => { it('should call plots diff with the commit whenever the current commit changes', async () => { const mockNow = getMockNow() + const noExperimentFixture = expShowFixtureWithoutErrors.map(exp => ({ + ...exp, + experiments: null + })) + const { data, experiments, mockPlotsDiff } = await buildPlots( disposable, - plotsDiffFixture + plotsDiffFixture, + noExperimentFixture ) + mockPlotsDiff.resetHistory() - const updatedExpShowFixture = expShowFixtureWithoutErrors.map(exp => { + const updatedExpShowFixture = noExperimentFixture.map(exp => { if (exp.rev === '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77') { + if (experimentHasError(exp)) { + throw new Error('Experiment should not have error') + } + return { ...exp, - experiments: null, + data: { + ...exp.data, + rev: '9235a02880a0372545e5f7f4d79a5d9eee6331ac' + }, rev: '9235a02880a0372545e5f7f4d79a5d9eee6331ac' } } @@ -127,7 +142,7 @@ suite('Plots Test Suite', () => { REVISIONS[0], REVISIONS[1] ) - }) + }).timeout(WEBVIEW_TEST_TIMEOUT) it('should remove the temporary plots directory on dispose', async () => { const { mockRemoveDir, plots } = await buildPlots( diff --git a/extension/src/util/object.test.ts b/extension/src/util/object.test.ts index c6f44e22e4..42ef4d5d6e 100644 --- a/extension/src/util/object.test.ts +++ b/extension/src/util/object.test.ts @@ -1,51 +1,4 @@ -import { createTypedAccumulator, removeMissingKeysFromObject } from './object' - -describe('removeMissingKeysFromObject', () => { - it('should remove any keys that exist in the object but not the provided array', () => { - const expectedKeys = ['A', 'B', 'C', 'D'] - const extendedObject = { - A: 1, - B: 2, - C: 3, - D: 4, - E: 5, - F: 6, - G: 7 - } - - expect( - removeMissingKeysFromObject(expectedKeys, extendedObject) - ).toStrictEqual({ - A: 1, - B: 2, - C: 3, - D: 4 - }) - }) - - it('should not mutate the original object', () => { - const expectedKeys: string[] = [] - const extendedObject = { - A: 1, - B: 2, - C: 3, - D: 4, - E: 5, - F: 6, - G: 7 - } - const copyExtendedObject = { ...extendedObject } - - const emptyObject = removeMissingKeysFromObject( - expectedKeys, - extendedObject - ) - - expect(emptyObject).toStrictEqual({}) - expect(extendedObject).not.toStrictEqual({}) - expect(extendedObject).toStrictEqual(copyExtendedObject) - }) -}) +import { createTypedAccumulator } from './object' describe('createTypedAccumulator', () => { it('should create a typed accumulator from an enum like object', () => { diff --git a/extension/src/util/object.ts b/extension/src/util/object.ts index 8bdd18127e..098abf7d7e 100644 --- a/extension/src/util/object.ts +++ b/extension/src/util/object.ts @@ -2,21 +2,6 @@ export const hasKey = (maybeObject: unknown, key: string): boolean => typeof maybeObject === 'object' && Object.prototype.hasOwnProperty.call(maybeObject, key) -export const removeMissingKeysFromObject = < - T extends { [key: string]: unknown } ->( - retainKeys: string[], - items: T -): T => { - const copy = { ...items } - for (const key of Object.keys(copy)) { - if (!retainKeys.includes(key)) { - delete copy[key] - } - } - return copy -} - export const createTypedAccumulator = ( enumLike: Record ) => {