Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a mapping from revision to branches while using only one exp show call #3980

Merged
merged 26 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
04ce0b0
Revert "Revert "Use expShow once with multiple revisions""
May 25, 2023
7cda324
Revert "Revert "Change tests""
May 25, 2023
e95a418
Revert "Revert "Fix tests""
May 25, 2023
b54b8af
Add a mapping from revision to branches
May 25, 2023
0dc0ac0
Fix most tests
May 31, 2023
bfe1819
Add flag to failing test
May 31, 2023
0f4a7d2
Merge branch 'main' into map-revisions
mattseddon May 31, 2023
e649971
fix remaining test
mattseddon May 31, 2023
99e1334
Merge branch 'main' into map-revisions
mattseddon May 31, 2023
364c6de
reduce number of calls to git
mattseddon Jun 1, 2023
d9dc99a
rework call to get commit messages
mattseddon Jun 1, 2023
1a88bde
update structure of code and test fixture
mattseddon Jun 1, 2023
73c9a29
fix integration test
mattseddon Jun 1, 2023
1d38dfa
pull out type
mattseddon Jun 1, 2023
ed6e81c
Merge branch 'main' into map-revisions
mattseddon Jun 1, 2023
2d87949
Merge branch 'main' into map-revisions
mattseddon Jun 2, 2023
f14c017
do not duplicate commits
mattseddon Jun 2, 2023
8f76d4c
rename variables
mattseddon Jun 2, 2023
2fd16d5
slightly reduce diff
mattseddon Jun 2, 2023
53e1eda
remove passing of current branch
mattseddon Jun 2, 2023
dc851f7
fix indicator
mattseddon Jun 2, 2023
5c08bfc
fix bugs
mattseddon Jun 2, 2023
06acc91
Merge branch 'main' into map-revisions
mattseddon Jun 5, 2023
fe8d5cf
make changing tests easier
mattseddon Jun 5, 2023
ba12bde
refactor when to show branch divider
mattseddon Jun 5, 2023
9002047
Merge branch 'main' into map-revisions
mattseddon Jun 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions extension/src/cli/dvc/contract.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { CommitData } from '../../experiments/webview/contract'
import { Plot } from '../../plots/webview/contract'

export const MIN_CLI_VERSION = '2.58.1'
Expand Down Expand Up @@ -98,13 +99,17 @@ export type ExpWithError = {
rev: string
name?: string
branch: string | undefined
commit?: CommitData
description?: string
} & DvcError

type ExpWithData = {
rev: string
name?: string
branch: string | undefined
data: ExpData
commit?: CommitData
description?: string
}

export type ExpState = ExpWithData | ExpWithError
Expand Down
10 changes: 7 additions & 3 deletions extension/src/cli/git/reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,19 @@ export class GitReader extends GitCli {
return !output
}

public async getCommitMessages(cwd: string, sha: string): Promise<string> {
public async getCommitMessages(
cwd: string,
rev: string,
revisions?: number
): Promise<string> {
const options = getOptions(
cwd,
Command.LOG,
sha,
rev,
Flag.PRETTY_FORMAT_COMMIT_MESSAGE,
Flag.SEPARATE_WITH_NULL,
Flag.NUMBER,
'1'
String(revisions || 1)
)
try {
return await this.executeProcess(options)
Expand Down
120 changes: 87 additions & 33 deletions extension/src/experiments/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,23 @@ import {
import { getRelativePattern } from '../../fileSystem/relativePattern'
import { createFileSystemWatcher } from '../../fileSystem/watcher'
import { AvailableCommands, InternalCommands } from '../../commands/internal'
import { EXPERIMENT_WORKSPACE_ID, ExpShowOutput } from '../../cli/dvc/contract'
import {
EXPERIMENT_WORKSPACE_ID,
ExpRange,
ExpShowOutput,
ExpState
} from '../../cli/dvc/contract'
import { BaseData } from '../../data'
import { DOT_DVC, ExperimentFlag } from '../../cli/dvc/constants'
import { gitPath } from '../../cli/git/constants'
import { getGitPath } from '../../fileSystem'
import { ExperimentsModel } from '../model'
import { formatCommitMessage, collectCommitsData } from '../model/collect'
import { CommitData } from '../webview/contract'

interface HashInfo extends CommitData {
branches: string[]
}

export class ExperimentsData extends BaseData<ExpShowOutput> {
private readonly experiments: ExperimentsModel
Expand Down Expand Up @@ -47,36 +58,32 @@ export class ExperimentsData extends BaseData<ExpShowOutput> {
)

void this.updateAvailableBranchesToSelect(allBranches)
const data: ExpShowOutput = []

const { branches, currentBranch } = await this.getBranchesToShowWithCurrent(
allBranches
)
const flags: (ExperimentFlag | string)[] = []
const hashes: Record<string, HashInfo> = {}

await Promise.all(
branches.map(async branch => {
const branchFlags = [
ExperimentFlag.REV,
branch,
ExperimentFlag.NUM_COMMIT,
this.experiments.getNbOfCommitsToShow(branch).toString()
]

const output = (await this.expShow(
branchFlags,
branch
)) as ExpShowOutput

if (branch !== currentBranch) {
const workspaceIndex = output.findIndex(
exp => exp.rev === EXPERIMENT_WORKSPACE_ID
)
output.splice(workspaceIndex, 1)
}
data.push(...output)
})
for (const branch of branches) {
await this.collectRevisionGitDetails(branch, hashes, flags)
}

const output = await this.internalCommands.executeCommand<ExpShowOutput>(
AvailableCommands.EXP_SHOW,
this.dvcRoot,
...flags
)

const data: ExpShowOutput = []
for (const out of output) {
if (out.rev === EXPERIMENT_WORKSPACE_ID) {
data.push({ ...out, branch: currentBranch })
} else {
this.addDetailsToRevision(out, data, hashes)
}
}

this.collectFiles(data)

return this.notifyChanged(data)
Expand All @@ -86,6 +93,61 @@ export class ExperimentsData extends BaseData<ExpShowOutput> {
this.collectedFiles = collectFiles(data, this.collectedFiles)
}

private async collectRevisionGitDetails(
mattseddon marked this conversation as resolved.
Show resolved Hide resolved
branch: string,
hashes: Record<string, HashInfo>,
flags: (ExperimentFlag | string)[]
) {
const nbOfCommitsToShow = this.experiments.getNbOfCommitsToShow(branch)

const commitDataOutput = await this.internalCommands.executeCommand(
AvailableCommands.GIT_GET_COMMIT_MESSAGES,
mattseddon marked this conversation as resolved.
Show resolved Hide resolved
this.dvcRoot,
branch,
String(nbOfCommitsToShow)
)

const commits = collectCommitsData(commitDataOutput)

for (const { hash, ...commitData } of commits) {
if (hashes[hash]) {
hashes[hash].branches.push(branch)
} else {
hashes[hash] = {
branches: [branch],
...commitData
}
}
}

for (let i = 0; i < nbOfCommitsToShow; i++) {
const revision = `${branch}~${i}`

flags.push(ExperimentFlag.REV, revision)
}
}

private addDetailsToRevision(
out: ExpState & {
experiments?: ExpRange[] | null | undefined
},
data: ExpShowOutput,
hashes: Record<string, HashInfo>
) {
const revision = hashes[out.rev]
if (revision) {
mattseddon marked this conversation as resolved.
Show resolved Hide resolved
const { branches, ...commit } = revision
for (const branch of branches) {
data.push({
...out,
branch,
commit,
description: formatCommitMessage(commit.message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Can I move this upstream into the collect call?

})
}
}
}

private async getBranchesToShowWithCurrent(allBranches: string[]) {
const currentBranch = await this.internalCommands.executeCommand<string>(
AvailableCommands.GIT_GET_CURRENT_BRANCH,
Expand All @@ -100,16 +162,8 @@ export class ExperimentsData extends BaseData<ExpShowOutput> {
branches.push(currentBranch)
this.experiments.setBranchesToShow(branches)
}
return { branches, currentBranch }
}

private async expShow(flags: (ExperimentFlag | string)[], branch?: string) {
const data = await this.internalCommands.executeCommand<ExpShowOutput>(
AvailableCommands.EXP_SHOW,
this.dvcRoot,
...flags
)
return data.map(exp => ({ ...exp, branch }))
return { branches, currentBranch }
}

private async updateAvailableBranchesToSelect(branches?: string[]) {
Expand Down
25 changes: 2 additions & 23 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { DecorationProvider } from './model/decorationProvider'
import { starredFilter } from './model/filterBy/constants'
import { ResourceLocator } from '../resourceLocator'
import { AvailableCommands, InternalCommands } from '../commands/internal'
import { EXPERIMENT_WORKSPACE_ID, ExpShowOutput } from '../cli/dvc/contract'
import { ExpShowOutput } from '../cli/dvc/contract'
import { ViewKey } from '../webview/constants'
import { BaseRepository } from '../webview/repository'
import { Title } from '../vscode/title'
Expand All @@ -43,7 +43,6 @@ import { Toast } from '../vscode/toast'
import { ConfigKey } from '../vscode/config'
import { checkSignalFile, pollSignalFileForProcess } from '../fileSystem'
import { DVCLIVE_ONLY_RUNNING_SIGNAL_FILE } from '../cli/dvc/constants'
import { COMMITS_SEPARATOR } from '../cli/git/constants'

export const ExperimentsScale = {
...omit(ColumnType, 'TIMESTAMP'),
Expand Down Expand Up @@ -172,10 +171,9 @@ export class Experiments extends BaseRepository<TableData> {
public async setState(data: ExpShowOutput) {
const hadCheckpoints = this.hasCheckpoints()
const dvcLiveOnly = await this.checkSignalFile()
const commitsOutput = await this.getCommitOutput(data)
await Promise.all([
this.columns.transformAndSet(data),
mattseddon marked this conversation as resolved.
Show resolved Hide resolved
this.experiments.transformAndSet(data, dvcLiveOnly, commitsOutput)
this.experiments.transformAndSet(data, dvcLiveOnly)
])

if (hadCheckpoints !== this.hasCheckpoints()) {
Expand Down Expand Up @@ -538,25 +536,6 @@ export class Experiments extends BaseRepository<TableData> {
return this.webviewMessages.sendWebviewMessage()
}

private async getCommitOutput(data: ExpShowOutput | undefined) {
if (!data || data.length === 0) {
return
}
let output = ''
for (const commit of data) {
if (commit.rev === EXPERIMENT_WORKSPACE_ID) {
continue
}
output += await this.internalCommands.executeCommand(
AvailableCommands.GIT_GET_COMMIT_MESSAGES,
this.dvcRoot,
commit.rev
)
output += COMMITS_SEPARATOR
}
return output
}

private createWebviewMessageHandler() {
const webviewMessages = new WebviewMessages(
this.dvcRoot,
Expand Down
45 changes: 7 additions & 38 deletions extension/src/experiments/model/collect.test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
import { collectExperiments } from './collect'
import { COMMITS_SEPARATOR } from '../../cli/git/constants'
import { generateTestExpShowOutput } from '../../test/util/experiments'

describe('collectExperiments', () => {
it('should return an empty array if no commits are present', () => {
const { commits } = collectExperiments(
generateTestExpShowOutput({}),
false,
''
)
const { commits } = collectExperiments(generateTestExpShowOutput({}), false)
expect(commits).toStrictEqual([])
})

Expand All @@ -26,19 +21,19 @@ describe('collectExperiments', () => {
)

it('should define a workspace', () => {
const { workspace } = collectExperiments(expShowWithTwoCommits, false, '')
const { workspace } = collectExperiments(expShowWithTwoCommits, false)

expect(workspace).toBeDefined()
})

it('should find two branches from a repo with two branches', () => {
const { commits } = collectExperiments(expShowWithTwoCommits, false, '')
const { commits } = collectExperiments(expShowWithTwoCommits, false)

expect(commits.length).toStrictEqual(2)
})

it('should list commits in the same order as they are collected', () => {
const { commits } = collectExperiments(expShowWithTwoCommits, false, '')
const { commits } = collectExperiments(expShowWithTwoCommits, false)
const [commitA, commitB] = commits

expect(commitA.id).toStrictEqual('branchA')
Expand All @@ -48,44 +43,19 @@ describe('collectExperiments', () => {
it('should find two experiments on commitA', () => {
const { experimentsByCommit } = collectExperiments(
expShowWithTwoCommits,
false,
''
false
)
expect(experimentsByCommit.get('branchA')?.length).toStrictEqual(2)
})

it('should find no experiments on branchB', () => {
const { experimentsByCommit } = collectExperiments(
expShowWithTwoCommits,
false,
''
false
)
expect(experimentsByCommit.get('branchB')).toBeUndefined()
})

it('should add data from git to commits if git log output is provided', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a test to recreate but after a call to update not after collectExperiments. I was still at fixing failing tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

covered by the integration tests/it('should return the expected rows when given the base fixture'

const { commits } = collectExperiments(
expShowWithTwoCommits,
false,
`61bed4ce8913eca7f73ca754d65bc5daad1520e2\nJohn Smith\n3 days ago\nrefNames:tag: v.1.1\nmessage:add new feature${COMMITS_SEPARATOR}351e42ace3cb6a3a853c65bef285e60748cc6341\nrenovate[bot]\n5 weeks ago\nrefNames:\nmessage:update various dependencies\n* update dvc\n* update dvclive`
)
const [branch1, branch2] = commits
expect(branch1.description).toStrictEqual('add new feature')
expect(branch2.description).toStrictEqual('update various dependencies ...')
expect(branch1.commit).toStrictEqual({
author: 'John Smith',
date: '3 days ago',
message: 'add new feature',
tags: ['v.1.1']
})
expect(branch2.commit).toStrictEqual({
author: 'renovate[bot]',
date: '5 weeks ago',
message: 'update various dependencies\n* update dvc\n* update dvclive',
tags: []
})
})

it('should not collect the same experiment twice', () => {
const main = {
experiments: [
Expand Down Expand Up @@ -114,8 +84,7 @@ describe('collectExperiments', () => {

const { experimentsByCommit, commits } = collectExperiments(
expShowWithDuplicateCommits,
false,
''
false
)

expect(commits.length).toStrictEqual(3)
Expand Down
Loading