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

Enhanced synchronisation of type checking #951

Closed
colinRawlings opened this issue Aug 17, 2020 · 20 comments
Closed

Enhanced synchronisation of type checking #951

colinRawlings opened this issue Aug 17, 2020 · 20 comments
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request

Comments

@colinRawlings
Copy link

With pyright 1.1.63 (freshly installed with vs code restarted post install to be on the safe side) I am still seeing more limited sychronisation between pyright and the source code than I believe was previously achieved.

In the following screen grab taken from a "dummy" project involving flask I have created a venv and installed flask and my sophisticated flask_toy application into this venv.

My vs code is setup with:
"python.languageServer": "Microsoft",
in the settings.json.

In my scenario I have a run.py (upper editor frame) in which I call create_app from within the flask_toy.app module. At the point of the screen grab I have just finished a refactoring converting the argument type of create_app from a str to an int.

At the point of the screen grab I am hovering over the create_app callable in run.py:

IMG_4301

It can be seen that both the Microsoft language server and the pyright language server are contributing a definition of the symbol. The upper Microsoft one is up to date (showing int). The lower I believe from pyright is out of date (it still shows the old str input type). Furthermore a red squiggle is present which alerts on hover that I am calling create_app with an int and not a string.

I can get pyright back in sync with the source code by calling Pyright: Restart Server in the command palette (i.e. both show int as the input type and the red squiggle under the 1 goes away).

It does not appear to be that pyright is simply slower. At least over 10 minutes or so it does not seem to get back in sync with the source code. Furthermore usages within the same file stay rigorously in sync. That is to say the red squiggle around usages of create_app in app.py seem to be more or less immediately updated to reflect changes in the function definition. Meanwhile identical usages outside (e.g. in run.py) of the file can have different red squiggle states with respect to their twin within app.py. Finally while I focus on the type of input variables the synchronisation issue seems to be quite general (e.g. return type, callable name).

Describe the solution you'd like
A typical development scenario for me is a repo containing several python packages which are editably installed into the venv. These are developed in tandem (I freely accept this is bad practice). Manually syncing pyright is miles better than not having it available but it would be nice if this was unncessary. My impression is that despite the fix in the 1.1.63 release something is still awry with the file watching.

@colinRawlings colinRawlings added the enhancement request New feature or request label Aug 17, 2020
@erictraut
Copy link
Collaborator

When you editably install, I assume pip installs the local files into your venv. At that point, you have two copies. Which one are you editing? The original (which is in your workspace) or the copy (which is in your venv)? Or are you re-installing the package every time you make an edit?

@jakebailey
Copy link
Member

jakebailey commented Aug 18, 2020

Editable installs use pth files, which add paths to sys.path directly when the site package loads (which happens pre-execution of code without the -S flag). No copying or symlinks are used.

Since we run python to ask for sys.path, they should be appearing in the lists of paths and should end up being equivalent to extraPaths (but automatic).

@jakebailey
Copy link
Member

@colinRawlings Do you have this as a project somewhere we could check out?

(AFAIK we haven't thoroughly tested editable installs as much as they were tested in MPLS: microsoft/pylance-release#78)

@colinRawlings
Copy link
Author

colinRawlings commented Aug 18, 2020

Hi guys

I uploaded a toy project showing the workflow here: https://github.com/colinRawlings/demo_for_pyright

@erictraut as @jakebailey says the -e option avoids the duplication of code which otherwise would hinder development.

On my machine I can reproduce the lack of sychronisation by opening the app.py and run.py then editing the expected argument type on app.py: line 3. On app,py: line 18 I see a type error "immediately" where as I see an error on run.py: line 5 only after restarting the pyright server.

@colinRawlings
Copy link
Author

Ok last comment from me which is in danger of muddying the waters ...

I installed Pylance 2020.8.1 (and uninstall pyright and set the language server to pylance and restarted).

This setup exhibits similar behaviour. Running the same change of argument type int -> str causes in-file an immediate type error warning (app.py: line 18) where as the red squiggle appears in run.py: line 5 only after calling Python: Restart Language Server. Interestingly the context help appearing when you hover over the call to create_app in run.py is updated more or less immediately (i.e. it shows an expected input type of str). However the red squiggle is missing beneath the Literal[1]. Hence Pylance is inconsistent with itself.

I add this note here because (correct me if I am wrong) Pylance and Pyright share some very similar DNA so maybe this helps to understand the behaviour.

@erictraut
Copy link
Collaborator

Thanks for the details.

Here's a follow-up question for you. I can understand why pyright wouldn't detect changes at the time you perform the editable install because you're effectively changing the import search paths of the python interpreter behind pyright's back. However, once you do this and restart pyright, it should pick up any changes you subsequently make to the source files that are part of the editable install. It won't detect the changes as you type, but as soon as you save the file, it should trigger the file system watcher and trigger a reanalysis. Is this consistent with the behavior you're seeing? If so, then I think this is the intended behavior.

@erictraut erictraut added the question Further information is requested label Aug 19, 2020
@colinRawlings
Copy link
Author

Hi Eric,

the behaviour is still present if you restart after performing the (editable) install. After a restart it is not sufficient to save the file to trigger pyright to update. It did not used to be necessary to save the file nor is it necessary for the Microsoft language server (I guess because they listen for onEdited events from the editor).

To show the behaviour I recorded the following gif. In it you can see that:

  • Within the edited file pyright updates annotation without needing the file to be saved (circular blob on the editor tab) 👍
  • Saving the source file does not result in an update of pyright's annotation. 👎
  • The "Microsoft" language server updates both file's annotation without needing the file to be saved. 👍
  • Restarting the server causes pyright's annotation to become correct. 👍

demo2

@erictraut erictraut removed the question Further information is requested label Aug 19, 2020
@erictraut
Copy link
Collaborator

I've tried to replicate your setup as closely as possible, and I'm not able to repro the problem you're describing. For me, changes that I make to the installed package files immediately trigger a reanalysis, and results are almost instantaneous even without saving the file. I needed to restart the Pyright Language Server once after doing the editable install, but after that it worked great. I'm using Python 3.8 in a venv. I'm running the latest Pyright VS Code extension (1.1.64).

In the screenshot below, you can see that when I update the contents of the app.py file, the run.py file is immediately updated with corresponding type errors.

Any thoughts on what I'm doing differently from you?

Aug-19-2020 09-09-49

@erictraut erictraut added the question Further information is requested label Aug 19, 2020
@colinRawlings
Copy link
Author

Hi Eric,

Your approach looks identical to mine and the behavior you experience looks perfect. Interestingly I was able to reproduce on both my work pc and also my home PC. I am using python 3.6. It’s possible that it is related to my pyright Extension settings. I will try resetting everything to default. Unfortunately I am on vacation Friday-Sunday and so cannot check until Sunday evening/Monday.

@colinRawlings
Copy link
Author

I tried (on my home pc):

  • Deleting all user settings
  • Deleting all workspace settings except the pythonPath
  • Installing py 38

The behaviour was identical to that shown in my gif. A colleague with whom I share an office can also reproduce the problem on his development machine.

Tonight I will have a go at completely removing all vs code related files (extensions, language server databases, ... ) and try again to reproduce. I'll let you know how I get on.

@erictraut
Copy link
Collaborator

Thanks for the update. It's odd that we're seeing different behavior. We have a mystery on our hands.

Are you using multi-root workspaces in VS Code? You didn't mention that above, so I'm guessing the answer is no, but I figured I'd ask.

@colinRawlings
Copy link
Author

colinRawlings commented Aug 24, 2020

No multi root workspaces (had some issues although colleagues report it now works well). Tonight I can also set the pyright logging to trace and provide you with this as well ...

@colinRawlings
Copy link
Author

Removing all installed extensions (except python and pyright) did not alter the behaviour. As this point I am somewhat out of ideas.

@erictraut
Copy link
Collaborator

The only other difference that I can think of is that you're using Windows, and I'm on a Mac. That shouldn't matter here, but it's worth a try.

@jakebailey, could you please try to repro this problem on your Windows machine?

@jakebailey
Copy link
Member

I also can't seem to reproduce this using Pylance, using almost the steps (the makefile didn't work for me as it assumes the existence of py which isn't true on my Windows machine, so I did the steps by hand). I'm not running MPLS at the same time (that's a bad idea), but when I change that annotation from int to str I can see that the type updates in the other editor immediately.

One thing to consider is that in your settings.json file, you've set pythonPath and used one of the Python extension's variables, which isn't generically supported outside of the extension itself (including pylance/pyright). If you switch that to just venv/Scripts/python.exe or remove it entirely and use the Python extensions pythonPath-less mode, does it work?

I think some logs so we can see which interpreter is in use and what files are being modified and updated would be helpful; logLevel set to Trace should do it.

If you're going to run Pyright and not Pylance with the Python extension, I would recommend setting the python.languageServer setting to "None" to make it more obvious what is going on in some of these screenshots and the logs.

@jakebailey
Copy link
Member

I do think we need to add a bit of extra logging to list out the paths we use, as we begin to examine cases like these where the search paths and their classifications are important to know.

@erictraut
Copy link
Collaborator

There is already additional logging information conditionally output by Pyright if the "verboseOutput" config option is used. We should probably unify that better with the "python.analysis.logLevel" setting.

@colinRawlings
Copy link
Author

Hi guys sorry for the slow reply. I recorded another gif this time with only Pylance active to avoid any negative interaction between the MPLS and Pyright. I was working with py38. I also commented out the "python.pythonPath" (see the end of the gif for the content of my settings.json). Actually I was suprised that this didn't cause the python extension to assume I meant the python interpreter at C:/Python38.python.exe. I also set the "python.analysis.logLevel" to "Trace" here is the log:
log.txt

Just to spice things up I wrote a doc string for create_app. Interestingly this along with the correct type error red squiggle only appear after restarting the language server in run.py:

demo2

@erictraut
Copy link
Collaborator

Thanks for posting the animated gif. I think I see what's happening. It looks like there is a case difference in paths. For some of the paths, "Desktop" is uppercase, and in others "desktop" is lowercase.

Pyright uses case-sensitive paths internally when determining whether two paths point to the same source file. In your case, the "run.py" knows that there's a dependency on the file c:\users\colin\Desktop\demo_for_pyright\flask_toy\flask_toy\app.py, but you are modifying c:\users\colin\desktop\demo_for_pyright\flask_toy\flask_toy\app.py in the upper window of the editor. I'm guessing that the Windows version of pip is adding a lowercase version of the path to the sys.path when you do an editable install.

I'll need to think about this a bit more, but I think the correct fix is for Pyright to do either case-sensitive or case-insensitive path comparisons based on the platform. There are a number of cases where we do these comparisons in various places in the code, so it's going to be challenging to find them all.

@erictraut erictraut added addressed in next version Issue is fixed and will appear in next published version and removed question Further information is requested labels Sep 6, 2020
@erictraut
Copy link
Collaborator

This is now fixed in Pyright 1.1.69, which I just published. It will be in the next Pylance release as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants