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

Don't activate Python extension until needed (Tests Needed) #5193

Closed
kieferrm opened this issue Mar 12, 2021 · 33 comments
Closed

Don't activate Python extension until needed (Tests Needed) #5193

kieferrm opened this issue Mar 12, 2021 · 33 comments
Assignees
Labels
debt Code quality issues notebook-getting-started perf Performance issues

Comments

@kieferrm
Copy link
Member

Issue Type: Bug

When I startup VS Code I get this in a workspace that exclusively contains markdown files. This is the only window I have open. Why?

image

Extension version: 2021.2.636928669
VS Code version: Code - Insiders 1.55.0-insider (Universal) (9b2ee7fb7d970b9a628fdb3545bdd01d67078d16, 2021-03-11T05:15:23.749Z)
OS version: Darwin arm64 20.3.0

System Info
Item Value
CPUs undefined
GPU Status 2d_canvas: enabled
gpu_compositing: enabled
metal: disabled_off
multiple_raster_threads: enabled_on
oop_rasterization: enabled
opengl: enabled_on
protected_video_decode: unavailable_off
rasterization: enabled
skia_renderer: disabled_off_ok
video_decode: enabled
webgl: enabled
webgl2: enabled
Load (avg) 2, 3, 2
Memory (System) 16.00GB (0.28GB free)
Process Argv --enable-proposed-api lszomoru.lszomoru-proposed-api-sample --crash-reporter-id ef10dfea-3602-4c74-ba1a-109ce7da7825
Screen Reader no
VM 0%
A/B Experiments
vsliv695:30137379
vsins829:30139715
vsliv368:30146709
vsreu685:30147344
python383:30185418
pythonvspyt602:30263608
vspor879:30202332
vspor708:30202333
vspor363:30204092
vstry244:30244315
pythonvsdeb440:30224570
pythonvsded773:30223139
pythonvspyt875:30259475
pythonvsnew554:30265444
pythontb:30258533
vscoresta800:30265947
vspre833:30267464

@kieferrm
Copy link
Member Author

kieferrm commented Mar 12, 2021

The activation also happens btw for an empty window, but this time the extension thinks I have a python interpreter, just not the right one:

image

@brettcannon brettcannon self-assigned this Mar 15, 2021
@brettcannon
Copy link
Member

Our activation events are explicitly tied to python commands, language selection, or Python-specific files:

https://github.com/microsoft/vscode-python/blob/f85d5912f89da029a3f4fffcc2d46a387f70cff1/package.json#L51-L94

The only thing that changed recently is we trigger on more Python-related files:

  • pyproject.toml
  • Pipfile
  • setup.py
  • requirements.txt
  • manage.py
  • app.py

Otherwise our activationEvents have not changed in 4 months and that was only for registering a command.

Only other thing involving start-up is we added "requiresWorkspaceTrust": "onStart" recently, but our understanding is that doesn't force activation. Is there a way to see in the logs why an extension was activated?

@kieferrm
Copy link
Member Author

I realized this is because of DonJayamanne/typescript-notebook#4.

@kieferrm
Copy link
Member Author

The real trouble here is the dependency from the Jupyter extension to the Python extension. I.e. when Jupyter activates, Python activates although, as in this case, there is no reason for Python to be active.

@brettcannon
Copy link
Member

The long-term plan is to break the hard dependency and make it optional. I suspect Jupyter already has an issue open for this, but in case they don't I'll transfer your issue over to there.

@brettcannon brettcannon transferred this issue from microsoft/vscode-python Mar 17, 2021
@greazer
Copy link
Member

greazer commented Mar 17, 2021

Looks like a dupe of #4965

@greazer greazer closed this as completed Mar 17, 2021
@rchiodo rchiodo reopened this Mar 17, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Mar 17, 2021

This is a dependency issue. Because the typescript notebook extension loads on startup, it also loads the jupyter extension.

@greazer
Copy link
Member

greazer commented Mar 17, 2021

The jupyter extension should not need to load the Python extension. The dependency isn't supposed to work that way.

@greazer
Copy link
Member

greazer commented Mar 17, 2021

Dupe of #4526

@greazer greazer closed this as completed Mar 17, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Mar 18, 2021

Not a dupe of #4526.

@rchiodo rchiodo reopened this Mar 18, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Mar 18, 2021

The root cause of this is here:

const isPythonExtensionInstalled = serviceManager.get<IPythonExtensionChecker>(IPythonExtensionChecker);

Maybe there's another way we can determine if the python extension is installed other than calling the getExtension API.

@DonJayamanne
Copy link
Contributor

I think this is how it has always worked, looks like just because the extension is installed we assume it has been actiavted and try to use it (when we try to use this, then we actiavte it if hasn't been activated).

@brettcannon brettcannon removed their assignment Mar 18, 2021
@DonJayamanne
Copy link
Contributor

DonJayamanne commented Mar 18, 2021

When i open a typescript notebook, we still activate Python extension (if it is installed) because we're searching for kernels in interpreters.
Not sure i can get around this.
Basically we don't know what kind of notebook the user is dealing with.

E.g. assume the following

  • User has python extension installed
  • User creates a blank notebook
  • Now user goes into kernel picker:
    • How do we know whther user is creating python notebook or not.
    • We need to display kernels in Python interpreters.

I.e. i don't see any way around this.
Thoughts?

@greazer @IanMatthewHuff @rchiodo @DavidKutu @joyceerhl @claudiaregio /cc

@rchiodo
Copy link
Contributor

rchiodo commented Mar 18, 2021

Notebook seems okay to load the python extension. It just shouldn't load in a directory with markdown files.

@greazer
Copy link
Member

greazer commented Mar 18, 2021

One way we could potentially work around this problem is to prompt the user to set a language for the first cell of their notebook before the kernel picker is populated with anything. Right?

Example: Assuming your scenario, the user has a blank notebook with no language set in the first cell.

  • User click on the kernel picker. They just see a prompt describing the situation. Could be modal or not modal. Or perhaps the popup for the kernel picker just says "Choose a language for the first cell to choose a kernel"

@IanMatthewHuff
Copy link
Member

I dunno, still seems like if I was a Julia or R user that I wouldn't want the python extension activated just for opening a notebook. Creating a blank notebook could save a default language to use for new notebooks. Then only activate and search for python language notebooks?

@DonJayamanne
Copy link
Contributor

User click on the kernel picker. They just see a prompt describing the situation. Could be modal or not modal. Or perhaps the popup for the kernel picker just says "Choose a language for the first cell to choose a kernel"

Not sure i like this.
I'm with @rchiodo, its ok for notebooks.
I.e. my argument is, if the user opens a notebook and they have python extension installed, then I its ok to activate Python extension.

Having to get the user to first select a language then the kernel feels like too many steps.

@DonJayamanne
Copy link
Contributor

Creating a blank notebook could save a default language to use for new notebooks. Then only activate and search for python language notebooks?

Sure, but what if i now want to change the kernel to a Python kernel? At this point we won't be displaying all the python interpreters as the Python extension hasn't been actiavted.

I think its ok to activate the python extension.
The crux of the problem is Python extnesion is displaying prompts upon activation and fixing that would not cause issues. I.e. if we didn't have popups would this be an issue, i don't think so.

I'd say, if user installed Python extnesion, then its ok to activate.

@DonJayamanne
Copy link
Contributor

Python extenion should display prompts like select a pytohn interpreetr only when user attempts to do somethign with python, editing a file, or running some python command that requires python.

Else I don't see the need for the prompt just because the extension actiavted. But that's an issue at their end.

@greazer
Copy link
Member

greazer commented Mar 18, 2021

Ok. I think I see what you're saying Don. Here's a summary...

  • If the user does not have the Python extension installed and a user opens a blank or non-Python notebook, then they will not be prompted to install the Python extension (let alone have it activate) Question: what will the user see if they click on the kernel picker in this state?
  • If the user does not have the Python extension installed and a user opens a Python notebook OR the user chooses Python as the language for the first cell, they WILL be prompted to install the Python extension.
  • If the user has the Python extension installed and a user opens a non-Python notebook, then the Python extension will be activated (as silently and quickly as possible) such that we can show a list of Python interpreters/kernels in addition to other jupyter kernels we find when the user clicks on the kernel picker.
  • If the user has the Python extension installed and a user opens a Python notebook, then the Python extension is activated and kernel picker displays everything as we normally do today.

Is this complete? Do I have it right? Not sure I think it's the right answer yet, though.

@greazer greazer modified the milestones: May 2021 Release, June 2021 Release Apr 26, 2021
@DonJayamanne
Copy link
Contributor

This has been done, but we need tests for this.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jun 8, 2021

his has been done, but we need tests for this.

We've regressed as a result of the changes for Kernel Push (tests really need to be added for this so we don't regress).
More details here #6185 (comment)

@greazer
Copy link
Member

greazer commented Aug 2, 2021

It seems ok to activate the python extension if there are python cells in it. This just needs to e thought through with our getting started investigation.

@greazer greazer added the perf Performance issues label Aug 2, 2021
@greazer greazer modified the milestones: August 2021, old August 2021 Aug 10, 2021
@IanMatthewHuff IanMatthewHuff changed the title Don't activate Python extension until needed Don't activate Python extension until needed (Tests Needed) Sep 2, 2021
@rchiodo rchiodo added debt Code quality issues and removed issue_grooming_rich labels Oct 18, 2021
@rchiodo rchiodo self-assigned this Mar 1, 2022
@rchiodo
Copy link
Contributor

rchiodo commented Mar 1, 2022

I believe the test would be:

  • create untitled notebook
  • verify python extension does not load

But how does it create the notebook controllers then?

@rchiodo
Copy link
Contributor

rchiodo commented Mar 1, 2022

Actually that seems like it should load the python extension. As long as it doesn't prevent opening (which it currently doesn't).

I don't think there's a scenario to test for here then.

@rchiodo rchiodo closed this as completed Mar 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues notebook-getting-started perf Performance issues
Projects
None yet
Development

No branches or pull requests

7 participants