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.

Sign up for GitHub

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

Replace prompt with quickpick #14885

Merged
merged 2 commits into from
Dec 4, 2020
Merged

Replace prompt with quickpick #14885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a37e5d3
Replace prompt with quickpick for setting logdir
joyceerhl Dec 4, 2020
6cfb978
Feedback from Jeffrey
joyceerhl Dec 4, 2020
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
13 changes: 9 additions & 4 deletions package.nls.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,10 +221,15 @@
"StartPage.folderDesc": "- Open a <div class=\"link\" role=\"button\" onclick={0}>Folder</div><br /> - Open a <div class=\"link\" role=\"button\" onclick={1}>Workspace</div>",
"StartPage.badWebPanelFormatString": "<html><body><h1>{0} is not a valid file name</h1></body></html>",
"Jupyter.extensionRequired": "The Jupyter extension is required to perform that task. Click Yes to open the Jupyter extension installation page.",
"TensorBoard.logDirectoryPrompt" : "Please select a log directory to start TensorBoard with.",
"TensorBoard.useCurrentWorkingDirectory": "Use current working directory",
"TensorBoard.currentDirectory": "Current: {0}",
"TensorBoard.logDirectoryPrompt" : "Select a log directory to start TensorBoard with",
"TensorBoard.progressMessage" : "Starting TensorBoard session...",
"TensorBoard.failedToStartSessionError" : "We failed to start a TensorBoard session due to the following error: {0}",
"TensorBoard.nativeTensorBoardPrompt" : "VS Code now has native TensorBoard support. Would you like to launch TensorBoard?",
"TensorBoard.usingCurrentWorkspaceFolder": "We are using the current workspace folder as the log directory for your TensorBoard session.",
"TensorBoard.selectAFolder": "Select a folder"
"TensorBoard.nativeTensorBoardPrompt" : "VS Code now has native TensorBoard support. Would you like to launch TensorBoard? (Tip: Launch TensorBoard anytime by opening the command palette and searching for \"Launch TensorBoard\".)",
"TensorBoard.selectAFolder": "Select a folder",
"TensorBoard.selectAnotherFolder": "Select another folder",
"TensorBoard.selectAFolderDetail": "Select a log directory containing tfevent files",
"TensorBoard.selectAnotherFolderDetail": "Use the file explorer to select another folder",
"TensorBoard.useCurrentWorkingDirectoryDetail": "TensorBoard will search for tfevent files in all subdirectories of the current working directory"
}
26 changes: 20 additions & 6 deletions src/client/common/utils/localize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,18 @@ export namespace Jupyter {
}

export namespace TensorBoard {
export const useCurrentWorkingDirectoryDetail = localize(
'TensorBoard.useCurrentWorkingDirectoryDetail',
'TensorBoard will search for tfevent files in all subdirectories of the current working directory'
);
export const useCurrentWorkingDirectory = localize(
'TensorBoard.useCurrentWorkingDirectory',
'Use current working directory'
);
export const currentDirectory = localize('TensorBoard.currentDirectory', 'Current: {0}');
export const logDirectoryPrompt = localize(
'TensorBoard.logDirectoryPrompt',
'Please select a log directory to start TensorBoard with.'
'Select a log directory to start TensorBoard with'
);
export const progressMessage = localize('TensorBoard.progressMessage', 'Starting TensorBoard session...');
export const failedToStartSessionError = localize(
Expand All @@ -145,13 +154,18 @@ export namespace TensorBoard {
);
export const nativeTensorBoardPrompt = localize(
'TensorBoard.nativeTensorBoardPrompt',
'VS Code now has native TensorBoard support. Would you like to launch TensorBoard?'
);
export const usingCurrentWorkspaceFolder = localize(
'TensorBoard.usingCurrentWorkspaceFolder',
'We are using the current workspace folder as the log directory for your TensorBoard session.'
'VS Code now has native TensorBoard support. Would you like to launch TensorBoard? (Tip: Launch TensorBoard anytime by opening the command palette and searching for "Launch TensorBoard".)'
);
export const selectAFolder = localize('TensorBoard.selectAFolder', 'Select a folder');
export const selectAFolderDetail = localize(
'TensorBoard.selectAFolderDetail',
'Select a log directory containing tfevent files'
);
export const selectAnotherFolder = localize('TensorBoard.selectAnotherFolder', 'Select another folder');
export const selectAnotherFolderDetail = localize(
'TensorBoard.selectAnotherFolderDetail',
'Use the file explorer to select another folder'
);
}

export namespace LanguageService {
Expand Down
76 changes: 47 additions & 29 deletions src/client/tensorBoard/tensorBoardSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
Progress,
ProgressLocation,
ProgressOptions,
QuickPickItem,
ViewColumn,
WebviewPanel,
window
Expand All @@ -20,7 +21,7 @@ import { _SCRIPTS_DIR, tensorboardLauncher } from '../common/process/internal/sc
import { IProcessServiceFactory, ObservableExecutionResult } from '../common/process/types';
import { IInstaller, InstallerResponse, Product } from '../common/types';
import { createDeferred, sleep } from '../common/utils/async';
import { Common, TensorBoard } from '../common/utils/localize';
import { TensorBoard } from '../common/utils/localize';
import { IInterpreterService } from '../interpreter/contracts';

/**
Expand Down Expand Up @@ -101,20 +102,56 @@ export class TensorBoardSession {
}
}

// Display a prompt asking the user to acknowledge our autopopulated log directory or
private getQuickPickItems(logDir: string | undefined) {
if (logDir) {
const useCwd = {
label: TensorBoard.useCurrentWorkingDirectory(),
detail: TensorBoard.useCurrentWorkingDirectoryDetail()
};
const selectAnotherFolder = {
label: TensorBoard.selectAnotherFolder(),
detail: TensorBoard.selectAnotherFolderDetail()
};
return [useCwd, selectAnotherFolder];
} else {
const selectAFolder = {
label: TensorBoard.selectAFolder(),
detail: TensorBoard.selectAFolderDetail()
};
return [selectAFolder];
}
}

// Display a quickpick asking the user to acknowledge our autopopulated log directory or
// select a new one using the file picker. Default this to the folder that is open in
// the editor, if any, then the directory that the active text editor is in, if any.
private async askUserForLogDir(): Promise<string | undefined> {
const logDir = this.autopopulateLogDirectoryPath();
const gotIt = Common.gotIt();
const useCurrentWorkingDirectory = TensorBoard.useCurrentWorkingDirectory();
const selectAFolder = TensorBoard.selectAFolder();
const message = logDir ? TensorBoard.usingCurrentWorkspaceFolder() : TensorBoard.logDirectoryPrompt();
const prompts = logDir ? [gotIt, selectAFolder] : [selectAFolder];
const selection = await window.showInformationMessage(message, ...prompts);
switch (selection) {
case gotIt:
const selectAnotherFolder = TensorBoard.selectAnotherFolder();
Copy link

Choose a reason for hiding this comment

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

Do we want a don't ask me again? I kept getting this prompt every time I opened a folder.

Copy link
Author

@joyceerhl joyceerhl Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So there are 2 prompts. The prompt you're thinking of is the 'VSCode now has native TensorBoard support' prompt, which does have a do not ask again option that permanently disables it. The other prompt (the one in this PR) is being removed in this PR because we decided users were going to miss it.

Copy link
Author

Choose a reason for hiding this comment

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

This one:

vscode-python/src/client/tensorBoard/tensorBoardPrompt.ts

Lines 49 to 50 in d88c119

case doNotAskAgain:
await this.disablePrompt();

Copy link

Choose a reason for hiding this comment

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

Okay maybe it wasn't clear to me that I needed to disable the first prompt in order for the second one to stop appearing. I wanted to have the first prompt show up all the time (so that if I actually wanted to see the tensor board at some later date it would ask me again), but the second one kept asking me the same question over and over. I didn't want to have to answer the second question the same way more than once.

Copy link
Author

@joyceerhl joyceerhl Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I see, yeah there wasn't a way to disable the the second prompt (the one about using the cwd for the log directory). But that prompt is being removed. And yes the first prompt is being changed to show up all the time now (with a do not show again button for when the user gets sick of seeing it)!

We do still need to ask the user for the log directory on each session launch, because we need to point tensorboard to the logfiles. Maybe we can add a workspace-level setting to 'Always use this log directory'? /cc @jmew

const items: QuickPickItem[] = this.getQuickPickItems(logDir);
const quickPick = window.createQuickPick();
quickPick.title = TensorBoard.logDirectoryPrompt();
quickPick.canSelectMany = false;
quickPick.enabled = false;
quickPick.items = items;
if (logDir) {
quickPick.placeholder = TensorBoard.currentDirectory().format(logDir);
}
const selection = createDeferred<QuickPickItem>();
quickPick.onDidAccept(() => {
quickPick.hide();
selection.resolve(quickPick.selectedItems[0]);
});
quickPick.show();
const item = await selection.promise;
quickPick.dispose();
switch (item.label) {
case useCurrentWorkingDirectory:
return logDir;
case selectAFolder:
case selectAnotherFolder:
return this.showFilePicker();
default:
return undefined;
Expand All @@ -125,7 +162,6 @@ export class TensorBoardSession {
// Times out if it hasn't started up after 1 minute.
// Hold on to the process so we can kill it when the webview is closed.
private async startTensorboardSession(logDir: string): Promise<boolean> {
const cwd = this.getFullyQualifiedLogDirectory(logDir);
const pythonExecutable = await this.interpreterService.getActiveInterpreter();
if (!pythonExecutable) {
return false;
Expand All @@ -145,12 +181,12 @@ export class TensorBoardSession {

const processService = await this.processServiceFactory.create();
const args = tensorboardLauncher([logDir]);
const observable = processService.execObservable(pythonExecutable.path, args, { cwd });
const observable = processService.execObservable(pythonExecutable.path, args);

const result = await window.withProgress(
progressOptions,
(_progress: Progress<{}>, token: CancellationToken) => {
traceInfo(`Starting TensorBoard with log directory ${cwd}...`);
traceInfo(`Starting TensorBoard with log directory ${logDir}...`);

const spawnTensorBoard = this.waitForTensorBoardStart(observable);
const userCancellation = createPromiseFromCancellation({
Expand Down Expand Up @@ -247,24 +283,6 @@ export class TensorBoardSession {
}
}

// TensorBoard accepts absolute or relative log directory paths to tfevent files.
// It uses these files to populate its visualizations. If given a relative path,
// TensorBoard resolves them against the current working directory. Make the
// chosen filepath explicit in our logs. If a workspace folder is open, ensure
// we pass it as cwd to the spawned process. If there is no rootPath available,
// explicitly pass process.cwd, which is what `spawn` would use by default anyway.
private getFullyQualifiedLogDirectory(logDir: string) {
if (path.isAbsolute(logDir)) {
return logDir;
}
const rootPath = this.workspaceService.rootPath;
if (rootPath) {
return path.resolve(rootPath, logDir);
} else {
return path.resolve(process.cwd(), logDir);
}
}

private autopopulateLogDirectoryPath(): string | undefined {
if (this.workspaceService.rootPath) {
return this.workspaceService.rootPath;
Expand Down