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

Simplify DVC CLI Location/Version Logic #3784

Merged
merged 11 commits into from
May 3, 2023
107 changes: 46 additions & 61 deletions extension/src/cli/dvc/discovery.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
import {
LATEST_TESTED_CLI_VERSION,
MAX_CLI_VERSION,
MIN_CLI_VERSION
} from './contract'
import { LATEST_TESTED_CLI_VERSION } from './contract'
import { CliCompatible, isVersionCompatible } from './version'
import { IExtensionSetup } from '../../interfaces'
import { Toast } from '../../vscode/toast'
Expand All @@ -12,90 +8,82 @@ import {
getConfigValue,
setUserConfigValue
} from '../../vscode/config'
import { getPythonBinPath } from '../../extensions/python'
import { getFirstWorkspaceFolder } from '../../vscode/workspaceFolders'
import { delay } from '../../util/time'
import { SetupSection } from '../../setup/webview/contract'

export const warnUnableToVerifyVersion = () =>
Toast.warnWithOptions(
const warnWithSetupAction = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current possible options are:

Version not found:
An error was thrown when trying to access the CLI.
buttons: Setup, Don't Show Again

Version can not be verified:
The extension cannot initialize as we were unable to verify the DVC CLI version.
Button: Setup

Version is found but it's not up to date:
The extension cannot initialize because you are using the wrong version of the CLI
Buttons: Setup

Version is found but it's too far ahead:
The extension cannot initialize because you are using the wrong version of the extension.
Button: Setup

What do we think? Should the toasts be more/less simplified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated Toasts:

Version not found:
An error was thrown when trying to access the CLI.
buttons: Setup, Don't Show Again

Version can not be verified:
The extension cannot initialize as we were unable to verify the DVC CLI version.
Button: Setup

Version behind or ahead
The extension cannot initialize because the DVC CLI version is incompatible.
Button: Setup

setup: IExtensionSetup,
warningText: string
): Promise<void> => {
const response = await Toast.warnWithOptions(warningText, Response.SHOW_SETUP)

if (response === Response.SHOW_SETUP) {
return setup.showSetup(SetupSection.DVC)
}
}

const warnUnableToVerifyVersion = (setup: IExtensionSetup) => {
void warnWithSetupAction(
setup,
'The extension cannot initialize as we were unable to verify the DVC CLI version.'
)
}

export const warnVersionIncompatible = (
version: string,
const warnVersionIncompatible = (
setup: IExtensionSetup,
update: 'CLI' | 'extension'
): void => {
void Toast.warnWithOptions(
`The extension cannot initialize because you are using version ${version} of the DVC CLI. The expected version is ${MIN_CLI_VERSION} <= DVC < ${MAX_CLI_VERSION}. Please upgrade to the most recent version of the ${update} and reload this window.`
void warnWithSetupAction(
setup,
`The extension cannot initialize because you are using the wrong version of the ${update}.`
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
)
}

export const warnAheadOfLatestTested = (): void => {
const warnAheadOfLatestTested = (): void => {
void Toast.warnWithOptions(
`The located DVC CLI is at least a minor version ahead of the latest version the extension was tested with (${LATEST_TESTED_CLI_VERSION}). This could lead to unexpected behaviour. Please upgrade to the most recent version of the extension and reload this window.`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left this toast alone for now since we don't have any info on the latest tested cli version in Setup yet. Once we update our DVC Setup page (task is in #3434 list), we can simplify this one or remove entirely.

Copy link
Member

Choose a reason for hiding this comment

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

let's aim to remove it

)
}

const warnUserCLIInaccessible = async (
setup: IExtensionSetup,
warningText: string
setup: IExtensionSetup
): Promise<void> => {
if (getConfigValue<boolean>(ConfigKey.DO_NOT_SHOW_CLI_UNAVAILABLE)) {
return
}

const response = await Toast.warnWithOptions(
warningText,
'An error was thrown when trying to access the CLI.',
Response.SHOW_SETUP,
Response.NEVER
)

switch (response) {
case Response.SHOW_SETUP:
return setup.showSetup(SetupSection.EXPERIMENTS)
return setup.showSetup(SetupSection.DVC)
case Response.NEVER:
return setUserConfigValue(ConfigKey.DO_NOT_SHOW_CLI_UNAVAILABLE, true)
}
}

const warnUserCLIInaccessibleAnywhere = async (
setup: IExtensionSetup,
globalDvcVersion: string | undefined
): Promise<void> => {
const binPath = await getPythonBinPath()

return warnUserCLIInaccessible(
setup,
`The extension is unable to initialize. The CLI was not located using the interpreter provided by the Python extension. ${
globalDvcVersion ? globalDvcVersion + ' is' : 'The CLI is also not'
} installed globally. For auto Python environment activation, ensure the correct interpreter is set. Active Python interpreter: ${
binPath || 'unset'
}.`
)
}

const warnUser = (
setup: IExtensionSetup,
cliCompatible: CliCompatible,
version: string | undefined
cliCompatible: CliCompatible
): void => {
if (!setup.shouldWarnUserIfCLIUnavailable()) {
return
}
switch (cliCompatible) {
case CliCompatible.NO_BEHIND_MIN_VERSION:
return warnVersionIncompatible(version as string, 'CLI')
return warnVersionIncompatible(setup, 'CLI')
case CliCompatible.NO_CANNOT_VERIFY:
void warnUnableToVerifyVersion()
void warnUnableToVerifyVersion(setup)
return
case CliCompatible.NO_MAJOR_VERSION_AHEAD:
return warnVersionIncompatible(version as string, 'extension')
return warnVersionIncompatible(setup, 'extension')
case CliCompatible.NO_NOT_FOUND:
void warnUserCLIInaccessible(
setup,
'An error was thrown when trying to access the CLI.'
)
void warnUserCLIInaccessible(setup)
return
case CliCompatible.YES_MINOR_VERSION_AHEAD_OF_TESTED:
return warnAheadOfLatestTested()
Expand Down Expand Up @@ -128,7 +116,6 @@ const getVersionDetails = async (
): Promise<
CanRunCli & {
cliCompatible: CliCompatible
version: string | undefined
}
> => {
const version = await setup.getCliVersion(cwd, tryGlobalCli)
Expand All @@ -140,11 +127,12 @@ const getVersionDetails = async (
const processVersionDetails = (
setup: IExtensionSetup,
cliCompatible: CliCompatible,
version: string | undefined,
isAvailable: boolean,
isCompatible: boolean | undefined
isCompatible: boolean | undefined,
version: string | undefined
): CanRunCli => {
warnUser(setup, cliCompatible, version)
warnUser(setup, cliCompatible)

return {
isAvailable,
isCompatible,
Expand All @@ -159,21 +147,17 @@ const tryGlobalFallbackVersion = async (
const tryGlobal = await getVersionDetails(setup, cwd, true)
const { cliCompatible, isAvailable, isCompatible, version } = tryGlobal

if (setup.shouldWarnUserIfCLIUnavailable() && !isCompatible) {
void warnUserCLIInaccessibleAnywhere(setup, version)
}
if (
setup.shouldWarnUserIfCLIUnavailable() &&
cliCompatible === CliCompatible.YES_MINOR_VERSION_AHEAD_OF_TESTED
) {
warnAheadOfLatestTested()
}

if (isCompatible) {
setup.unsetPythonBinPath()
}

return { isAvailable, isCompatible, version }
return processVersionDetails(
setup,
cliCompatible,
isAvailable,
isCompatible,
version
)
}

const extensionCanAutoRunCli = async (
Expand All @@ -190,12 +174,13 @@ const extensionCanAutoRunCli = async (
if (pythonCliCompatible === CliCompatible.NO_NOT_FOUND) {
return tryGlobalFallbackVersion(setup, cwd)
}

return processVersionDetails(
setup,
pythonCliCompatible,
pythonVersion,
pythonVersionIsAvailable,
pythonVersionIsCompatible
pythonVersionIsCompatible,
pythonVersion
)
}

Expand All @@ -213,9 +198,9 @@ export const extensionCanRunCli = async (
return processVersionDetails(
setup,
cliCompatible,
version,
isAvailable,
isCompatible
isCompatible,
version
)
}

Expand Down
34 changes: 24 additions & 10 deletions extension/src/setup/runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ import { Toast } from '../vscode/toast'
import { Response } from '../vscode/response'
import { VscodePython } from '../extensions/python'
import { executeProcess } from '../process/execution'
import {
LATEST_TESTED_CLI_VERSION,
MAX_CLI_VERSION,
MIN_CLI_VERSION
} from '../cli/dvc/contract'
import { LATEST_TESTED_CLI_VERSION, MIN_CLI_VERSION } from '../cli/dvc/contract'
import { extractSemver, ParsedSemver } from '../cli/dvc/version'
import { delay } from '../util/time'
import { Title } from '../vscode/title'
Expand Down Expand Up @@ -455,6 +451,24 @@ describe('run', () => {
expect(mockedInitialize).toHaveBeenCalledTimes(1)
})

it('should send a specific message to the user if the extension is unable to verify the version', async () => {
const unverifyableVersion = 'not a valid version'
mockedGetFirstWorkspaceFolder.mockReturnValueOnce(mockedCwd)
mockedShouldWarnUserIfCLIUnavailable.mockReturnValueOnce(true)
mockedGetCliVersion.mockResolvedValueOnce(unverifyableVersion)

await run(setup)
await flushPromises()
expect(mockedWarnWithOptions).toHaveBeenCalledTimes(1)
expect(mockedWarnWithOptions).toHaveBeenCalledWith(
'The extension cannot initialize as we were unable to verify the DVC CLI version.',
Response.SHOW_SETUP
)
expect(mockedGetCliVersion).toHaveBeenCalledTimes(1)
expect(mockedResetMembers).toHaveBeenCalledTimes(1)
expect(mockedInitialize).not.toHaveBeenCalled()
})

it('should send a specific message to the user if the Python extension is being used, the CLI is not available in the virtual environment and the global CLI is not compatible', async () => {
const belowMinVersion = '2.0.0'
mockedGetFirstWorkspaceFolder.mockReturnValueOnce(mockedCwd)
Expand All @@ -472,9 +486,8 @@ describe('run', () => {
await flushPromises()
expect(mockedWarnWithOptions).toHaveBeenCalledTimes(1)
expect(mockedWarnWithOptions).toHaveBeenCalledWith(
`The extension is unable to initialize. The CLI was not located using the interpreter provided by the Python extension. ${belowMinVersion} is installed globally. For auto Python environment activation, ensure the correct interpreter is set. Active Python interpreter: ${mockedPythonPath}.`,
Response.SHOW_SETUP,
Response.NEVER
'The extension cannot initialize because you are using the wrong version of the CLI.',
Response.SHOW_SETUP
)
expect(mockedGetCliVersion).toHaveBeenCalledTimes(2)
expect(mockedResetMembers).toHaveBeenCalledTimes(1)
Expand Down Expand Up @@ -539,7 +552,8 @@ describe('run', () => {
await flushPromises()
expect(mockedWarnWithOptions).toHaveBeenCalledTimes(1)
expect(mockedWarnWithOptions).toHaveBeenCalledWith(
`The extension cannot initialize because you are using version ${MajorAhead} of the DVC CLI. The expected version is ${MIN_CLI_VERSION} <= DVC < ${MAX_CLI_VERSION}. Please upgrade to the most recent version of the extension and reload this window.`
'The extension cannot initialize because you are using the wrong version of the extension.',
'Setup'
)
expect(mockedGetCliVersion).toHaveBeenCalledTimes(1)
expect(mockedResetMembers).toHaveBeenCalledTimes(1)
Expand All @@ -564,7 +578,7 @@ describe('run', () => {
await flushPromises()
expect(mockedWarnWithOptions).toHaveBeenCalledTimes(1)
expect(mockedWarnWithOptions).toHaveBeenCalledWith(
`The extension is unable to initialize. The CLI was not located using the interpreter provided by the Python extension. The CLI is also not installed globally. For auto Python environment activation, ensure the correct interpreter is set. Active Python interpreter: ${mockedPythonPath}.`,
'An error was thrown when trying to access the CLI.',
Response.SHOW_SETUP,
Response.NEVER
)
Expand Down
9 changes: 1 addition & 8 deletions extension/src/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,8 @@ export class Status extends Disposable {
return
}

if (this.available) {
return {
command: RegisteredCommands.EXTENSION_SETUP_WORKSPACE,
title: Title.SETUP_WORKSPACE
}
}

return {
command: RegisteredCommands.SETUP_SHOW,
command: RegisteredCommands.SETUP_SHOW_DVC,
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
title: Title.SHOW_SETUP
}
}
Expand Down
15 changes: 6 additions & 9 deletions extension/src/test/suite/status.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ suite('Status Test Suite', () => {
const loadingText = '$(loading~spin) DVC (Global)'
const waitingText = '$(circle-large-outline) DVC (Global)'

const setupWorkspaceCommand = {
command: RegisteredCommands.EXTENSION_SETUP_WORKSPACE,
title: Title.SETUP_WORKSPACE
const setupShowCommand = {
command: RegisteredCommands.SETUP_SHOW_DVC,
title: Title.SHOW_SETUP
}

it('should show the correct status of the cli', async () => {
Expand Down Expand Up @@ -88,7 +88,7 @@ suite('Status Test Suite', () => {
await status.setAvailability(true)

expect(mockStatusBarItem.text).to.equal(waitingText)
expect(mockStatusBarItem.command).to.deep.equal(setupWorkspaceCommand)
expect(mockStatusBarItem.command).to.deep.equal(setupShowCommand)

processStarted.fire(firstFinishedCommand)

Expand Down Expand Up @@ -118,15 +118,12 @@ suite('Status Test Suite', () => {
})

expect(mockStatusBarItem.text).to.equal(waitingText)
expect(mockStatusBarItem.command).to.deep.equal(setupWorkspaceCommand)
expect(mockStatusBarItem.command).to.deep.equal(setupShowCommand)

await status.setAvailability(false)

expect(mockStatusBarItem.text).to.equal(disabledText)
expect(mockStatusBarItem.command).to.deep.equal({
command: RegisteredCommands.SETUP_SHOW,
title: Title.SHOW_SETUP
})
expect(mockStatusBarItem.command).to.deep.equal(setupShowCommand)
})

it('should floor the number of workers at 0', async () => {
Expand Down