-
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
Faster activation of Python environments such as Conda #8342
Conversation
931acd8
to
a902a7f
Compare
a902a7f
to
ac69845
Compare
@@ -10,7 +10,7 @@ export const IEnvironmentActivationService = Symbol('IEnvironmentActivationServi | |||
export interface IEnvironmentActivationService { | |||
getActivatedEnvironmentVariables( | |||
resource: Resource, | |||
interpreter?: PythonEnvironment, | |||
interpreter: PythonEnvironment, |
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.
Its never undefined, hence (all of these optionals make the code lot more messy).
Fortunately most of them aren't optional anymore (simply because we were always passing the values).
Codecov Report
@@ Coverage Diff @@
## main #8342 +/- ##
======================================
Coverage 71% 71%
======================================
Files 373 375 +2
Lines 23554 23718 +164
Branches 3619 3646 +27
======================================
+ Hits 16881 17038 +157
+ Misses 5211 5209 -2
- Partials 1462 1471 +9
|
@@ -239,6 +236,7 @@ export class PythonInstaller implements IPythonInstaller { | |||
@inject(IMemento) @named(GLOBAL_MEMENTO) private readonly memento: Memento | |||
) {} | |||
|
|||
@traceDecorators.verbose('Installing Product', TraceOptions.Arguments | TraceOptions.BeforeCall) |
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.
More logging
@@ -289,34 +287,6 @@ export class PythonInstaller implements IPythonInstaller { | |||
} | |||
} | |||
|
|||
// eslint-disable-next-line max-classes-per-file | |||
@injectable() | |||
export class EnvironmentActivationService implements IEnvironmentActivationService { |
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.
Moved to a separate file (logic is more complex now)
@@ -78,7 +78,7 @@ export type PythonApi = { | |||
*/ | |||
getActivatedEnvironmentVariables( | |||
resource: Resource, | |||
interpreter?: PythonEnvironment, | |||
interpreter: PythonEnvironment, |
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.
This is never undefined, we always pass this around, hence changed this to not be nullable.
Makes the code cleaner
cancel?: CancellationToken, | ||
reInstallAndUpdate?: boolean, | ||
installPipIfRequired?: boolean | ||
): Promise<InstallerResponse> { | ||
// Precondition | ||
if (isResource(interpreterUri)) { |
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.
This would never happen with our code path, hence removed this, typings ensure this never happens.
I think this was a carry on from when we moved code from Python repo
}); | ||
return; | ||
} | ||
traceVerbose(`Activation Commands received ${activationCommands} for shell ${shellInfo.shell}`); |
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.
Borrowed this logic from Python extension
pythonEnvType: envType, | ||
source: 'jupyter', | ||
failed: true, | ||
reason: 'noActivationCommands' |
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.
Better telemetry, this way when things fall over we know what could be causing issues with kernel startups.
E.g. if we cannot activate a conda enviornment, then we know for certain that this is the cause for kernel startup to fail.
* We've found that doing this in jupyter yields much better results.// Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Stats: In Jupyter activation takes 800ms & the same in Python would take 2.6s, or with a complex Conda (5s vs 9s). | ||
* Note: We cache the activate commands, as this is not something that changes day to day. Its almost a constant. | ||
* Either way, we always fetch the latest from Python extension & update the cache. |
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.
Comments added,
} | ||
} | ||
if (!envVariablesOurSelves.resolved) { | ||
traceVerbose(`Got env vars with python ext faster ${getDisplayPath(interpreter?.path)}`); |
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.
Maybe we should add telemetry to track this?
For #7849
Here are some stats:
So we're definitely saving a few seconds