Make it possible to use the LSP to talk to Jedi #13855

rchiodo · 2020-09-09T22:03:43Z

For #11995

This is the beginning of the work to switch to the Jedi LSP. It installs a fork of https://github.com/pappasam/jedi-language-server and sets up an experiment to control whether or not to use it.

Still left to do:

Get the tests to pass. About half of the language server tests fail when using the LSP. It's things like positions being off, markdown not matching, and signatures coming out different.
Remove pylint minimal checkers (see VS Code does not honor pylint pyproject.toml file under default settings #13321)

The goal here is to submit this and then split up the work of fixing the bugs in our fork and then push the changes the original pappasam main fork.

I forked it here: https://github.com/vscode-python/jedi-language-server

rchiodo · 2020-09-09T22:07:39Z

requirements.txt

-python-jsonrpc-server==0.2.0 \
-    --hash=sha256:59ce9c9523c14c493a327b3a27ee37464a36dc2b9d8ab485ecbcedd38840380a \
-    # via -r requirements.in
+click==7.1.2              # via jedi-language-server


Hashes don't seem to work with git hub dependencies. At least I haven't figured out how to make them work. Still looking into it.

DonJayamanne · 2020-09-09T22:10:56Z

package.json

                            "All"
                        ]
                    },
                    "description": "List of experiment to opt into. If empty, user is assigned the default experiment groups. See https://github.com/microsoft/vscode-python/wiki/Experiments for more details.",
-                    "scope": "machine"
+                    "scope": "resource"


These need to be per machine, we cannot have user opt into an experiment in one workspace & not the other.

This is required to get the tests to pass. Otherwise I have to do something like write the global settings.json or create an environment variable that overrides this in the actual source code.

Is this only a problem in multiple workspaces?

If the requirement is to get the tests to pass, then you can easily update this package.json during tests (that's what I do in native notebooks), i.e. in tests update the scope.
Else we're changing functionality of the product here, just for tests to pass, & i don' think that's the right thing.

Example:

User opts into experiment that deprecate pythonPath setting,

User opens another worksapce, now VS Code will complainin about python path not being setup

I.e. our code needs to be written in such a way that experiments are scoped to a workspace instance & not global. Today we assume that user is in experiment. We don't assume that an instance of vs code is in an experiment.

If we want to change this behavior thats fine, i'd leave this for core team to decide.

Okay I put it back. Test startup code writes the package.json now.

DonJayamanne · 2020-09-09T22:11:04Z

package.json

                            "All"
                        ]
                    },
                    "description": "List of experiment to opt out of. If empty, user is assigned the default experiment groups. See https://github.com/microsoft/vscode-python/wiki/Experiments for more details.",
-                    "scope": "machine"
+                    "scope": "resource"


See previous comment.

codecov-commenter · 2020-09-09T22:13:15Z

Codecov Report

Merging #13855 into main will decrease coverage by 0.21%.
The diff coverage is 31.07%.

@@            Coverage Diff             @@
##             main   #13855      +/-   ##
==========================================
- Coverage   60.03%   59.82%   -0.22%     
==========================================
  Files         685      691       +6     
  Lines       38086    38337     +251     
  Branches     5487     5515      +28     
==========================================
+ Hits        22866    22934      +68     
- Misses      14039    14219     +180     
- Partials     1181     1184       +3

Impacted Files	Coverage Δ
src/client/activation/common/cancellationUtils.ts	`13.95% <ø> (ø)`
...rc/client/activation/node/languageClientFactory.ts	`36.36% <ø> (ø)`
...rc/client/activation/jedi/multiplexingActivator.ts	`17.74% <17.74%> (ø)`
src/client/activation/jedi/languageServerProxy.ts	`27.27% <27.27%> (ø)`
src/client/activation/jedi/manager.ts	`30.30% <30.30%> (ø)`
...rc/client/activation/jedi/languageClientFactory.ts	`40.00% <40.00%> (ø)`
src/client/activation/serviceRegistry.ts	`77.27% <45.45%> (-5.01%)`	⬇️
src/client/activation/jedi/activator.ts	`70.00% <70.00%> (ø)`
src/client/activation/jedi/analysisOptions.ts	`71.42% <71.42%> (ø)`
src/client/activation/node/languageServerProxy.ts	`26.58% <100.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bcc807...74bf550. Read the comment docs.

DonJayamanne

I think the changes to the scope are leftovers of the tests

DonJayamanne · 2020-09-09T22:13:36Z

src/client/activation/jedi/languageClientFactory.ts

+        const jediServerModulePath = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'runJediLanguageServer.py');
+        const interpreter = await this.interpreterService.getActiveInterpreter(resource);
+        const pythonPath = interpreter ? interpreter.path : 'python';
+        const args = [`${jediServerModulePath}`];


Suggested change

const args = [`${jediServerModulePath}`];

const args = [jediServerModulePath];

Yes that was silly. I was original putting quotes around it, but not necessary.

DonJayamanne · 2020-09-09T22:15:14Z

tsconfig.json

-        "noFallthroughCasesInSwitch": true
+        "noFallthroughCasesInSwitch": true,
+        "resolveJsonModule": true,
+        "removeComments": true


Why are we removing the comments?

So I can import json with comments. Didn't think comments were necessary and it makes reading the settings.json super simple.

DonJayamanne · 2020-09-09T22:17:45Z

src/client/common/experiments/groups.ts

@@ -99,6 +99,10 @@ export enum SurveyAndInterpreterTipNotification {
    surveyExperiment = 'pythonMailingListPromptWording'
 }

+// Experiment to switch Jedi to use an LSP instead of direct providers
+export enum JediLSP {
+    experiment = 'jediLSP'


Do we need seprate Enums?
I'm not sure why we ended up creating 1 enum for each experiment.
Why not create an enum named Experiments and we always use that single enum. Then its strongly typed as well, rather than just accepting strings.

FYI - I know this is how the old stuff has been written, just never understand why, its more stuff that needs to be imported & the experiments are not strongly typed (as we just accept strings, & we need to dig around to find the right enum).

Yeah I'm not going to change it now. We can enter an issue if you want to refactor this stuff.

sonarqubecloud · 2020-09-10T16:44:34Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

rchiodo · 2020-09-10T17:35:09Z

@karthiknadig can you take a look when you get the chance? Thanks

karthiknadig · 2020-09-10T23:56:41Z

pythonFiles/runJediLanguageServer.py

+
+# Trick language server into thinking it started from 'jedi-language-server.exe'
+sys.argv[0] = "jedi-language-server.exe"
+sys.exit(cli())


We should file this as a bug. We should be able to run this python EXTENSION_ROOT/pythonFiles/lib/python the same way we run debugpy. We won't need this file.

I think this is particular to how we're installing it. If you pip install it, it generates an exe in your scripts folder that you would normally use (that exe has this exact code in it)

This script does not restrict running this only to windows. So argv[0] will be wrong for linux/mac. The problem is in the package itself. You should be able to just run via __main__.py in that package. This is a fairly small change, and it preservers existing functionality. Secondly, the package itself won;t require installation to run. since we don;t want to install this for each python environment.

Oh there is no __main__.py in the package. So maybe adding that would fix the problem?

We don't install this for every environment. We run it like we ran jedi. It ships as part of the VSIX.

Yep, it is a simple enough fix. We can take that later. For now lets get this working.

I know we don;t install for every environment. My point was that the package is designed in a way where it expects that. Hence you had to add this script to run it. But really it should not require installation nor should it require this script. The problem is with the packaging.

rchiodo added 13 commits September 3, 2020 15:58

Ship language server too

cd33bec

Remove all jedi code and make copy of LS activation

ef1f6e3

Compiling but not working

d669e5d

Merge remote-tracking branch 'origin/main' into rchiodo/jedi_lsp

70469aa

Merge remote-tracking branch 'origin/main' into rchiodo/jedi_lsp

6d5e4af

Working with intellisense

bd57161

Enable the experiment in order to switch

efbb500

Fix up registration so LSP works

c223500

Fix version reading

8c73e89

Write test wrapper to try out lsp

b11843f

Let experiments be in a workspace

289157a

Support installing jedi-language-server from custom fork

5b2b135

Merge remote-tracking branch 'origin/main' into rchiodo/jedi_lsp

fc07a0c

rchiodo added the no-changelog No news entry required label Sep 9, 2020

rchiodo requested review from brettcannon, IanMatthewHuff, ericsnowcurrently, DonJayamanne, karthiknadig, karrtikr and kimadeline September 9, 2020 22:03

rchiodo self-assigned this Sep 9, 2020

rchiodo commented Sep 9, 2020

View reviewed changes

DonJayamanne reviewed Sep 9, 2020

View reviewed changes

Fix sonar problems

9feeb13

DonJayamanne requested changes Sep 9, 2020

View reviewed changes

rchiodo added 2 commits September 9, 2020 15:32

More sonar fixes - dupe code

1a049a6

Put back the package json changes

82d9d2d

Skip dupe checking for language server proxy classes

d8e5674

rchiodo requested a review from DonJayamanne September 9, 2020 22:51

brettcannon removed their request for review September 10, 2020 16:19

DonJayamanne approved these changes Sep 10, 2020

View reviewed changes

Merge remote-tracking branch 'origin/main' into rchiodo/jedi_lsp

74bf550

karthiknadig reviewed Sep 10, 2020

View reviewed changes

karthiknadig approved these changes Sep 11, 2020

View reviewed changes

rchiodo merged commit ed3f29f into main Sep 11, 2020

rchiodo deleted the rchiodo/jedi_lsp branch September 11, 2020 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it possible to use the LSP to talk to Jedi #13855

Make it possible to use the LSP to talk to Jedi #13855

rchiodo commented Sep 9, 2020

rchiodo Sep 9, 2020

DonJayamanne Sep 9, 2020

rchiodo Sep 9, 2020

rchiodo Sep 9, 2020

rchiodo Sep 9, 2020

DonJayamanne Sep 9, 2020

DonJayamanne Sep 9, 2020

rchiodo Sep 9, 2020

DonJayamanne Sep 9, 2020

codecov-commenter commented Sep 9, 2020 •

edited

Loading

DonJayamanne left a comment

DonJayamanne Sep 9, 2020

rchiodo Sep 9, 2020

DonJayamanne Sep 9, 2020

rchiodo Sep 9, 2020

DonJayamanne Sep 9, 2020

DonJayamanne Sep 9, 2020

rchiodo Sep 9, 2020

sonarqubecloud bot commented Sep 10, 2020

rchiodo commented Sep 10, 2020

karthiknadig Sep 10, 2020

rchiodo Sep 10, 2020

karthiknadig Sep 11, 2020

rchiodo Sep 11, 2020

rchiodo Sep 11, 2020

karthiknadig Sep 11, 2020

karthiknadig Sep 11, 2020

	const args = [`${jediServerModulePath}`];
	const args = [jediServerModulePath];

Make it possible to use the LSP to talk to Jedi #13855

Make it possible to use the LSP to talk to Jedi #13855

Conversation

rchiodo commented Sep 9, 2020

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

codecov-commenter commented Sep 9, 2020 • edited Loading

Codecov Report

DonJayamanne 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 10, 2020

rchiodo commented Sep 10, 2020

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

codecov-commenter commented Sep 9, 2020 •

edited

Loading