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

Exclude sub-project files from file and editor watchers #4283

Merged
merged 6 commits into from
Jul 17, 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
34 changes: 21 additions & 13 deletions extension/src/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { InternalCommands } from '../commands/internal'
import { ExpShowOutput, PlotsOutputOrError } from '../cli/dvc/contract'
import { uniqueValues } from '../util/array'
import { DeferredDisposable } from '../class/deferred'
import { isSameOrChild } from '../fileSystem'
import { isPathInSubProject, isSameOrChild } from '../fileSystem'

export type ExperimentsOutput = {
availableNbCommits: { [branch: string]: number }
Expand All @@ -29,6 +29,7 @@ export abstract class BaseData<
protected readonly internalCommands: InternalCommands
protected collectedFiles: string[] = []

private readonly relSubProjects: string[]
private readonly staticFiles: string[]

private readonly updated: EventEmitter<T> = this.dispose.track(
Expand All @@ -39,6 +40,7 @@ export abstract class BaseData<
dvcRoot: string,
internalCommands: InternalCommands,
updateProcesses: { name: string; process: () => Promise<unknown> }[],
subProjects: string[],
staticFiles: string[] = []
) {
super()
Expand All @@ -49,6 +51,9 @@ export abstract class BaseData<
)
this.internalCommands = internalCommands
this.onDidUpdate = this.updated.event
this.relSubProjects = subProjects.map(subProject =>
relative(this.dvcRoot, subProject)
)
this.staticFiles = staticFiles

this.watchFiles()
Expand Down Expand Up @@ -78,20 +83,23 @@ export abstract class BaseData<
return createFileSystemWatcher(
disposable => this.dispose.track(disposable),
getRelativePattern(this.dvcRoot, '**'),
path => {
const relPath = relative(this.dvcRoot, path)
if (
this.getWatchedFiles().some(
watchedRelPath =>
path.endsWith(watchedRelPath) ||
isSameOrChild(relPath, watchedRelPath)
)
) {
void this.managedUpdate(path)
}
}
path => this.listener(path)
)
}

private listener(path: string) {
const relPath = relative(this.dvcRoot, path)
if (
this.getWatchedFiles().some(
watchedRelPath =>
path.endsWith(watchedRelPath) ||
isSameOrChild(relPath, watchedRelPath)
) &&
!isPathInSubProject(relPath, this.relSubProjects)
) {
void this.managedUpdate(path)
}
}

abstract managedUpdate(path?: string): Promise<void>
}
23 changes: 16 additions & 7 deletions extension/src/experiments/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import { Event, EventEmitter, window } from 'vscode'
import { Disposable, Disposer } from '@hediet/std/disposable'
import { ContextKey, setContextValue } from '../vscode/context'
import { standardizePossiblePath } from '../fileSystem/path'
import { isPathInProject, isPathInSubProject } from '../fileSystem'

const setContextOnDidChangeParamsFiles = (
setActiveEditorContext: (paramsFileActive: boolean) => void,
onDidChangeColumns: Event<void>,
getParamsFiles: () => Set<string>
getParamsFiles: () => Set<string>,
subProjects: string[]
): Disposable =>
onDidChangeColumns(() => {
const path = standardizePossiblePath(
Expand All @@ -16,7 +18,10 @@ const setContextOnDidChangeParamsFiles = (
return
}

if (!getParamsFiles().has(path) && !path.endsWith('dvc.yaml')) {
if (
(!getParamsFiles().has(path) && !path.endsWith('dvc.yaml')) ||
isPathInSubProject(path, subProjects)
) {
return
}
setActiveEditorContext(true)
Expand All @@ -25,7 +30,8 @@ const setContextOnDidChangeParamsFiles = (
const setContextOnDidChangeActiveEditor = (
setActiveEditorContext: (paramsFileActive: boolean) => void,
dvcRoot: string,
getParamsFiles: () => Set<string>
getParamsFiles: () => Set<string>,
subProjects: string[]
): Disposable =>
window.onDidChangeActiveTextEditor(event => {
const path = standardizePossiblePath(event?.document.fileName)
Expand All @@ -34,7 +40,7 @@ const setContextOnDidChangeActiveEditor = (
return
}

if (!path.includes(dvcRoot)) {
if (!isPathInProject(path, dvcRoot, subProjects)) {
return
}

Expand All @@ -49,7 +55,8 @@ export const setContextForEditorTitleIcons = (
disposer: (() => void) & Disposer,
getParamsFiles: () => Set<string>,
experimentsFileFocused: EventEmitter<string | undefined>,
onDidChangeColumns: Event<void>
onDidChangeColumns: Event<void>,
subProjects: string[]
): void => {
const setActiveEditorContext = (experimentsFileActive: boolean) => {
void setContextValue(
Expand All @@ -64,15 +71,17 @@ export const setContextForEditorTitleIcons = (
setContextOnDidChangeParamsFiles(
setActiveEditorContext,
onDidChangeColumns,
getParamsFiles
getParamsFiles,
subProjects
)
)

disposer.track(
setContextOnDidChangeActiveEditor(
setActiveEditorContext,
dvcRoot,
getParamsFiles
getParamsFiles,
subProjects
)
)
}
4 changes: 3 additions & 1 deletion extension/src/experiments/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> {
constructor(
dvcRoot: string,
internalCommands: InternalCommands,
experiments: ExperimentsModel
experiments: ExperimentsModel,
subProjects: string[]
) {
super(
dvcRoot,
internalCommands,
[{ name: 'update', process: () => this.update() }],
subProjects,
['dvc.lock', 'dvc.yaml', 'params.yaml', DOT_DVC]
)

Expand Down
16 changes: 12 additions & 4 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export class Experiments extends BaseRepository<TableData> {
selectBranches: (
branchesSelected: string[]
) => Promise<string[] | undefined>,
subProjects: string[],
data?: ExperimentsData
) {
super(dvcRoot, resourceLocator.beaker)
Expand Down Expand Up @@ -143,7 +144,13 @@ export class Experiments extends BaseRepository<TableData> {
)

this.data = this.dispose.track(
data || new ExperimentsData(dvcRoot, internalCommands, this.experiments)
data ||
new ExperimentsData(
dvcRoot,
internalCommands,
this.experiments,
subProjects
)
)

this.dispose.track(this.data.onDidUpdate(data => this.setState(data)))
Expand All @@ -158,7 +165,7 @@ export class Experiments extends BaseRepository<TableData> {

this.webviewMessages = this.createWebviewMessageHandler()
this.setupInitialData()
this.watchActiveEditor()
this.watchActiveEditor(subProjects)
}

public update() {
Expand Down Expand Up @@ -600,13 +607,14 @@ export class Experiments extends BaseRepository<TableData> {
)
}

private watchActiveEditor() {
private watchActiveEditor(subProjects: string[]) {
setContextForEditorTitleIcons(
this.dvcRoot,
this.dispose,
() => this.columns.getParamsFiles(),
this.experimentsFileFocused,
this.onDidChangeColumns
this.onDidChangeColumns,
subProjects
)
}

Expand Down
4 changes: 3 additions & 1 deletion extension/src/experiments/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ export class WorkspaceExperiments extends BaseWorkspaceWebviews<

public createRepository(
dvcRoot: string,
subProjects: string[],
pipeline: WorkspacePipeline,
resourceLocator: ResourceLocator
) {
Expand All @@ -283,7 +284,8 @@ export class WorkspaceExperiments extends BaseWorkspaceWebviews<
pipeline.getRepository(dvcRoot),
resourceLocator,
this.workspaceState,
(branchesSelected: string[]) => this.selectBranches(branchesSelected)
(branchesSelected: string[]) => this.selectBranches(branchesSelected),
subProjects
)
)

Expand Down
23 changes: 18 additions & 5 deletions extension/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,17 +277,26 @@ class Extension extends Disposable {
public async initialize() {
this.resetMembers()

const dvcRoots = this.getRoots()
const subProjects = this.getSubProjects()

await Promise.all([
this.repositories.create(this.getRoots()),
this.repositoriesTree.initialize(this.getRoots()),
this.pipelines.create(this.getRoots())
this.repositories.create(dvcRoots, subProjects),
this.repositoriesTree.initialize(dvcRoots),
this.pipelines.create(dvcRoots, subProjects)
])
this.experiments.create(
this.getRoots(),
dvcRoots,
subProjects,
this.pipelines,
this.resourceLocator
)
this.plots.create(this.getRoots(), this.resourceLocator, this.experiments)
this.plots.create(
dvcRoots,
subProjects,
this.resourceLocator,
this.experiments
)

return Promise.all([
this.experiments.isReady(),
Expand All @@ -308,6 +317,10 @@ class Extension extends Disposable {
private getRoots() {
return this.setup.getRoots()
}

private getSubProjects() {
return this.setup.getSubProjects()
}
}

let extension: undefined | Extension
Expand Down
52 changes: 51 additions & 1 deletion extension/src/fileSystem/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import {
getModifiedTime,
findOrCreateDvcYamlFile,
writeJson,
writeCsv
writeCsv,
isPathInProject
} from '.'
import { dvcDemoPath } from '../test/util'
import { DOT_DVC } from '../cli/dvc/constants'
Expand Down Expand Up @@ -408,3 +409,52 @@ describe('findOrCreateDvcYamlFile', () => {
)
})
})

describe('isPathInProject', () => {
it('should return true if the path is in the project', () => {
const path = join(dvcDemoPath, 'dvc.yaml')
const dvcRoot = dvcDemoPath
const subProjects: string[] = []
expect(isPathInProject(path, dvcRoot, subProjects)).toBe(true)
})
mattseddon marked this conversation as resolved.
Show resolved Hide resolved

it('should return false if the path is not in the project', () => {
const path = resolve(dvcDemoPath, '..', 'dvc.yaml')
const dvcRoot = dvcDemoPath
const subProjects: string[] = []
expect(isPathInProject(path, dvcRoot, subProjects)).toBe(false)
})

it('should return false if the path is the project', () => {
const path = dvcDemoPath
const dvcRoot = dvcDemoPath
const subProjects: string[] = []
expect(isPathInProject(path, dvcRoot, subProjects)).toBe(false)
})

it('should return false if the path is in the project but also in a sub-project', () => {
const path = join(dvcDemoPath, 'nested1', 'dvc.yaml')
const dvcRoot = dvcDemoPath
const subProjects: string[] = [join(dvcDemoPath, 'nested1')]
expect(isPathInProject(path, dvcRoot, subProjects)).toBe(false)
})

it('should return false if the path is in the project but also in one of many sub-projects', () => {
const path = join(dvcDemoPath, 'nested2', 'dvc.yaml')
const dvcRoot = dvcDemoPath
const subProjects: string[] = [
join(dvcDemoPath, 'nested1'),
join(dvcDemoPath, 'nested2'),
join(dvcDemoPath, 'nested3'),
join(dvcDemoPath, 'nested4')
]
expect(isPathInProject(path, dvcRoot, subProjects)).toBe(false)
})

it('should return true if the path is in the project but not in a sub-project', () => {
const path = join(dvcDemoPath, 'nested1', 'dvc.yaml')
const dvcRoot = dvcDemoPath
const subProjects: string[] = [join(dvcDemoPath, 'nested2')]
expect(isPathInProject(path, dvcRoot, subProjects)).toBe(true)
})
})
14 changes: 14 additions & 0 deletions extension/src/fileSystem/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,20 @@ export const isSameOrChild = (root: string, path: string) => {
return !rel.startsWith('..')
}

export const isPathInSubProject = (
path: string,
subProjects: string[]
): boolean => subProjects.some(dvcRoot => path.startsWith(dvcRoot))

export const isPathInProject = (
path: string | undefined,
dvcRoot: string,
subProjects: string[]
): boolean =>
!!path?.startsWith(dvcRoot) &&
path !== dvcRoot &&
!isPathInSubProject(path, subProjects)

export type Out =
| string
| Record<string, { checkpoint?: boolean; cache?: boolean }>
Expand Down
Loading
Loading