Apply experiment table filters to commits (#4363)

* implement working prototype for new filter logic

* refactor logic

* fix with no experiments story

Author: mattseddon

extension/src/experiments/index.ts

@@ -341,6 +341,10 @@ export class Experiments extends BaseRepository<TableData> {
     return this.experiments.getFilteredCount()
   }
 
+  public getRecordCount() {
+    return this.experiments.getRecordCount()
+  }
+
   public getExperimentCount() {
     if (!this.columns.hasNonDefaultColumns()) {
       return 0

extension/src/experiments/model/filterBy/collect.ts

@@ -0,0 +1,46 @@
+import { FilterDefinition, filterExperiment } from '.'
+import { definedAndNonEmpty } from '../../../util/array'
+import { Commit, Experiment } from '../../webview/contract'
+
+export const collectFiltered = (
+  acc: Experiment[],
+  commit: Commit,
+  experiments: Experiment[] | undefined,
+  filters: FilterDefinition[]
+) => {
+  let hasUnfilteredExperiment = false
+  for (const experiment of experiments || []) {
+    if (!filterExperiment(filters, experiment)) {
+      acc.push(experiment)
+      continue
+    }
+    hasUnfilteredExperiment = true
+  }
+  if (!hasUnfilteredExperiment && !filterExperiment(filters, commit)) {
+    acc.push(commit)
+  }
+}
+
+export const collectUnfiltered = (
+  commit: Commit,
+  experiments: Experiment[] | undefined,
+  filters: FilterDefinition[]
+): Commit | undefined => {
+  const unfilteredExperiments = experiments?.filter(
+    (experiment: Experiment) => !!filterExperiment(filters, experiment)
+  )
+
+  const hasUnfilteredExperiments = definedAndNonEmpty(unfilteredExperiments)
+  const filtered =
+    !hasUnfilteredExperiments && !filterExperiment(filters, commit)
+
+  if (filtered) {
+    return
+  }
+
+  if (hasUnfilteredExperiments) {
+    commit.subRows = unfilteredExperiments
+  }
+
+  return commit
+}

extension/src/experiments/model/filterBy/tree.ts

@@ -12,11 +12,7 @@ import { RegisteredCommands } from '../../../commands/external'
 import { InternalCommands } from '../../../commands/internal'
 import { sendViewOpenedTelemetryEvent } from '../../../telemetry'
 import { EventName } from '../../../telemetry/constants'
-import {
-  definedAndNonEmpty,
-  joinTruthyItems,
-  sortCollectedArray
-} from '../../../util/array'
+import { definedAndNonEmpty, sortCollectedArray } from '../../../util/array'
 import { createTreeView, getRootItem, isRoot } from '../../../tree'
 import { Disposable } from '../../../class/dispose'
 import { sum } from '../../../util/math'
@@ -171,11 +167,12 @@ export class ExperimentsFilterByTree
         this.experiments.getRepository(dvcRoot).getFilteredCount()
       )
     )
+    const total = sum(
+      dvcRoots.flatMap(dvcRoot =>
+        this.experiments.getRepository(dvcRoot).getRecordCount()
+      )
+    )
 
-    return joinTruthyItems([
-      `${filteredCount || 'No'}`,
-      'Experiment' + (filteredCount === 1 ? '' : 's'),
-      'Filtered'
-    ])
+    return [filteredCount, 'of', total, 'Filtered'].join(' ')
   }
 }

extension/src/experiments/model/index.ts

@@ -1,6 +1,7 @@
 import { Memento } from 'vscode'
 import { SortDefinition, sortExperiments } from './sortBy'
-import { FilterDefinition, filterExperiment, getFilterId } from './filterBy'
+import { FilterDefinition, getFilterId } from './filterBy'
+import { collectFiltered, collectUnfiltered } from './filterBy/collect'
 import {
   collectAddRemoveCommitsDetails,
   collectBranches,
@@ -27,7 +28,7 @@ import {
   GitRemoteStatus,
   RunningExperiment
 } from '../webview/contract'
-import { definedAndNonEmpty, reorderListSubset } from '../../util/array'
+import { reorderListSubset } from '../../util/array'
 import {
   Executor,
   ExpShowOutput,
@@ -409,31 +410,27 @@ export class ExperimentsModel extends ModelWithPersistence {
   }
 
   public getRowData() {
-    const commitsBySha: { [sha: string]: Commit } = {}
-    for (const commit of this.commits) {
-      commitsBySha[commit.sha as string] = commit
-    }
+    const commitsBySha = this.applyFiltersToCommits()
 
-    return [
-      { branch: undefined, ...this.addDetails(this.workspace) },
-      ...this.rowOrder.map(({ branch, sha }) => {
-        const commit = { ...commitsBySha[sha], branch }
-        const experiments = this.getExperimentsByCommit(commit)
-        const commitWithSelectedAndStarred = this.addDetails(commit)
-
-        if (!definedAndNonEmpty(experiments)) {
-          return commitWithSelectedAndStarred
-        }
-
-        return {
-          ...commitWithSelectedAndStarred,
-          subRows: experiments.filter(
-            (experiment: Experiment) =>
-              !!filterExperiment(this.getFilters(), experiment)
-          )
-        }
-      })
+    const rows: Commit[] = [
+      { branch: undefined, ...this.addDetails(this.workspace) }
     ]
+    for (const { branch, sha } of this.rowOrder) {
+      const commit = commitsBySha[sha]
+      if (!commit) {
+        continue
+      }
+      if (commit.subRows) {
+        commit.subRows = commit.subRows.map(experiment => ({
+          ...experiment,
+          branch
+        }))
+      }
+
+      rows.push({ ...commit, branch })
+    }
+
+    return rows
   }
 
   public getHasMoreCommits() {
@@ -461,6 +458,10 @@ export class ExperimentsModel extends ModelWithPersistence {
     return filtered.length
   }
 
+  public getRecordCount() {
+    return this.getCombinedList().length
+  }
+
   public getCombinedList() {
     return [this.workspace, ...this.commits, ...this.getExperimentsAndQueued()]
   }
@@ -530,10 +531,14 @@ export class ExperimentsModel extends ModelWithPersistence {
   private getFilteredExperiments() {
     const acc: Experiment[] = []
 
-    for (const experiment of this.getExperiments()) {
-      if (!filterExperiment(this.getFilters(), experiment)) {
-        acc.push(experiment)
-      }
+    for (const commit of this.commits) {
+      const experiments = this.getExperimentsByCommit(commit)
+      collectFiltered(
+        acc,
+        this.addDetails(commit),
+        experiments,
+        this.getFilters()
+      )
     }
 
     return acc
@@ -543,10 +548,7 @@ export class ExperimentsModel extends ModelWithPersistence {
     const experiments = this.experimentsByCommit
       .get(commit.id)
       ?.map(originalExperiment => {
-        const experiment = {
-          ...this.addDetails(originalExperiment),
-          branch: commit.branch
-        }
+        const experiment = this.addDetails(originalExperiment)
 
         this.addRemoteStatus(experiment)
 
@@ -697,4 +699,25 @@ export class ExperimentsModel extends ModelWithPersistence {
     }
     return uniqueStatus
   }
+
+  private applyFiltersToCommits() {
+    const commitsBySha: { [sha: string]: Commit } = {}
+    for (const commit of this.commits) {
+      const commitWithSelectedAndStarred = this.addDetails(commit)
+      const experiments = this.getExperimentsByCommit(
+        commitWithSelectedAndStarred
+      )
+      const unfilteredCommit = collectUnfiltered(
+        commitWithSelectedAndStarred,
+        experiments,
+        this.getFilters()
+      )
+      if (!unfilteredCommit) {
+        continue
+      }
+
+      commitsBySha[commit.sha as string] = unfilteredCommit
+    }
+    return commitsBySha
+  }
 }

extension/src/test/suite/experiments/model/filterBy/tree.test.ts

@@ -92,9 +92,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
             const accuracy = experiment.metrics?.['summary.json']?.accuracy
             return !!(accuracy === undefined || gte45(accuracy))
           })
-        },
-        fe2919b,
-        _7df876c
+        }
       ]
 
       const filteredTableData: Partial<TableData> = {
@@ -305,7 +303,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
       void experiments.addFilter()
       await tableFilterAdded
 
-      expect(mockTreeView.description).to.equal('3 Experiments Filtered')
+      expect(mockTreeView.description).to.equal('5 of 11 Filtered')
 
       stubPrivateMethod(experimentsModel, 'getFilteredExperiments')
         .onFirstCall()
@@ -319,13 +317,13 @@ suite('Experiments Filter By Tree Test Suite', () => {
       workspaceExperiments.experimentsChanged.fire()
       await allButOneExperimentFilteredEvent
 
-      expect(mockTreeView.description).to.equal('1 Experiment Filtered')
+      expect(mockTreeView.description).to.equal('1 of 11 Filtered')
 
       const threeExperimentsFilteredEvent = getUpdateEvent()
       workspaceExperiments.experimentsChanged.fire()
       await threeExperimentsFilteredEvent
 
-      expect(mockTreeView.description).to.equal('3 Experiments Filtered')
+      expect(mockTreeView.description).to.equal('3 of 11 Filtered')
 
       const tableFilterRemoved = getUpdateEvent()
       experiments.removeFilter(getFilterId(filter))
@@ -336,24 +334,22 @@ suite('Experiments Filter By Tree Test Suite', () => {
     })
 
     it('should be able to filter to starred experiments', async () => {
-      const { experiments, messageSpy } =
+      const { experiments, experimentsModel, messageSpy } =
         stubWorkspaceExperimentsGetters(disposable)
       await experiments.isReady()
       await experiments.showWebview()
 
+      experimentsModel.toggleStars(['main'])
+
       await addFilterViaQuickInput(experiments, starredFilter)
 
-      const [workspace, main, fe2919b, _7df876c] = rowsFixture
+      const [workspace, main] = rowsFixture
 
-      const filteredRows = [
-        workspace,
-        {
-          ...main,
-          subRows: []
-        },
-        fe2919b,
-        _7df876c
-      ]
+      const mainNoSubRows = { ...main }
+      delete mainNoSubRows.subRows
+      mainNoSubRows.starred = true
+
+      const filteredRows = [workspace, mainNoSubRows]
 
       const filteredTableData: Partial<TableData> = {
         changes: workspaceChangesFixture,

webview/src/experiments/components/App.test.tsx

@@ -47,7 +47,7 @@ import {
   renderTableWithoutRunningExperiments,
   renderTableWithPlaceholder,
   renderTableWithSortingData,
-  renderTableWithWorkspaceRowOnly,
+  renderTableWithNoRows,
   selectedRows,
   setTableData
 } from '../../test/experimentsTable'
@@ -109,8 +109,8 @@ describe('App', () => {
     expect(noColumnsState).not.toBeInTheDocument()
   })
 
-  it('should show the no experiments empty state when only the workspace is provided', () => {
-    renderTableWithWorkspaceRowOnly()
+  it('should show the no experiments empty state when no rows are provided', () => {
+    renderTableWithNoRows()
 
     const noExperimentsState = screen.queryByText('No Experiments to Display.')
     expect(noExperimentsState).toBeInTheDocument()

webview/src/experiments/components/Experiments.tsx

@@ -182,9 +182,9 @@ export const ExperimentsTable: React.FC = () => {
   }, [toggleAllRowsExpanded])
 
   const hasOnlyDefaultColumns = columns.length <= 1
-  const hasOnlyWorkspace = data.length <= 1
-  if (hasOnlyDefaultColumns || hasOnlyWorkspace) {
-    return <GetStarted showWelcome={!hasColumns || hasOnlyWorkspace} />
+  const hasNoRows = data.length === 0
+  if (hasOnlyDefaultColumns || hasNoRows) {
+    return <GetStarted showWelcome={!hasColumns || hasNoRows} />
   }
   return (
     <RowSelectionProvider>

webview/src/stories/Table.stories.tsx

@@ -297,7 +297,7 @@ LoadingData.args = { tableData: undefined }
 
 export const WithNoExperiments = Template.bind({})
 WithNoExperiments.args = {
-  tableData: { ...tableData, rows: [rowsFixture[0]] }
+  tableData: { ...tableData, rows: [] }
 }
 
 export const WithNoColumns = Template.bind({})

webview/src/test/experimentsTable.tsx

@@ -52,8 +52,8 @@ export const renderTableWithNoColumns = () => {
   renderTable({ ...tableDataFixture, columns: [] })
 }
 
-export const renderTableWithWorkspaceRowOnly = () => {
-  renderTable({ ...tableDataFixture, rows: [tableDataFixture.rows[0]] })
+export const renderTableWithNoRows = () => {
+  renderTable({ ...tableDataFixture, rows: [] })
 }
 
 export const renderTableWithSortingData = () => {