Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Provide an option to disable specific stubs in settings #840

Closed
southwood opened this issue Jan 16, 2021 · 28 comments
Closed

Provide an option to disable specific stubs in settings #840

southwood opened this issue Jan 16, 2021 · 28 comments
Labels
enhancement New feature or request

Comments

@southwood
Copy link

This is a great project - very useful and much faster than other language servers.

However, I've noticed that the stubs aren't accurate for a lot of things. I would much rather navigate to the source definitions than see misleading stub interfaces. It's always edge-cases. Most of the stubs are correct. It took me longer than it should have to realize that my local language server had changed, and that pylance was ignoring the actual source files. Reminds me of the early days of tsds and DefinitelyTyped with reverse engineered type definitions.

Please add an exclusion list for stubs so that developers have a way to fall-back to source definitions other than switching the language server.

The best work-around I have found so far is to toggle "justMyCode": false, in the launch config and step-into code where the pylance stubs are misleading. Before VSCode started using pylance it was possible to navigate into the source definition of installed packages. This feature is much more useful than navigating into a stub definition even when the stubs are correct.

As a corollary, I suggest that the stubs not be used at all when viewing the source definition of an inferred function or class. Specifically with the f12 hot-key or right-click navigation of "Go To Definition".

@jakebailey
Copy link
Member

jakebailey commented Jan 16, 2021

Rather than jumping to a specific solution, I'd like to better understand the problems you're having first; we fully intend that things will work with the stubs present, and anything past that indicates that there are bugs to solve rather than just disabling stubs (which will only make things worse overall; inferencing is not going to go well, and comparing it to MPLS is a bit tough because it made a number of questionable inferencing decisions).

It seems like you're saying a set of stubs we bundle is incorrect in some way. Which stubs are incorrect? Are you trying to do type checking, or just use Pylance in its default state? We'd really be interested in making sure those stubs are fixed; we have our own stubs but source some from typeshed and other repos.

You mention navigation; we intend for navigation to work, just that stubs may appear as well. There are some oddities in how VS Code shows this, often putting preselecting the wrong file. There have been requests to allow disabling going to stubs at all (#65), and we are working on modifying our behavior to improve navigation and add customization. What behavior are you seeing? Do you have an example? Are you seeing both locations, or just one and it's the stub?

@jakebailey jakebailey added the waiting for user response Requires more information from user label Jan 16, 2021
@github-actions github-actions bot removed the triage label Jan 16, 2021
@southwood
Copy link
Author

I see, thank you for the reply and clarifications.

I first noticed here with Django's AsyncClient:

But, a number of things seemed broken. I'll take some notes next time I'm working with it.

I still think you should consider the feature to disable stubs for select packages. Thinking back to the early days of typescript and community sourced definitions in DefinitelyTyped, it was around 50% chance that tsds would pay-off vs causing more trouble than they averted for any given dependency.

My current workaround of disabling pylance isn't pretty but it's functionally more effective than disabling justMyCode and stepping into functions with a unit test each time I need to inspect something (which, is also often more effective than looking up the sources online or grep'ing for each one).

My ideal solution would allow me to leave pylance enabled in general for all dependencies (because it is fantastic, and much faster than jedi) except a specific deny-list if I suspect something isn't accurate. Once finished I'd remove everything from the deny-list and hope that the next time I sync'd things would have improved in the stubs. I'm guessing the number of packages covered by stubs will increase in the coming weeks / months, so this may become more important if there is any chance of inaccuracy.

@jakebailey jakebailey added needs decision Do we want this enhancement? and removed waiting for user response Requires more information from user labels Feb 17, 2021
@Xalaxis
Copy link

Xalaxis commented Feb 19, 2021

I have an issue that may be related to this? This is unfortunately mostly outside my understanding, so apologies if this is in the wrong place.

I'm using a beta version of SQLAlchemy, but as best I can tell PyLance is using some bundled stubs which mean that the new imports show as missing (as it only refers to the released version, even when a newer version is installed via pip). Would a setting like this allow me to tell PyLance to look at the installed packages rather than use the pre-shipped definitions?

@jakebailey
Copy link
Member

The way things are ordered, our bundled stubs come last. I assume SQLAlchemy remains untyped, so we use the stubs we bundled. So yes, some configuration to do this would allow you to bypass these stubs.

For now, a workaround is to delete them from the Pylance distribution.

@Xalaxis
Copy link

Xalaxis commented Feb 19, 2021

Thanks! Deleting the types (from .vscode\extensions\ms-python.vscode-pylance-2021.2.3\dist\bundled\stubs\sqlalchemy) has fixed the ghosts telling me functions don't exist 😃 . I would support this feature to provide a way to ignore them in the configuration/GUI.

@savannahostrowski savannahostrowski added the enhancement New feature or request label Jun 8, 2021
@strogonoff
Copy link

Any pre-bundled stubs should be opt-in, or the extension can simply ask the user to install stubs.

The golden rule is to be explicit, not implicit. I’m OK to be warned that typings are missing, and can install the requisite typings as I normally do. I’m not OK with IDE sneaking in code from third parties not vetted by core maintainers of the libraries I trust, and resulting typing mismatches.

@zzzeek
Copy link

zzzeek commented Dec 23, 2021

SQLAlchemy maintainer here.

My 2c is that in SQLAlchemy 2.0 we hope (emphasis on hope...) to have the entire library typed within the source code. so to the degree that pylance will automatically use the .py file that has typing information vs. what it has in a stub, we're good, but I dont know the mechanics of that. However, if pylance still keeps consulting the stubs and confusing things, then we want to give our users instructions on how to use SQLAlchemy 2.0 and make sure no obsolete stubs are being pulled in from their IDE.

@erictraut
Copy link
Contributor

@zzzeek, thanks for posting. It's great to hear that SQLAlchemy 2.0 might have full and complete inlined type information! I'm very encouraged and excited to hear that. Fingers crossed that you can make that happen. Let us know if there's anything we (the pylance and pyright maintainers) can do to help. (One thing that you might find useful is the "--verifytypes" option provided by the command-line version of pyright. It allows you to verify whether any public symbols in a py.typed library are missing type information.)

We release pylance every week (except for the next few weeks when we're on a hiatus for the holidays). We can therefore respond quickly once you have the SQLAlchemy 2.0 plans finalized. It's in everyone's best interest that pylance and SQLAlchemy work great together. Let's collaborate to figure out the best answer. Do you have a rough timeline for SQLAlchemy 2.0?

@CaselIT
Copy link

CaselIT commented Dec 23, 2021

Hi @erictraut, another SQLAlchemy maintainer here, thanks for the offer!

I've started some work on typing by trying to convert some the decorators, but I think we encountered a bug with ParamSpec . I've tagged you to the issue in pyright

@CaselIT
Copy link

CaselIT commented Dec 30, 2021

Let us know if there's anything we (the pylance and pyright maintainers) can do to help

Having the ability of listing a series of package for which to ignore any stub would be really helpful to develop for a package like sqlalchemy for which the are bundled stub and there may also be stubs installed from pypi.

Tracking down all the possible install location to remove the stubs from both the vscode folder, the npm/yarn/npx pyright install location etc is really tedious, and it's easy to miss a location or an update.

Ideally this would be a pyright configuration we can add to pyproject. something like ignore_stubs = ['sqlalchemy', '...']

I understand that this is use case is somewhat limited to library author for which there are bundled stubs, but you asked :)

@erictraut
Copy link
Contributor

I'd prefer to find a solution that's more automatic than a manual configuration override. Pylance has millions of users, so asking each of them to do manual configuration to get the correct behaviors is far from ideal. We should try to find a solution that "just works" for the vast majority of pylance & sqlalchemy users.

PEP 561 unfortunately allows installed stub packages to override the types provided inline by a "py.typed" library even if the stub package isn't marked "partial". I don't recall this ever being an issue prior to this, but it will be a problem for sqlalchemy since there are existing stub packages on pypi, and you're now adding inlined types.

One option is to effectively deprecate the pypi stub package by publishing a new version that is marked as "partial" and removing all modules from it. That way, if someone installs the latest version of the stub package along with the new "py.typed" version of sqlalchemy, they'll work fine together. That doesn't address the situation where users have an older version of the stub package installed and then install the "py.typed" version of sqlalchemy.

Another option is for pylance to detect when a stub package is present along with a corresponding "py.typed" package. It could follow PEP 561 precedence but emit a warning to recommend uninstalling the stub package. This isn't fully automatic, but it would be much more discoverable than a hidden configuration option.

I'll discuss this more with my colleagues once everyone is back from holiday vacations. Maybe they can come up with additional options.

@CaselIT
Copy link

CaselIT commented Dec 31, 2021

I agree that an automatic solution would be the best. My suggestion of a configuration option was not for general usage but targeted just to library developers.

Ideally once a library is updated no config option is needed.

Thanks for taking the time of considering it!

@zzzeek
Copy link

zzzeek commented Jan 9, 2022

One option is to effectively deprecate the pypi stub package by publishing a new version that is marked as "partial" and removing all modules from it. That way, if someone installs the latest version of the stub package along with the new "py.typed" version of sqlalchemy, they'll work fine together. That doesn't address the situation where users have an older version of the stub package installed and then install the "py.typed" version of sqlalchemy.

it seems very likely we will have to do something like this.

My impression is that this would not as much be a new version of sqlalchemy2-stubs, because im not aware of any way for the package to be "linked" only to SQLAlchemy 2.0; that is, if we published it to pypi, users of sqlalchemy 1.4 would get it also and their typing would then be completely broken (if you can confirm there's no mechanism I'm not aware of here to handle this problem).

so assuming that, it seems like it would be straightforward to release a package "sqlalchemy-null-stubs", the purpose of which is to replace any existing stubs packages, by providing a blank package that is nonetheless importable under "sqlalchemy-stubs", which IIUC is the magic name that all the type checkers look for. At least with such a package, we only have to put it up once and then it's there for all future releases.

Questions remain then what is the structure of this package:

  1. for it to be installed at all, I would assume it at least needs to have an __init__.pyi file present?
  2. if this file is present, and we assume it's blank, does that cause the typing annotations in sqlalchemy/__init__.py to be skipped by type checkers or does this work? otherwise how do we compose this blank stubs package.
  3. can we otherwise assume the presence of a blank sqlalchemy-stubs package will instruct correctly implemented type checkers to honor all annotations in all files within the main sqlalchemy package?
  4. the presence of a blank sqlalchemy-stubs package will definitely mean that none of the pre-installed stubs get used? e.g. .vscode/extensions/ms-python.vscode-pylance-2021.12.3-pre.1/dist/bundled/stubs/sqlalchemy, .npm/_npx/<number>/node_modules/pyright/dist/typeshed-fallback/stubs/SQLAlchemy/sqlalchemy etc? just fyi these files are really hard to find.

@erictraut
Copy link
Contributor

You wouldn't want to include an __init__.pyi file because it will override the real __init__.py. I don't think you need to include any .pyi file in the null stub package. Just a "py.typed" file that includes "partial".

This will all require testing across the various type checkers because PEP 561 isn't sufficiently clear about how this all works, and type checker authors have struggled to get consistent behavior here.

@CaselIT
Copy link

CaselIT commented Jan 9, 2022

My impression is that this would not as much be a new version of sqlalchemy2-stubs, because im not aware of any way for the package to be "linked" only to SQLAlchemy 2.0; that is, if we published it to pypi, users of sqlalchemy 1.4 would get it also and their typing would then be completely broken (if you can confirm there's no mechanism I'm not aware of here to handle this problem).

I think this is possible. We could provide a version on pypi, (let's call it 2.0) that has sqlalchemy >= 2 in the requirements.
Then we can continue to publish other non-empty version of the stub using sqlalchemy < 2 as requirements.
Pip should understand these constraints, and newer version of pip will also report if a non compatible version is installed

@zzzeek
Copy link

zzzeek commented Jan 9, 2022

OK it works from a pypi perspective, but what does pylance / pyright do? from what I can see, there is only one stubs package being shipped. they would need to not package the latest version of sqlalchemy2-stubs

@zzzeek
Copy link

zzzeek commented Jan 9, 2022

also, someone has sqlalchemy 1.4 installed. then they say, "pip install sqlalchemy2-stubs --upgrade". isnt that going to bump their SQLAlchemy to 2.0 ? id rather not have it work that way.

@erictraut
Copy link
Contributor

Pyright doesn't ship any sqlalchemy stubs. Pylance does include sqlalchemy stubs, but we ship a new version of pylance every week and can make whatever changes are necessary to make it work well with sqlalchemy 2.0.

@CaselIT
Copy link

CaselIT commented Jan 10, 2022

isnt that going to bump their SQLAlchemy to 2.0 ? id rather not have it work that way.

I think so, but pip will then print an error because of incompatible dependencies

@zzzeek
Copy link

zzzeek commented Jan 10, 2022

OK sorry to fill up your issue tracker, this discussion is moved to sqlalchemy/sqlalchemy#7555. thanks for your help @erictraut !

@zzzeek
Copy link

zzzeek commented Jan 10, 2022

Pyright doesn't ship any sqlalchemy stubs. Pylance does include sqlalchemy stubs, but we ship a new version of pylance every week and can make whatever changes are necessary to make it work well with sqlalchemy 2.0.

@erictraut can I assume pyright is downloading the stubs from https://github.com/python/typeshed/tree/master/stubs each time you run it, something like that? that is, our goal would be to block / replace the stubs that come from typeshed.

@erictraut
Copy link
Contributor

Pyright bundles the stubs from typeshed. It doesn't download them dynamically. I update the bundled stubs from typeshed every few weeks, so any change in typeshed will be quickly reflected in pyright. But these shouldn't be an issue anyway because typeshed stubs are resolved last in PEP 561 ordering.

@zzzeek
Copy link

zzzeek commented Jan 14, 2022

I just tried the approach of implementing a null stubs package, with py.typed containing the word "partial". Then I went into pylance setup and set the path to these stubs in "stubs-path". The approach still does not work, until I did a search in my .vscode directory for all SQLAlchemy stubs and deleted them (there were four separate copies of it).

the reason seems clear from the spec:

For the benefit of type checking and code editors, packages can be "partial". This means modules not found in the stub package SHOULD be searched for in parts four and five of the module resolution order above, namely inline packages and typeshed.

so just having a single py.typed file isn't going to work. We would need to either provide a stub package that features a blank .pyi file for every module in SQLAlchemy (not really feasible), or we somehow get our "blank" stubs into typeshed, or pylance includes some option to de-select certain typeshed stubs from being used.

I remain unclear on how to put multiple versions of stubs in typeshed that are dramatically different based on the version of the main library that's in use.

@erictraut
Copy link
Contributor

We (the pylance team) talked about this internally. Perhaps we're overthinking this. Maybe it's sufficient to fix the bundled stubs and then tell users to uninstall any packaged stubs that they have within their Python environment. There will be a short period of time where there's some confusion, but it will clear up over time. If it proves to be a big problem, pyright could detect the incompatible stub package and present the user with a warning and suggest that they uninstall it.

@zzzeek
Copy link

zzzeek commented Jan 14, 2022

We (the pylance team) talked about this internally. Perhaps we're overthinking this. Maybe it's sufficient to fix the bundled stubs and then tell users to uninstall any packaged stubs that they have within their Python environment. There will be a short period of time where there's some confusion, but it will clear up over time. If it proves to be a big problem, pyright could detect the incompatible stub package and present the user with a warning and suggest that they uninstall it.

I just asked the typeshed folks over at python/typing#1022 and it seems once SQLAlchemy 2.0 is up and has inline typing, typeshed will remove sqlalchemy stubs from it's current repo. so this would mean pyright/pylance would no longer include it either in new releases and that would be it.

so it seems the only limiting factor now is how quickly typeshed will make this change once we release SQLAlchemy 2.0.

@erictraut
Copy link
Contributor

We bundle the typeshed stubs with pyright/pylance. I typically incorporate the latest versions from typeshed ever two weeks, but I can do it on demand. So if it becomes necessary to delete the SQLAlchemy typeshed stubs quickly, we can take care of that. The typeshed maintainers are also very responsive, so it usually takes less than 24 hours to get a PR reviewed and merged.

@zzzeek
Copy link

zzzeek commented Jan 14, 2022

OK let's go with that then. seems like it will work out.

@judej judej removed the needs decision Do we want this enhancement? label Apr 19, 2022
@judej
Copy link
Contributor

judej commented Apr 19, 2022

Moving this issue to discussion as an enhancement request for comments and upvotes.

@microsoft microsoft locked and limited conversation to collaborators Apr 19, 2022
@judej judej converted this issue into discussion #2663 Apr 19, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants