Do not call activate the discovery component before registering all the classes by karrtikr · Pull Request #15379 · microsoft/vscode-python

karrtikr · 2021-02-10T21:15:14Z

We were activating the discovery component before we even register all the classes that the component might need, which led in a few issues as mentioned in #15132 (comment). Removed this activation. Note we already activate using the component adapter for the discovery component so not calling activate() is not a problem.

Opened #15380 to fix this in a general way for all components.

Reverts some parts of #15132

… the classes

karthiknadig · 2021-02-10T22:26:17Z

This cannot be merged by itself. It will turn off the discovery component.

karrtikr · 2021-02-10T22:30:01Z

Note we already activate using the component adapter for the discovery component so not calling activate() is not a problem.

@karthiknadig I have mentioned this in the description. It won't turn off the component as we already activate using the component adapter.

Right here:

vscode-python/src/client/activation/activationManager.ts

Lines 75 to 76 in db8e1e2

    
           // Get latest interpreter list in the background. 
        
           this.interpreterService.getInterpreters(resource).ignoreErrors();

karrtikr · 2021-02-10T22:43:25Z

I have cleared this up in the comment as well.

karthiknadig · 2021-02-10T23:03:03Z

Ah! yes. I missed that part. Thanks for taking care of this.

ericsnowcurrently

Does it not work to just move activateLegacy() first?

karrtikr · 2021-02-10T23:47:46Z

@ericsnowcurrently We can do that too, but we need not block activating components on all of activateLegacy(), we only need to block it on registration. So the right solution will be to separate registration from activateLegacy() which #15380 is intended for.

ericsnowcurrently · 2021-02-11T00:11:50Z

The right solution is to pass the relevant objects into the component init/activation, block in components where necessary, and eventually eliminate activateLegacy().

karrtikr · 2021-02-11T00:24:02Z

Yes eventually when we have everything refactored into components, #15380 can be thought of as an intermediately step. But either way, any solution we go with right now is a temporary solution, so I think this is okay.

ericsnowcurrently · 2021-02-11T01:06:56Z

With your change we are disabling activation of the component. If you move the activateLegacy() call then you shouldn't need to disable anything.

karrtikr · 2021-02-11T01:47:16Z

@ericsnowcurrently Please read #15379 (comment). The component adapter already activates it.

ericsnowcurrently · 2021-02-11T16:37:16Z

@ericsnowcurrently Please read #15379 (comment). The component adapter already activates it.

That isn't happening in the component adapter. It's code related to the old discovery machinery. I'd argue that the line in activationManager.ts should only be run if not in the experiment.

Regardless, that line is only pre-running env discovery. That part of the extension has no knowledge of what the envs component needs to do during activation. It only happens to be doing what the component does during activation. Instead, the component should be strictly in charge of its activation. We shouldn't short-circuit that design for the sake of any short-term objectives, else we'll end up back with the tangle of special cases and interdependence we're trying to eliminate with the component work.

ericsnowcurrently · 2021-02-11T16:40:22Z

We were activating the discovery component before we even register all the classes that the component might need,

Which classes does it rely on? (It seems that we've ended up with the component depending on the old DI framework, which is exactly what we were working to avoid in the component.)

karrtikr · 2021-02-11T18:16:07Z

Which classes does it rely on? (It seems that we've ended up with the component depending on the old DI framework, which is exactly what we were working to avoid in the component.)

It relies on IInterpreterService (which is actually not part of the old DI framework), it then calls the adapter based on if we're in discovery experiment, which is fine.

And you're right, I was looking at it short-term. I can and will change the code to call activateLegacy() first, but it still has its problems #15379 (comment) so it is short term too.

ericsnowcurrently

LGTM

Thanks for the changes. Ideally we would not have introduced the dependencies on the legacy code in the first place. However, at this point it's something to correct later. 😞

karrtikr · 2021-02-11T22:03:29Z

@ericsnowcurrently InterpreterService is not all legacy code tho, it queries legacy code based on if we're in experiment. Please have a look.

ericsnowcurrently · 2021-02-11T22:43:28Z

Sorry, I should have been more clear. If it isn't in src/client/pythonEnvironments then I consider it legacy code. (Plus, InterpreterService was added in 2018 and will be gone once we move everything else to components.)

Regardless, thanks for fixing this.

…he classes (microsoft#15379) * Do not attempt to activate discovery component before registering all the classes * Add clarification comment * Code reviews

* Do not call activate the discovery component before registering all the classes (#15379) * Do not attempt to activate discovery component before registering all the classes * Add clarification comment * Code reviews * Skip windows store and shims paths when using known path locators (#15388) * Skip windows store and shims paths when using known path locators * Clean up and comments * Tests * Handle cases where envs variables might not be set * Typo Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> * Change "Pylance not installed" prompt to allow reverting to Jedi (#15420) * Allow on suggestion refresh by default (#15430) Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Jake Bailey <[email protected]>

* Cherry pick fix for native notebook support into release branch (#15369) * Fix problem with notebook apis not being used. (#15366) * Update changelog * Remove news file * Add extraPaths support to JediLSP (#15365) * Add extraPaths support * Remove duplicate opt option * Eslint cleanup * Fix tests * Ignore directories with the same name as python binaries (#15367) * Ignore directories with the same name as pyhton binaries * Use withFileTypes instead of lstat * Correct pipenv activation provider when in discovery experiment (#15319) * Correct pipenv activation provider when in discovery experiment * Fix ESLint errors * In PythonEnvsReducer.resolveEnv(), always fall back to the wrapped locator. (#15350) fixes #15118 * Fix issue with missing interpreter info for some cases (#15376) * Use child process apis directly. * Use raw API in process service * Handle process cleanup * Address sonar * Refactor process service by pulling the raw process APIs out of the class * Address comments * Add reference to CVE-2020-16977 fixed in Oct 2020. (#15381) * Fix CI failing with the number of constructor arguments error (#15347) * Fix Command 'workbench.action.closeAllEditors' timed out failure on CI (#15322) * Ensure environment variables provider can be created using standard classes (#15377) * Ensure environment variables provider can be created using standard classes * Fix unit tests * Fix ESLint errors for environment variable provider (#15383) * Fix problem with notebook apis not being used. (#15366) * Show Python: Launch TensorBoard command in command palette (#15382) (#15386) * Always register Launch TensorBoard command * Put usage tracking behind experiment * Cherry picks from main to release (#15421) * Do not call activate the discovery component before registering all the classes (#15379) * Do not attempt to activate discovery component before registering all the classes * Add clarification comment * Code reviews * Skip windows store and shims paths when using known path locators (#15388) * Skip windows store and shims paths when using known path locators * Clean up and comments * Tests * Handle cases where envs variables might not be set * Typo Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> * Change "Pylance not installed" prompt to allow reverting to Jedi (#15420) * Allow on suggestion refresh by default (#15430) Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Jake Bailey <[email protected]> Co-authored-by: Rich Chiodo <[email protected]> Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Eric Snow <[email protected]> Co-authored-by: Jim Griesmer <[email protected]> Co-authored-by: Joyce Er <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Jake Bailey <[email protected]>

* Cherry pick fix for native notebook support into release branch (#15369) * Fix problem with notebook apis not being used. (#15366) * Update changelog * Remove news file * Add extraPaths support to JediLSP (#15365) * Add extraPaths support * Remove duplicate opt option * Eslint cleanup * Fix tests * Ignore directories with the same name as python binaries (#15367) * Ignore directories with the same name as pyhton binaries * Use withFileTypes instead of lstat * Correct pipenv activation provider when in discovery experiment (#15319) * Correct pipenv activation provider when in discovery experiment * Fix ESLint errors * In PythonEnvsReducer.resolveEnv(), always fall back to the wrapped locator. (#15350) fixes #15118 * Fix issue with missing interpreter info for some cases (#15376) * Use child process apis directly. * Use raw API in process service * Handle process cleanup * Address sonar * Refactor process service by pulling the raw process APIs out of the class * Address comments * Add reference to CVE-2020-16977 fixed in Oct 2020. (#15381) * Fix CI failing with the number of constructor arguments error (#15347) * Fix Command 'workbench.action.closeAllEditors' timed out failure on CI (#15322) * Ensure environment variables provider can be created using standard classes (#15377) * Ensure environment variables provider can be created using standard classes * Fix unit tests * Fix ESLint errors for environment variable provider (#15383) * Fix problem with notebook apis not being used. (#15366) * Show Python: Launch TensorBoard command in command palette (#15382) (#15386) * Always register Launch TensorBoard command * Put usage tracking behind experiment * Cherry picks from main to release (#15421) * Do not call activate the discovery component before registering all the classes (#15379) * Do not attempt to activate discovery component before registering all the classes * Add clarification comment * Code reviews * Skip windows store and shims paths when using known path locators (#15388) * Skip windows store and shims paths when using known path locators * Clean up and comments * Tests * Handle cases where envs variables might not be set * Typo Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> * Change "Pylance not installed" prompt to allow reverting to Jedi (#15420) * Allow on suggestion refresh by default (#15430) Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Jake Bailey <[email protected]> * Release final (#15433) * Version update * Change log updates * TPN update * Ensure pyenv virtual envs are not skipped when in discovery experiment (#15451) * Ensure pyenv virtual envs are not skipped * Add news * Clean up * Address comments * Register Jedi regardless of what language server is configured (#15452) * Register Jedi regardless of what language server is configured * News entry * Only call component adapter behind the discovery experiment (#15459) * Update version and change log for point release (#15463) * Version update * Update change log * Update start-up telemetry for Jedi LSP (#15419) (#15571) * Version and change log update for point release (#15572) Co-authored-by: Rich Chiodo <[email protected]> Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Eric Snow <[email protected]> Co-authored-by: Jim Griesmer <[email protected]> Co-authored-by: Joyce Er <[email protected]> Co-authored-by: Kim-Adeline Miguel <[email protected]> Co-authored-by: Jake Bailey <[email protected]>

karrtikr added no-changelog No news entry required skip tests Updates to tests unnecessary labels Feb 10, 2021

github-actions bot assigned karrtikr Feb 10, 2021

karrtikr changed the title ~~Revert "Only force background envs refresh if the component is enabled."~~ Do not call activate the discovery component before registering all the classes Feb 10, 2021

Do not attempt to activate discovery component before registering all…

58a5e11

… the classes

karrtikr force-pushed the revert-15132-pyenvs-component-activate-if-in-experiment branch from 24dd818 to 58a5e11 Compare February 10, 2021 21:33

karrtikr mentioned this pull request Feb 10, 2021

Only activate components after registering all the classes #15380

Open

karrtikr marked this pull request as ready for review February 10, 2021 21:45

github-actions bot requested review from ericsnowcurrently and karthiknadig February 10, 2021 21:59

Add clarification comment

11cafb9

karthiknadig approved these changes Feb 10, 2021

View reviewed changes

ericsnowcurrently reviewed Feb 10, 2021

View reviewed changes

karrtikr requested a review from ericsnowcurrently February 11, 2021 00:24

Code reviews

9e1e04f

karrtikr force-pushed the revert-15132-pyenvs-component-activate-if-in-experiment branch from fa345df to 9e1e04f Compare February 11, 2021 18:56

ericsnowcurrently approved these changes Feb 11, 2021

View reviewed changes

karrtikr merged commit 334a3bb into main Feb 11, 2021

karrtikr deleted the revert-15132-pyenvs-component-activate-if-in-experiment branch February 11, 2021 22:03

karrtikr restored the revert-15132-pyenvs-component-activate-if-in-experiment branch February 11, 2021 22:03

karrtikr deleted the revert-15132-pyenvs-component-activate-if-in-experiment branch February 11, 2021 22:04

Conversation

karrtikr commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karthiknadig commented Feb 10, 2021

Uh oh!

karrtikr commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karrtikr commented Feb 10, 2021

Uh oh!

karthiknadig commented Feb 10, 2021

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

karrtikr commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Feb 11, 2021

Uh oh!

karrtikr commented Feb 11, 2021

Uh oh!

ericsnowcurrently commented Feb 11, 2021

Uh oh!

karrtikr commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Feb 11, 2021

Uh oh!

ericsnowcurrently commented Feb 11, 2021

Uh oh!

karrtikr commented Feb 11, 2021

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

karrtikr commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karrtikr commented Feb 10, 2021 •

edited

Loading

karrtikr commented Feb 10, 2021 •

edited

Loading

karrtikr commented Feb 10, 2021 •

edited

Loading

karrtikr commented Feb 11, 2021 •

edited

Loading

karrtikr commented Feb 11, 2021 •

edited

Loading