Replace Jedi with Jedi LSP #17273

kimadeline · 2021-09-03T21:31:42Z

For #11995

What this PR does:

remove the JediLSP experiment
remove the JediLSP option under python.languageServer, it's all Jedi now
the Jedi option in the settings now refers to what used to be JediLSP
deleted Jedi tests that are failing now that old Jedi is out of commission
if on Python 2.7 and using the Default language server value -> fallback to Pylance
if on Python 2.7 and explicitly using Jedi as a language server -> turn the language server off

About the last 2 bullet points, I will add a notification that says "IntelliSense with Jedi for Python 2.7 is no longer supported. Learn more." in a separate PR.

src/client/activation/common/defaultlanguageServer.ts

kimadeline · 2021-09-07T22:33:47Z

src/client/common/configSettings.ts

            this.languageServerIsDefault = true;
+        } else if (userLS === 'JediLSP') {


@jakebailey did you mean this settings parser?

Yes, exactly; this is the "untrusted" part where we validate the user's settings before actually casting them to the enum.

jakebailey · 2021-09-07T22:36:05Z

src/client/common/configSettings.ts

@@ -293,8 +293,12 @@ export class PythonSettings implements IPythonSettings {
            userLS === 'Default' ||
            !Object.values(LanguageServerType).includes(userLS as LanguageServerType)
        ) {
-            this.languageServer = this.defaultLS?.defaultLSType ?? LanguageServerType.Jedi;
+            this.languageServer = this.defaultLS?.defaultLSType ?? LanguageServerType.None;


Trying to remember, but I believe that there was some test that relied on this staying Jedi, some weird case where there can't be a default LS present yet (since the default LS thing is possibly undefined). Maybe after removing old Jedi, this turns out fine.

kimadeline · 2021-09-09T21:48:41Z

src/client/activation/activationService.ts

+        // If the interpreter is Python 2 and the LS setting is explicitly set to Jedi, turn it off.
+        // If set to Default, use Pylance.
+        if (interpreter && (interpreter.version?.major ?? 0) < 3) {
+            if (serverType === LanguageServerType.Jedi) {


If I have the inner condition the other way around (if default -> node, else if jedi -> none) then the 2.7 debugger tests break 🤷

Reverting the condition can change the language server as the default LS can be Jedi as well, although not sure how intellisense affects debugger 😂

Reverting the condition can change the language server as the default LS can be Jedi as well

True!

although not sure how intellisense affects debugger 😂

The debugger tests spawn a VS Code instance, it's something about the extension activating and not finding Pylance (not sure why it causes an activation failure instead of displaying the "Pylance is not installed, do you want to install it" message though)

kimadeline · 2021-09-09T21:49:26Z

src/client/activation/activationService.ts

@@ -348,4 +345,23 @@ export class LanguageServerExtensionActivationService
        const values = await Promise.all([...this.cache.values()]);
        values.forEach((v) => (v.clearAnalysisCache ? v.clearAnalysisCache() : noop()));
    }
+
+    private updateLanguageServerSetting(resource: Resource): void {


Do you think there's a better spot where this could be done? It feels weird updating the settings.json file here.

We do similar this in testing too. But we have the code in a separate file. (See updateTestSettings.ts). I don't think we have a convention for this, and it seems ok to do it here.

Ideally we would have a settings manager and we would register this as a change provider with that.

I assume this is just for settings migration. We do have a diagnostics convention for stuff like this, ideally updateTestSettings.ts should be a diagnostic too. See

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

Line 237 in 6878880

lsNotSupportedDiagnosticService.handle(diagnostic).ignoreErrors();

for example.

I assume this is just for settings migration.

Yes, it replaces the JediLSP value of the python.languageServer setting with Jedi.

Looking at the LSNotSupportedDiagnosticService class it seems to only display an information message, and doesn't migrate settings?

I'm going to display the "Python 2.7 isn't supported" notification as a diagnostic, so I'm going to move the settings migration there in that PR.

Looking at the LSNotSupportedDiagnosticService class it seems to only display an information message, and doesn't migrate settings?

Yes, the point I was making was it could be used for migration.

vscode-python/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts

Line 136 in 48d66bd

await this.fixLaunchJson(diagnostic.code);

is probably a better example where we migrate.

I'm going to display the "Python 2.7 isn't supported" notification as a diagnostic, so I'm going to move the settings migration there in that PR.

Great!

karthiknadig · 2021-09-09T22:44:33Z

src/client/activation/activationService.ts

@@ -348,4 +345,23 @@ export class LanguageServerExtensionActivationService
        const values = await Promise.all([...this.cache.values()]);
        values.forEach((v) => (v.clearAnalysisCache ? v.clearAnalysisCache() : noop()));
    }
+
+    private updateLanguageServerSetting(resource: Resource): void {


We do similar this in testing too. But we have the code in a separate file. (See updateTestSettings.ts). I don't think we have a convention for this, and it seems ok to do it here.

Ideally we would have a settings manager and we would register this as a change provider with that.

jakebailey · 2021-09-10T18:22:06Z

src/client/activation/activationService.ts

+            return;
+        }
+
+        this.configurationService.updateSetting('languageServer', LanguageServerType.Jedi, resource, configTarget);


Is it a good idea to change this? It's no longer a valid enum value but is still accepted by the settings parser.

People may have checked in these settings, so I'd be wary of forcibly changing them to something else at startup, but maybe it won't be too many people.

We could leave it to show squiggles for a few releases and later remove it?

Removing it from package.json will effectively do this, so that's okay.

I don't have a strong preference; I know that Pylance has migration code for old MPLS settings, so I guess we're already accepting setting migrations. Have the same thing here already (though in that other test settings migrator thingy).

We do migrations like this all the time for launch.json, or other settings like python.unitTest to python.testing, so I don't think this should be a problem.

karrtikr

Mostly LGTM. Do you plan to remove completion.py and other Jedi related code later?

karrtikr · 2021-09-10T18:31:50Z

.github/workflows/pr-check.yml

@@ -100,6 +100,10 @@ jobs:
          # We need to have debugpy so that tests relying on it keep passing, but we don't need install_debugpy's logic in the test phase.
          python -m pip --disable-pip-version-check install -t ./pythonFiles/lib/python --no-cache-dir --implementation py --no-deps --upgrade --pre debugpy

+      - name: Install JediLSP requirements


Is pr-check.yml the only place where we need to do this?

There are 2 other actions that require Jedi LSP bits:

build.yml, which relies on the build-vsix action which already installs JediLSP requirements

nightly-coverage.yml, I'll add it there as well 👍

karrtikr · 2021-09-10T18:38:15Z

src/client/activation/activationService.ts

+        // If the interpreter is Python 2 and the LS setting is explicitly set to Jedi, turn it off.
+        // If set to Default, use Pylance.
+        if (interpreter && (interpreter.version?.major ?? 0) < 3) {
+            if (serverType === LanguageServerType.Jedi) {


Reverting the condition can change the language server as the default LS can be Jedi as well, although not sure how intellisense affects debugger 😂

karrtikr · 2021-09-10T18:49:52Z

src/client/activation/activationService.ts

@@ -348,4 +345,23 @@ export class LanguageServerExtensionActivationService
        const values = await Promise.all([...this.cache.values()]);
        values.forEach((v) => (v.clearAnalysisCache ? v.clearAnalysisCache() : noop()));
    }
+
+    private updateLanguageServerSetting(resource: Resource): void {


I assume this is just for settings migration. We do have a diagnostics convention for stuff like this, ideally updateTestSettings.ts should be a diagnostic too. See

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

Line 237 in 6878880

lsNotSupportedDiagnosticService.handle(diagnostic).ignoreErrors();

for example.

karrtikr · 2021-09-10T18:52:59Z

src/client/activation/activationService.ts

+            return;
+        }
+
+        this.configurationService.updateSetting('languageServer', LanguageServerType.Jedi, resource, configTarget);


We do migrations like this all the time for launch.json, or other settings like python.unitTest to python.testing, so I don't think this should be a problem.

kimadeline · 2021-09-10T19:02:47Z

@karrtikr

Do you plan to remove completion.py and other Jedi related code later?

Yep, there will be 2 follow-up PRs:

one to add the notification about 2.7 support
one to remove all the old Jedi code

Remove the experiment

b4424c1

kimadeline added the skip package*.json package.json and package-lock.json don't both need updating label Sep 3, 2021

kimadeline self-assigned this Sep 3, 2021

kimadeline added 2 commits September 3, 2021 14:33

News file

7993e17

Fix linting issue

560ea71

jakebailey reviewed Sep 7, 2021

View reviewed changes

src/client/activation/common/defaultlanguageServer.ts Show resolved Hide resolved

Turn JediLSP option into Jedi

68bb632

kimadeline commented Sep 7, 2021

View reviewed changes

jakebailey reviewed Sep 7, 2021

View reviewed changes

kimadeline added 21 commits September 7, 2021 16:10

Remove commented code

185d72e

Update comment

0c12ed3

Always register no LS activator

34ea029

Add basic 2.7 LS support (Pylance or None)

ca5dbff

Skip tests on Python 2.7

c48302d

Fix unit tests

0fd8455

Default to Pylance

4f5612c

Fix unit tests

61dad1c

Set default LS to Jedi

ec0db02

Install JediLSP requirements for testing

777680e

Change default back to none

9044986

Typo

6bf6a24

Turn condition around to match comment

4bafd38

Remove JediLSP requirements

7f5bb1b

Default to None

bd91ebe

Install JediLSP requirements

ac36ebd

Install JediLSP reqs for smoke tests too

df2841f

Merge branch 'main' into 11995-use-jedi-lsp-

f432385

Install JediLSP reqs in coverage step

efdab05

Undo PR check workflow changes

3733b58

Turns out it won't run if there's no 'run' keyword

d85d0a9

kimadeline added 6 commits September 9, 2021 09:36

Change condition order

fc85f2a

Add activation service unit tests

5418db5

Remove comment

010d3d4

Add NoLanguageServerExtensionActivator check

f38558f

Delete Jedi tests

b1f3819

Change condition order

54ca6b5

kimadeline commented Sep 9, 2021

View reviewed changes

kimadeline marked this pull request as ready for review September 9, 2021 22:22

kimadeline requested review from karthiknadig, karrtikr and jakebailey September 9, 2021 22:22

karthiknadig approved these changes Sep 9, 2021

View reviewed changes

jakebailey reviewed Sep 10, 2021

View reviewed changes

karrtikr reviewed Sep 10, 2021

View reviewed changes

Install Jedi LSP reqs in nightly coverage

5a3f849

kimadeline merged commit 862a88d into microsoft:main Sep 13, 2021

kimadeline deleted the 11995-use-jedi-lsp- branch September 13, 2021 23:56

kimadeline mentioned this pull request Sep 13, 2021

Display a notification when using JediLSP with Python 2.7 #17407

Merged

kimadeline added this to the September 2021 milestone Sep 13, 2021

kimadeline mentioned this pull request Sep 16, 2021

Remove Jedi 0.17 and ancillary projects #17225

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Jedi with Jedi LSP #17273

Replace Jedi with Jedi LSP #17273

kimadeline commented Sep 3, 2021 •

edited

Loading

kimadeline Sep 7, 2021

jakebailey Sep 7, 2021

jakebailey Sep 7, 2021 •

edited

Loading

kimadeline Sep 9, 2021

karrtikr Sep 10, 2021

kimadeline Sep 10, 2021

kimadeline Sep 9, 2021

karthiknadig Sep 9, 2021

karrtikr Sep 10, 2021

kimadeline Sep 10, 2021

kimadeline Sep 10, 2021

karrtikr Sep 10, 2021

karthiknadig Sep 9, 2021

jakebailey Sep 10, 2021

karthiknadig Sep 10, 2021

jakebailey Sep 10, 2021

karrtikr Sep 10, 2021

karrtikr left a comment

karrtikr Sep 10, 2021

kimadeline Sep 10, 2021

karrtikr Sep 10, 2021

karrtikr Sep 10, 2021

karrtikr Sep 10, 2021

kimadeline commented Sep 10, 2021

		this.languageServerIsDefault = true;
		} else if (userLS === 'JediLSP') {

Replace Jedi with Jedi LSP #17273

Replace Jedi with Jedi LSP #17273

Conversation

kimadeline commented Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karrtikr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimadeline commented Sep 10, 2021

kimadeline commented Sep 3, 2021 •

edited

Loading

jakebailey Sep 7, 2021 •

edited

Loading