-
Notifications
You must be signed in to change notification settings - Fork 290
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
Default plots to PNG output, and support showing PNGs or SVGs in the existing PlotViewer control #7140
Default plots to PNG output, and support showing PNGs or SVGs in the existing PlotViewer control #7140
Changes from all commits
18695d3
421da07
8ac93a1
ce3a50b
e753f98
c9c0bf9
05b84ef
a46bc35
f04a51a
c3732b0
a990ea7
012e533
cc9497b
e37aac9
0d221f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Default plot output to just PNG, and support showing PNGs or SVGs in the Plot Viewer control. The enablePlotViewer setting still turns on both PNG and SVG plot output, but it's now off by default, not on. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1792,8 +1792,8 @@ | |
}, | ||
"jupyter.enablePlotViewer": { | ||
"type": "boolean", | ||
"default": true, | ||
"description": "Modify plot output so that it can be expanded into a plot viewer window.", | ||
"default": false, | ||
"description": "Modify plot output to include SVG output, for better display in the plot viewer", | ||
"scope": "resource" | ||
}, | ||
"jupyter.codeLenses": { | ||
|
@@ -2078,6 +2078,7 @@ | |
"glob": "^7.1.2", | ||
"hash.js": "^1.1.7", | ||
"iconv-lite": "^0.4.21", | ||
"image-size": "^1.0.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DonJayamanne image-size was added here. |
||
"inversify": "^5.0.1", | ||
"is-online": "^9.0.0", | ||
"jsonc-parser": "^2.0.3", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -658,10 +658,10 @@ export namespace CodeSnippets { | |
]; | ||
export const ChangeDirectoryCommentIdentifier = '# ms-toolsai.jupyter added'; // Not translated so can compare. | ||
export const ImportIPython = '{0}\nfrom IPython import get_ipython\n\n{1}'; | ||
export const MatplotLibInitSvg = `import matplotlib\n%matplotlib inline\n${Identifiers.MatplotLibDefaultParams} = dict(matplotlib.rcParams)\n%config InlineBackend.figure_formats = {'svg', 'png'}`; | ||
export const MatplotLibInitPng = `import matplotlib\n%matplotlib inline\n${Identifiers.MatplotLibDefaultParams} = dict(matplotlib.rcParams)\n%config InlineBackend.figure_formats = {'png'}`; | ||
export const ConfigSvg = `%config InlineBackend.figure_formats = {'svg', 'png'}`; | ||
export const ConfigPng = `%config InlineBackend.figure_formats = {'png'}`; | ||
export const MatplotLibInitSvg = `import matplotlib\n%matplotlib inline\n${Identifiers.MatplotLibDefaultParams} = dict(matplotlib.rcParams)\n%config InlineBackend.figure_formats = ['svg', 'png']`; | ||
export const MatplotLibInitPng = `import matplotlib\n%matplotlib inline\n${Identifiers.MatplotLibDefaultParams} = dict(matplotlib.rcParams)\n%config InlineBackend.figure_formats = ['png']`; | ||
export const ConfigSvg = `%config InlineBackend.figure_formats = ['svg', 'png']`; | ||
export const ConfigPng = `%config InlineBackend.figure_formats = ['png']`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this has been broken for a long time / forever? For multiples it seems to take {} or [], but for the singular it can't: Guessing we didn't notice this before as png is Jupyter default I'm pretty sure. So it would fail to run, but then just get PNG anyways from the Jupyter default. |
||
export const UpdateCWDAndPath = | ||
'import os\nimport sys\n%cd "{0}"\nif os.getcwd() not in sys.path:\n sys.path.insert(0, os.getcwd())'; | ||
export const disableJedi = '%config Completer.use_jedi = False'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,13 +7,13 @@ import { Observable } from 'rxjs/Observable'; | |
import { Subscriber } from 'rxjs/Subscriber'; | ||
import * as uuid from 'uuid/v4'; | ||
import * as path from 'path'; | ||
import { Disposable, Event, EventEmitter, Uri } from 'vscode'; | ||
import { ColorThemeKind, Disposable, Event, EventEmitter, Uri } from 'vscode'; | ||
import { CancellationToken } from 'vscode-jsonrpc'; | ||
import { ServerStatus } from '../../../datascience-ui/interactive-common/mainState'; | ||
import { IApplicationShell, IVSCodeNotebook, IWorkspaceService } from '../../common/application/types'; | ||
import { CancellationError, createPromiseFromCancellation } from '../../common/cancellation'; | ||
import '../../common/extensions'; | ||
import { traceError, traceInfo, traceInfoIf, traceWarning } from '../../common/logger'; | ||
import { traceError, traceInfo, traceWarning } from '../../common/logger'; | ||
|
||
import { IConfigurationService, IDisposableRegistry, Resource } from '../../common/types'; | ||
import { createDeferred, Deferred, waitForPromise } from '../../common/utils/async'; | ||
|
@@ -41,20 +41,18 @@ import { KernelConnectionMetadata } from './kernels/types'; | |
// eslint-disable-next-line @typescript-eslint/no-require-imports | ||
import cloneDeep = require('lodash/cloneDeep'); | ||
import { concatMultilineString, formatStreamText, splitMultilineString } from '../../../datascience-ui/common'; | ||
import { isCI, PYTHON_LANGUAGE } from '../../common/constants'; | ||
import { PYTHON_LANGUAGE } from '../../common/constants'; | ||
import { IFileSystem } from '../../common/platform/types'; | ||
import { RefBool } from '../../common/refBool'; | ||
import { PythonEnvironment } from '../../pythonEnvironments/info'; | ||
import { handleTensorBoardDisplayDataOutput } from '../notebook/helpers/executionHelpers'; | ||
import { | ||
getInterpreterFromKernelConnectionMetadata, | ||
getKernelConnectionLanguage, | ||
isPythonKernelConnection, | ||
sendTelemetryForPythonKernelExecutable | ||
isPythonKernelConnection | ||
} from './kernels/helpers'; | ||
import { isResourceNativeNotebook } from '../notebook/helpers/helpers'; | ||
import { sendKernelTelemetryEvent } from '../telemetry/telemetry'; | ||
import { IPythonExecutionFactory } from '../../common/process/types'; | ||
|
||
class CellSubscriber { | ||
public get startTime(): number { | ||
|
@@ -157,7 +155,6 @@ class CellSubscriber { | |
export class JupyterNotebookBase implements INotebook { | ||
private sessionStartTime: number; | ||
private pendingCellSubscriptions: CellSubscriber[] = []; | ||
private ranInitialSetup = false; | ||
private _resource: Resource; | ||
private _identity: Uri; | ||
private _disposed: boolean = false; | ||
|
@@ -209,8 +206,7 @@ export class JupyterNotebookBase implements INotebook { | |
private workspace: IWorkspaceService, | ||
private applicationService: IApplicationShell, | ||
private fs: IFileSystem, | ||
private readonly vscNotebook: IVSCodeNotebook, | ||
private readonly executionFactory: IPythonExecutionFactory | ||
private readonly vscNotebook: IVSCodeNotebook | ||
) { | ||
this.sessionStartTime = Date.now(); | ||
|
||
|
@@ -287,93 +283,6 @@ export class JupyterNotebookBase implements INotebook { | |
return this.session ? this.session.waitForIdle(timeoutMs) : Promise.resolve(); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intialize code (working dir / startup commands / plot init) now all moved from Notebook into Kernel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gave a quick peek with notebooks and IW. Startup command, new working directory, and plots all looked good after a notebook start as well as after a kernel restart. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DonJayamanne could you take a re-look? It's still blocked on the change requested. |
||
// Set up our initial plotting and imports | ||
// eslint-disable-next-line complexity | ||
public async initialize(cancelToken?: CancellationToken): Promise<void> { | ||
if (this.ranInitialSetup) { | ||
return; | ||
} | ||
this.ranInitialSetup = true; | ||
this._workingDirectory = undefined; | ||
|
||
traceInfo(`Initial setup for ${this.identity.toString()} starting ...`); | ||
|
||
try { | ||
// When we start our notebook initial, change to our workspace or user specified root directory | ||
await this.updateWorkingDirectoryAndPath(); | ||
traceInfoIf(isCI, `Initial setup after for updateWorkingDirectoryAndPath ...`); | ||
let isDefinitelyNotAPythonKernel = false; | ||
if ( | ||
this._executionInfo.kernelConnectionMetadata?.kind === 'startUsingKernelSpec' && | ||
this._executionInfo.kernelConnectionMetadata.kernelSpec.language && | ||
this._executionInfo.kernelConnectionMetadata.kernelSpec.language.toLowerCase() !== | ||
PYTHON_LANGUAGE.toLocaleLowerCase() | ||
) { | ||
isDefinitelyNotAPythonKernel = true; | ||
} | ||
if ( | ||
this._executionInfo.kernelConnectionMetadata?.kind === 'connectToLiveKernel' && | ||
this._executionInfo.kernelConnectionMetadata.kernelModel.language && | ||
this._executionInfo.kernelConnectionMetadata.kernelModel.language.toLowerCase() !== | ||
PYTHON_LANGUAGE.toLocaleLowerCase() | ||
) { | ||
isDefinitelyNotAPythonKernel = true; | ||
} | ||
|
||
const settings = this.configService.getSettings(this.resource); | ||
if (settings && settings.themeMatplotlibPlots) { | ||
// We're theming matplotlibs, so we have to setup our default state. | ||
traceInfoIf(isCI, `Initialize config for plots for ${this.identity.toString()}`); | ||
if (!isDefinitelyNotAPythonKernel) { | ||
await this.initializeMatplotlib(cancelToken); | ||
} | ||
} else { | ||
this.initializedMatplotlib = false; | ||
const configInit = | ||
(!settings || settings.enablePlotViewer) && | ||
!isResourceNativeNotebook(this._resource, this.vscNotebook, this.fs) | ||
? CodeSnippets.ConfigSvg | ||
: CodeSnippets.ConfigPng; | ||
traceInfoIf(isCI, `Initialize config for plots for ${this.identity.toString()}`); | ||
if (!isDefinitelyNotAPythonKernel) { | ||
await this.executeSilently(configInit, cancelToken); | ||
} | ||
} | ||
traceInfoIf(isCI, `Initial setup for ${this.identity.toString()} half way ...`); | ||
if ( | ||
!isDefinitelyNotAPythonKernel && | ||
this._executionInfo.connectionInfo.localLaunch && | ||
this._executionInfo.kernelConnectionMetadata | ||
) { | ||
await sendTelemetryForPythonKernelExecutable( | ||
this, | ||
this._resource?.fsPath || this._identity.fsPath, | ||
this._executionInfo.kernelConnectionMetadata, | ||
this.executionFactory | ||
); | ||
} | ||
// Run any startup commands that we specified. Support the old form too | ||
let setting = settings.runStartupCommands || settings.runMagicCommands; | ||
|
||
// Convert to string in case we get an array of startup commands. | ||
if (Array.isArray(setting)) { | ||
setting = setting.join(`\n`); | ||
} | ||
|
||
if (setting) { | ||
// Cleanup the line feeds. User may have typed them into the settings UI so they will have an extra \\ on the front. | ||
traceInfoIf(isCI, 'Begin Run startup code for notebook'); | ||
const cleanedUp = setting.replace(/\\n/g, '\n'); | ||
const cells = await this.executeSilently(cleanedUp, cancelToken); | ||
traceInfoIf(isCI, `Run startup code for notebook: ${cleanedUp} - results: ${cells.length}`); | ||
} | ||
|
||
traceInfo(`Initial setup complete for ${this.identity.toString()}`); | ||
} catch (e) { | ||
traceWarning('Failed initialize', e); | ||
} | ||
} | ||
|
||
public clear(_id: string): void { | ||
// We don't do anything as we don't cache results in this class. | ||
noop(); | ||
|
@@ -528,9 +437,6 @@ export class JupyterNotebookBase implements INotebook { | |
// Restart our kernel | ||
await this.session.restart(timeoutMs); | ||
|
||
// Rerun our initial setup for the notebook | ||
await this.runInitialSetup(); | ||
|
||
// Tell our loggers | ||
this.loggers.forEach((l) => l.onKernelRestarted(this.getNotebookId())); | ||
traceInfo(`Time to restart kernel is ${(Date.now() - this.sessionStartTime) / 1000}s`); | ||
|
@@ -541,13 +447,6 @@ export class JupyterNotebookBase implements INotebook { | |
throw this.getDisposedError(); | ||
} | ||
|
||
public async runInitialSetup() { | ||
this.ranInitialSetup = false; | ||
traceInfo('restartKernel - initialSetup'); | ||
await this.initialize(); | ||
traceInfo('restartKernel - initialSetup completed'); | ||
} | ||
|
||
@captureTelemetry(Telemetry.InterruptJupyterTime) | ||
public async interruptKernel(timeoutMs: number): Promise<InterruptResult> { | ||
if (this.session) { | ||
|
@@ -573,10 +472,6 @@ export class JupyterNotebookBase implements INotebook { | |
this.sessionStartTime = Date.now(); | ||
traceWarning('Kernel restarting during interrupt'); | ||
|
||
// Indicate we have to redo initial setup. We can't wait for starting though | ||
// because sometimes it doesn't happen | ||
this.ranInitialSetup = false; | ||
|
||
// Indicate we restarted the race below | ||
restarted.resolve([]); | ||
|
||
|
@@ -716,18 +611,12 @@ export class JupyterNotebookBase implements INotebook { | |
public async setKernelConnection(connectionMetadata: KernelConnectionMetadata, timeoutMS: number): Promise<void> { | ||
// We need to start a new session with the new kernel spec | ||
if (this.session) { | ||
// Turn off setup | ||
this.ranInitialSetup = false; | ||
|
||
// Change the kernel on the session | ||
await this.session.changeKernel(this.resource, connectionMetadata, timeoutMS); | ||
|
||
// Change our own kernel spec | ||
// Only after session was successfully created. | ||
this._executionInfo.kernelConnectionMetadata = connectionMetadata; | ||
|
||
// Rerun our initial setup | ||
await this.initialize(); | ||
} else { | ||
// Change our own kernel spec | ||
this._executionInfo.kernelConnectionMetadata = connectionMetadata; | ||
|
@@ -821,6 +710,16 @@ export class JupyterNotebookBase implements INotebook { | |
// get a request to update style | ||
await this.executeSilently(matplobInit, cancelToken); | ||
|
||
const useDark = this.applicationService.activeColorTheme.kind === ColorThemeKind.Dark; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was in the kernel.ts version, but not here so I added it as it looks correct for native theming. |
||
if (!settings.ignoreVscodeTheme) { | ||
// Reset the matplotlib style based on if dark or not. | ||
await this.executeSilently( | ||
useDark | ||
? "matplotlib.style.use('dark_background')" | ||
: `matplotlib.rcParams.update(${Identifiers.MatplotLibDefaultParams})` | ||
); | ||
} | ||
|
||
// Use this flag to detemine if we need to rerun this or not. | ||
this.initializedMatplotlib = true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to discuss this. I feel that customers might still want the old behavior, so I left the setting in, but defaulted it to false instead of true, so they can opt into getting the extra SVGs if they want. My code will prefer SVG output to any PNG output if present.