-
Notifications
You must be signed in to change notification settings - Fork 293
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
More progress indicator when starting a kernel #8584
Changes from 2 commits
f66973a
db66dd9
eef1b66
7741fbf
a87896e
ec3999c
8703ca7
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 |
---|---|---|
@@ -1 +1 @@ | ||
{ "run_number": 85, "head_ref": "envActivationService", "results": {} } | ||
{ "run_number": 93, "head_ref": "addProgressIndicator", "results": {} } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,8 @@ import { printEnvVariablesToFile } from './internal/scripts'; | |
import { ProcessService } from './proc'; | ||
import { BufferDecoder } from './decoder'; | ||
import { testOnlyMethod } from '../utils/decorators'; | ||
import { KernelProgressReporter } from '../../datascience/progress/kernelProgressReporter'; | ||
import { DataScience } from '../utils/localize'; | ||
|
||
const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6'; | ||
const ENVIRONMENT_TIMEOUT = 30000; | ||
|
@@ -132,6 +134,18 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi | |
public async getActivatedEnvironmentVariables( | ||
resource: Resource, | ||
@logValue<PythonEnvironment>('path') interpreter: PythonEnvironment | ||
): Promise<NodeJS.ProcessEnv | undefined> { | ||
const title = DataScience.activatingPythonEnvironment().format( | ||
interpreter.displayName || getDisplayPath(interpreter.path) | ||
); | ||
return KernelProgressReporter.wrapAndReportProgress(resource, title, () => | ||
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. Display progress message |
||
this.getActivatedEnvironmentVariablesImpl(resource, interpreter) | ||
); | ||
} | ||
@traceDecorators.verbose('Getting activated env variables', TraceOptions.BeforeCall | TraceOptions.Arguments) | ||
public async getActivatedEnvironmentVariablesImpl( | ||
resource: Resource, | ||
@logValue<PythonEnvironment>('path') interpreter: PythonEnvironment | ||
): Promise<NodeJS.ProcessEnv | undefined> { | ||
const stopWatch = new StopWatch(); | ||
const envVariablesOurSelves = createDeferredFromPromise( | ||
|
@@ -218,9 +232,19 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi | |
): Promise<NodeJS.ProcessEnv | undefined> { | ||
const workspaceKey = this.workspace.getWorkspaceFolderIdentifier(resource); | ||
const key = `${workspaceKey}_${interpreter && getInterpreterHash(interpreter)}`; | ||
|
||
if (this.activatedEnvVariablesCache.has(key)) { | ||
return this.activatedEnvVariablesCache.get(key); | ||
} | ||
|
||
const shellInfo = defaultShells[this.platform.osType]; | ||
const envType = interpreter?.envType; | ||
if (!shellInfo) { | ||
traceWarning( | ||
`Cannot get activated env variables for ${getDisplayPath( | ||
interpreter?.path | ||
)}, shell cannot be determined.` | ||
); | ||
sendTelemetryEvent(Telemetry.GetActivatedEnvironmentVariables, 0, { | ||
envType, | ||
pythonEnvType: envType, | ||
|
@@ -231,68 +255,65 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi | |
return; | ||
} | ||
|
||
if (this.activatedEnvVariablesCache.has(key)) { | ||
return this.activatedEnvVariablesCache.get(key); | ||
} | ||
|
||
const customEnvVarsPromise = this.envVarsService.getEnvironmentVariables(resource); | ||
// If this is a conda environment that supports conda run, then we don't need conda activation commands. | ||
let activationCommandsPromise = this.getActivationCommands(resource, interpreter); | ||
if (interpreter.envType === EnvironmentType.Conda) { | ||
const condaVersion = await this.condaService.getCondaVersion(); | ||
if (condaVersionSupportsLiveStreaming(condaVersion)) { | ||
activationCommandsPromise = Promise.resolve([] as string[]); | ||
const promise = (async () => { | ||
const customEnvVarsPromise = this.envVarsService.getEnvironmentVariables(resource); | ||
// If this is a conda environment that supports conda run, then we don't need conda activation commands. | ||
let activationCommandsPromise = this.getActivationCommands(resource, interpreter); | ||
if (interpreter.envType === EnvironmentType.Conda) { | ||
const condaVersion = await this.condaService.getCondaVersion(); | ||
if (condaVersionSupportsLiveStreaming(condaVersion)) { | ||
activationCommandsPromise = Promise.resolve([] as string[]); | ||
} | ||
} | ||
} | ||
|
||
const [activationCommands, customEnvVars] = await Promise.all([ | ||
activationCommandsPromise, | ||
customEnvVarsPromise | ||
]); | ||
|
||
// Check cache. | ||
const customEnvVariablesHash = getTelemetrySafeHashedString(JSON.stringify(customEnvVars)); | ||
const cachedVariables = this.getActivatedEnvVariablesFromCache( | ||
resource, | ||
interpreter, | ||
customEnvVariablesHash, | ||
activationCommands | ||
); | ||
if (cachedVariables) { | ||
traceVerbose(`Got activation Env Vars from cache`); | ||
return cachedVariables; | ||
} | ||
const [activationCommands, customEnvVars] = await Promise.all([ | ||
activationCommandsPromise, | ||
customEnvVarsPromise | ||
]); | ||
|
||
const condaActivation = async () => { | ||
const stopWatch = new StopWatch(); | ||
try { | ||
const env = await this.getCondaEnvVariables(resource, interpreter); | ||
sendTelemetryEvent(Telemetry.GetActivatedEnvironmentVariables, stopWatch.elapsedTime, { | ||
envType, | ||
pythonEnvType: envType, | ||
source: 'jupyter', | ||
failed: Object.keys(env || {}).length === 0, | ||
reason: Object.keys(env || {}).length === 0 ? 'emptyFromCondaRun' : undefined | ||
}); | ||
return env; | ||
} catch (ex) { | ||
sendTelemetryEvent(Telemetry.GetActivatedEnvironmentVariables, stopWatch.elapsedTime, { | ||
envType, | ||
pythonEnvType: envType, | ||
source: 'jupyter', | ||
failed: true, | ||
reason: 'unhandledError' | ||
}); | ||
traceError('Failed to get activated environment variables ourselves', ex); | ||
// Check cache. | ||
const customEnvVariablesHash = getTelemetrySafeHashedString(JSON.stringify(customEnvVars)); | ||
const cachedVariables = this.getActivatedEnvVariablesFromCache( | ||
resource, | ||
interpreter, | ||
customEnvVariablesHash, | ||
activationCommands | ||
); | ||
if (cachedVariables) { | ||
traceVerbose(`Got activation Env Vars from cache`); | ||
return cachedVariables; | ||
} | ||
}; | ||
|
||
const promise = (async () => { | ||
const condaActivation = async () => { | ||
const stopWatch = new StopWatch(); | ||
try { | ||
const env = await this.getCondaEnvVariables(resource, interpreter); | ||
sendTelemetryEvent(Telemetry.GetActivatedEnvironmentVariables, stopWatch.elapsedTime, { | ||
envType, | ||
pythonEnvType: envType, | ||
source: 'jupyter', | ||
failed: Object.keys(env || {}).length === 0, | ||
reason: Object.keys(env || {}).length === 0 ? 'emptyFromCondaRun' : undefined | ||
}); | ||
return env; | ||
} catch (ex) { | ||
sendTelemetryEvent(Telemetry.GetActivatedEnvironmentVariables, stopWatch.elapsedTime, { | ||
envType, | ||
pythonEnvType: envType, | ||
source: 'jupyter', | ||
failed: true, | ||
reason: 'unhandledError' | ||
}); | ||
traceError('Failed to get activated environment variables ourselves', ex); | ||
} | ||
}; | ||
|
||
if (interpreter.envType !== EnvironmentType.Conda) { | ||
return this.getActivatedEnvVarsUsingActivationCommands(resource, interpreter); | ||
} | ||
return condaActivation(); | ||
})(); | ||
|
||
promise.catch(() => { | ||
if (this.activatedEnvVariablesCache.get(key) === promise) { | ||
this.activatedEnvVariablesCache.delete(key); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import { suppressShutdownErrors } from './raw-kernel/rawKernel'; | |
import { IJupyterSession, ISessionWithSocket, KernelSocketInformation } from './types'; | ||
import { KernelInterruptTimeoutError } from './errors/kernelInterruptTimeoutError'; | ||
import { SessionDisposedError } from './errors/sessionDisposedError'; | ||
import { KernelProgressReporter } from './progress/kernelProgressReporter'; | ||
|
||
/** | ||
* Exception raised when starting a Jupyter Session fails. | ||
|
@@ -295,30 +296,40 @@ export abstract class BaseJupyterSession implements IJupyterSession { | |
isRestartSession?: boolean | ||
): Promise<void> { | ||
if (session && session.kernel) { | ||
traceInfo(`Waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`); | ||
const progress = isRestartSession | ||
? undefined | ||
: KernelProgressReporter.reportProgress( | ||
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. Display progress messages |
||
this.resource, | ||
localize.DataScience.waitingForJupyterSessionToBeIdle() | ||
); | ||
try { | ||
traceInfo(`Waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`); | ||
|
||
// When our kernel connects and gets a status message it triggers the ready promise | ||
const deferred = createDeferred<string>(); | ||
const handler = (_session: Kernel.IKernelConnection, status: KernelMessage.Status) => { | ||
if (status == 'idle') { | ||
deferred.resolve(status); | ||
// When our kernel connects and gets a status message it triggers the ready promise | ||
const deferred = createDeferred<string>(); | ||
const handler = (_session: Kernel.IKernelConnection, status: KernelMessage.Status) => { | ||
if (status == 'idle') { | ||
deferred.resolve(status); | ||
} | ||
}; | ||
session.kernel.statusChanged?.connect(handler); | ||
if (session.kernel.status == 'idle') { | ||
deferred.resolve(session.kernel.status); | ||
} | ||
}; | ||
session.kernel.statusChanged?.connect(handler); | ||
if (session.kernel.status == 'idle') { | ||
deferred.resolve(session.kernel.status); | ||
} | ||
|
||
const result = await Promise.race([deferred.promise, sleep(timeout)]); | ||
session.kernel.statusChanged?.disconnect(handler); | ||
traceInfo(`Finished waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`); | ||
const result = await Promise.race([deferred.promise, sleep(timeout)]); | ||
session.kernel.statusChanged?.disconnect(handler); | ||
traceInfo(`Finished waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`); | ||
|
||
if (result.toString() == 'idle') { | ||
return; | ||
if (result.toString() == 'idle') { | ||
return; | ||
} | ||
// If we throw an exception, make sure to shutdown the session as it's not usable anymore | ||
this.shutdownSession(session, this.statusHandler, isRestartSession).ignoreErrors(); | ||
throw new JupyterWaitForIdleError(localize.DataScience.jupyterLaunchTimedOut()); | ||
} finally { | ||
progress?.dispose(); | ||
} | ||
// If we throw an exception, make sure to shutdown the session as it's not usable anymore | ||
this.shutdownSession(session, this.statusHandler, isRestartSession).ignoreErrors(); | ||
throw new JupyterWaitForIdleError(localize.DataScience.jupyterLaunchTimedOut()); | ||
} else { | ||
throw new JupyterInvalidKernelError(undefined); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,7 +107,7 @@ export class JupyterKernelService { | |
kernel.interpreter.path | ||
)} for ${kernel.id}` | ||
); | ||
await this.updateKernelEnvironment(kernel.interpreter, kernel.kernelSpec, specFile, token); | ||
await this.updateKernelEnvironment(resource, kernel.interpreter, kernel.kernelSpec, specFile, token); | ||
} | ||
} | ||
|
||
|
@@ -200,6 +200,7 @@ export class JupyterKernelService { | |
return kernelSpecFilePath; | ||
} | ||
private async updateKernelEnvironment( | ||
resource: Resource, | ||
interpreter: PythonEnvironment | undefined, | ||
kernel: IJupyterKernelSpec, | ||
specFile: string, | ||
|
@@ -242,7 +243,7 @@ export class JupyterKernelService { | |
// Get the activated environment variables (as a work around for `conda run` and similar). | ||
// This ensures the code runs within the context of an activated environment. | ||
specModel.env = await this.activationHelper | ||
.getActivatedEnvironmentVariables(undefined, interpreter, true) | ||
.getActivatedEnvironmentVariables(resource, interpreter, true) | ||
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. Pass resource so we can display progress message |
||
.catch(noop) | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
.then((env) => (env || {}) as any); | ||
|
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.
Ensure we make these calls only once