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

Patch Studio API update timinig issue #4619

Merged
merged 1 commit into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
58 changes: 56 additions & 2 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import omit from 'lodash.omit'
import { ColumnLike, addStarredToColumns } from './columns/like'
import { setContextForEditorTitleIcons } from './context'
import { ExperimentsModel } from './model'
import { collectRemoteExpShas } from './model/collect'
import {
pickExperiment,
pickExperiments,
Expand Down Expand Up @@ -229,9 +230,10 @@ export class Experiments extends BaseRepository<TableData> {
}

public async unsetPushing(ids: string[]) {
await this.update()
await Promise.all([this.update(), this.patchStudioApiTimingIssue(ids)])

this.experiments.unsetPushing(ids)
return this.notifyChanged()
return this.webviewMessages.sendWebviewMessage()
}

public hasCheckpoints() {
Expand Down Expand Up @@ -759,4 +761,56 @@ export class Experiments extends BaseRepository<TableData> {
const columnLikes = addStarredToColumns(columns)
return pickColumnToFilter(columnLikes)
}

private async patchStudioApiTimingIssue(ids: string[]) {
if (!this.studio.isConnected()) {
return
}

const [, { lsRemoteOutput }] = await Promise.all([
this.waitForStudioUpdate(),
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] If we don't wait for this the assumePushed shas can be overwritten.

this.waitForRemoteUpdate()
])

const remoteExpShas = collectRemoteExpShas(lsRemoteOutput)

const shas = []
for (const { id, sha } of this.experiments.getExperiments()) {
if (ids.includes(id) && sha && remoteExpShas.has(sha)) {
shas.push(sha)
}
}

this.experiments.assumePushed(shas)
}

private waitForStudioUpdate() {
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] I tried to combine the waitForStudioUpdate & waitForRemoteUpdate methods but the code was a lot harder to read.

return new Promise(resolve => {
const listener = this.dispose.track(
this.data.onDidUpdate(data => {
if (!isStudioExperimentsOutput(data)) {
return
}
this.dispose.untrack(listener)
listener.dispose()
resolve(undefined)
})
)
})
}

private waitForRemoteUpdate(): Promise<{ lsRemoteOutput: string }> {
return new Promise(resolve => {
const listener = this.dispose.track(
this.data.onDidUpdate(data => {
if (!isRemoteExperimentsOutput(data)) {
return
}
this.dispose.untrack(listener)
listener.dispose()
resolve(data)
})
)
})
}
}
6 changes: 3 additions & 3 deletions extension/src/experiments/model/collect.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { collectExperiments, collectRemoteExpRefs } from './collect'
import { collectExperiments, collectRemoteExpShas } from './collect'
import { generateTestExpShowOutput } from '../../test/util/experiments'
import { ExpShowOutput } from '../../cli/dvc/contract'

Expand Down Expand Up @@ -102,7 +102,7 @@ describe('collectExperiments', () => {
})
})

describe('collectRemoteExpRefs', () => {
describe('collectRemoteExpShas', () => {
it('should parse the git ls-remote output', () => {
const output = `263e4408e42a0e215b0f70b36b2ab7b65a160d7e refs/exps/a9/b32d14966b9be1396f2211d9eb743359708a07/vital-taal
d4f2a35773ead55b7ce4b596f600e98360e49372 refs/exps/a9/b32d14966b9be1396f2211d9eb743359708a07/whole-bout
Expand All @@ -111,7 +111,7 @@ describe('collectRemoteExpRefs', () => {
390aef747f45fc49ec8928b24771f8950d057393 refs/exps/a9/d8057e088d46842f15c3b6d1bb2e4befd5f677/known-flus
142a803b83ff784ba1106cc4ad0ba03310da6186 refs/exps/a9/d8057e088d46842f15c3b6d1bb2e4befd5f677/tight-lira
21ce298cd1743405a0d73f5cb4cf52289ffa3276 refs/exps/bf/6ca8a35911bc6e62fb9bcaa506d4f4e185450c/crumb-orcs`
const remoteExpShas = collectRemoteExpRefs(output)
const remoteExpShas = collectRemoteExpShas(output)
expect(remoteExpShas).toStrictEqual(
new Set([
'263e4408e42a0e215b0f70b36b2ab7b65a160d7e',
Expand Down
2 changes: 1 addition & 1 deletion extension/src/experiments/model/collect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ export const collectRunningInWorkspace = (
}
}

export const collectRemoteExpRefs = (lsRemoteOutput: string): Set<string> => {
export const collectRemoteExpShas = (lsRemoteOutput: string): Set<string> => {
const acc = new Set<string>()
for (const shaAndRef of trimAndSplit(lsRemoteOutput)) {
const [sha] = shaAndRef.split(/\s+/)
Expand Down
13 changes: 11 additions & 2 deletions extension/src/experiments/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
collectAddRemoveCommitsDetails,
collectExperiments,
collectOrderedCommitsAndExperiments,
collectRemoteExpRefs,
collectRemoteExpShas,
collectRunningInQueue,
collectRunningInWorkspace
} from './collect'
Expand Down Expand Up @@ -181,7 +181,7 @@ export class ExperimentsModel extends ModelWithPersistence {
}

public transformAndSetRemote(lsRemoteOutput: string) {
const remoteExpShas = collectRemoteExpRefs(lsRemoteOutput)
const remoteExpShas = collectRemoteExpShas(lsRemoteOutput)
this.remoteExpShas = remoteExpShas
}

Expand Down Expand Up @@ -563,6 +563,15 @@ export class ExperimentsModel extends ModelWithPersistence {
this.studioPushedExperiments = pushed
}

public assumePushed(shas: string[]) {
for (const sha of shas) {
if (this.studioPushedExperiments.includes(sha)) {
continue
}
this.studioPushedExperiments.push(sha)
}
}

public hasDvcLiveOnlyRunning() {
return !!this.dvcLiveOnlyExpName
}
Expand Down
4 changes: 4 additions & 0 deletions extension/src/experiments/studio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ export class Studio extends DeferredDisposable {
return this.accessTokenSet
}

public isConnected() {
return !!this.baseUrl
}

public getAccessToken() {
return this.studioAccessToken
}
Expand Down
68 changes: 65 additions & 3 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
CancellationToken,
WorkspaceConfiguration,
MessageItem,
ConfigurationTarget
ConfigurationTarget,
EventEmitter
} from 'vscode'
import {
DEFAULT_EXPERIMENTS_OUTPUT,
Expand Down Expand Up @@ -94,6 +95,7 @@ import { MAX_SELECTED_EXPERIMENTS } from '../../../experiments/model/status'
import { Pipeline } from '../../../pipeline'
import { ColumnLike } from '../../../experiments/columns/like'
import * as Clipboard from '../../../vscode/clipboard'
import { ExperimentsOutput } from '../../../data'

const { openFileInEditor } = FileSystem

Expand Down Expand Up @@ -593,8 +595,18 @@ suite('Experiments Test Suite', () => {
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should handle a message to push an experiment', async () => {
const { experiments, mockMessageReceived } =
await stubWorkspaceGettersWebview(disposable)
const {
experiments,
experimentsModel,
messageSpy,
mockMessageReceived,
webview
} = await stubWorkspaceGettersWebview(disposable)

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const studio = (experiments as any).studio

const mockIsConnected = stub(studio, 'isConnected').returns(false)

const mockExpId = 'exp-e7a67'

Expand Down Expand Up @@ -634,6 +646,15 @@ suite('Experiments Test Suite', () => {
RegisteredCommands.SETUP_SHOW_STUDIO_CONNECT
)

const experimentWithoutLink = experimentsModel
.getRowData()[1]
.subRows?.find(({ id }) => id === mockExpId)

expect(experimentWithoutLink?.studioLinkType).not.to.equal(
StudioLinkType.PUSHED
)

mockIsConnected.restore()
mockGetStudioAccessToken.resetBehavior()

const tokenFound = new Promise(resolve =>
Expand Down Expand Up @@ -664,6 +685,25 @@ suite('Experiments Test Suite', () => {
})
)

const dataUpdated = disposable.track(
new EventEmitter<ExperimentsOutput>()
)
// eslint-disable-next-line @typescript-eslint/no-explicit-any
;(experiments as any).data.onDidUpdate = dataUpdated.event

let calls = 0

const remoteUpdated = new Promise(resolve =>
disposable.track(
dataUpdated.event(() => {
calls = calls + 1
if (calls === 2) {
resolve(undefined)
}
})
)
)

mockMessageReceived.fire({
payload: [mockExpId],
type: MessageFromWebviewType.PUSH_EXPERIMENT
Expand All @@ -677,6 +717,28 @@ suite('Experiments Test Suite', () => {
message:
"Experiment major-lamb is up to date on Git remote 'origin'.\nView your experiments in [Studio](https://studio.iterative.ai/user/mattseddon/projects/vscode-dvc-demo-ynm6t3jxdx)"
})

messageSpy.restore()
const mockShow = stub(webview, 'show')

const messageSent = new Promise(resolve =>
mockShow.callsFake(() => {
resolve(undefined)
return Promise.resolve(true)
})
)
dataUpdated.fire({ live: [], pushed: [], baseUrl: '' })
dataUpdated.fire({
lsRemoteOutput: `42b8736b08170529903cd203a1f40382a4b4a8cd refs/exps/a9/b32d14966b9be1396f2211d9eb743359708a07/test-branch
4fb124aebddb2adf1545030907687fa9a4c80e70 refs/exps/a9/53c3851f46955fa3e2b8f6e1c52999acc8c9ea77/${mockExpId}`
})
await Promise.all([remoteUpdated, messageSent])

const experimentWithLink = experimentsModel
.getRowData()[1]
.subRows?.find(({ id }) => id === mockExpId)

expect(experimentWithLink?.studioLinkType).to.equal(StudioLinkType.PUSHED)
}).timeout(WEBVIEW_TEST_TIMEOUT)

it("should handle a message to copy an experiment's Studio link", async () => {
Expand Down
6 changes: 4 additions & 2 deletions extension/src/test/suite/experiments/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ export const stubWorkspaceGettersWebview = async (
experiments,
experimentsModel,
messageSpy,
mockMessageReceived
mockMessageReceived,
webview
} = await buildExperimentsWebview({ disposer })

return {
Expand All @@ -395,6 +396,7 @@ export const stubWorkspaceGettersWebview = async (
experimentsModel,
messageSpy,
...stubWorkspaceExperiments(dvcRoot, experiments),
mockMessageReceived
mockMessageReceived,
webview
}
}
Loading