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

Prevent unnecessary activation of the Python extension #5199

Merged
merged 14 commits into from
Mar 19, 2021
Merged

Conversation

DonJayamanne
Copy link
Contributor

For #5193

) {
const previouslyInstalled = this.extensionChecker.isPythonExtensionInstalled;
if (!previouslyInstalled) {
this.extensions.onDidChange(
Copy link
Contributor Author

@DonJayamanne DonJayamanne Mar 19, 2021

Choose a reason for hiding this comment

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

If the extension is subsequently installed, then register the hooks

@codecov-io
Copy link

codecov-io commented Mar 19, 2021

Codecov Report

Merging #5199 (9a5a488) into main (d99989e) will decrease coverage by 0%.
The diff coverage is 71%.

❗ Current head 9a5a488 differs from pull request most recent head dc3a1d2. Consider uploading reports for the commit dc3a1d2 to get more accurate results

@@          Coverage Diff          @@
##            main   #5199   +/-   ##
=====================================
- Coverage     73%     73%   -1%     
=====================================
  Files        402     402           
  Lines      26403   26456   +53     
  Branches    3799    3809   +10     
=====================================
- Hits       19432   19429    -3     
- Misses      5403    5444   +41     
- Partials    1568    1583   +15     
Impacted Files Coverage Δ
src/client/activation/activationManager.ts 53% <0%> (-3%) ⬇️
src/client/api/types.ts 100% <ø> (ø)
...tascience/telemetry/workspaceInterpreterTracker.ts 60% <0%> (-8%) ⬇️
...tascience/kernel-launcher/kernelDaemonPreWarmer.ts 73% <54%> (-5%) ⬇️
...t/datascience/telemetry/interpreterCountTracker.ts 76% <57%> (-9%) ⬇️
src/client/interpreter/display/visibilityFilter.ts 84% <64%> (-3%) ⬇️
...datascience/telemetry/interpreterPackageTracker.ts 79% <66%> (+<1%) ⬆️
src/client/api/pythonApi.ts 64% <67%> (-1%) ⬇️
...lient/datascience/telemetry/interpreterPackages.ts 84% <80%> (+<1%) ⬆️
...upyter/interpreter/jupyterInterpreterStateStore.ts 94% <85%> (-6%) ⬇️
... and 19 more

@DonJayamanne DonJayamanne marked this pull request as ready for review March 19, 2021 02:11
@DonJayamanne DonJayamanne requested a review from a team as a code owner March 19, 2021 02:11
.catch(noop);
if (this.extensionChecker.isPythonExtensionInstalled) {
if (this.extensionChecker.isPythonExtensionActive && this.eventHandlerAdded) {
this.hookupOnDidChangeInterpreterEvent();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add event handlers only if extension has been activated, and add it later once python loads.

@@ -220,11 +216,6 @@ export class NativeEditorStorage implements INotebookStorage {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return (notebookData.metadata.language_info.codemirror_mode as any).version;
}
// Use the active interpreter if allowed
if (this.extensionChecker.isPythonExtensionInstalled) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is totally unnecessary, we're just storing this in metadata for language, today we generate all of this information from the kernel, hence we have more accurate information than this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this was more stuff that slowed down notebooks, unnecessary when creating blank notebooks.

@@ -19,12 +19,14 @@ export interface ILanguageServer extends Disposable {

export const IPythonApiProvider = Symbol('IPythonApi');
export interface IPythonApiProvider {
onDidActivatePythonExtension: Event<void>;
Copy link
Member

Choose a reason for hiding this comment

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

Not fully sure if I'm suggesting this. But does this seem to you like it should be on IPythonExtensionChecker? That has everything relevant to checking if the extension is installed or active.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No can do, its the PythonApiProvider that activates the Python extension. So only PythonAPIProvider knows if the python extension was activated or not.
VS Code doesn't fire events when extensions load.
Here we're only interested in situations when we load the python extension & other parts of the code can then react to that.

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

Looks solid. Just had one question on the interfaces and one spot that looked like maybe a bug with the event hookup.

Copy link
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

Waiting on check that active interpreter is still used for interactive window

Copy link
Contributor Author

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Waiting on check that active interpreter is still used for interactive window

Don't quite follow this comment.

@rchiodo
Copy link
Contributor

rchiodo commented Mar 19, 2021

See the comment in localKernelFinder.ts

@DonJayamanne
Copy link
Contributor Author

See the comment in localKernelFinder.ts

Somehow that comment was missing when i replied. anyways.

@DonJayamanne DonJayamanne requested a review from joyceerhl March 19, 2021 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants