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

Release 9.0 breaks mypy (and pylint etc.) #940

Closed
vertti opened this issue May 4, 2021 · 30 comments
Closed

Release 9.0 breaks mypy (and pylint etc.) #940

vertti opened this issue May 4, 2021 · 30 comments
Labels

Comments

@vertti
Copy link

vertti commented May 4, 2021

Running a project that uses websockets 9.0 against mypy, throws:

error: Module has no attribute "connect"

for

import websockets
await websockects.connect(uri)

and

Module 'websockets' has no attribute 'connect'

for

from websockets import connect

Version 8.1 of websockets has no such issue. Using the latest mypy 0.812.

@apljungquist
Copy link

I expect this is caused by the new lazy import system. Another consequence of this is that trying to jump to definitions no longer works (one always lands in the package dunder init file) which is unfortunate since the ability to quickly inspect third party code is one of my favourite features of python.

@aaugustin
Copy link
Member

Oh, the joy.

@aaugustin aaugustin added the bug label May 5, 2021
@gmjosack
Copy link

gmjosack commented May 6, 2021

The lazy importer also breaks pylance, similar to the error for mypy, as well as pyinstaller as it fails to include the correct modules in the distribution.

as a work around I'm currently using

from websockets.legacy.client import connect as ws_connect

which resolved both issues.

@aaugustin
Copy link
Member

This is the pragmatic workaround at this point. I hate that it exposes the .legacy package before the new package is ready, but I'm not coming up with something better.

https://stackoverflow.com/questions/60739889/bypass-mypys-module-has-no-attribute-on-dynamic-attribute-setting has a good discussion of the options and they're all bad :-(

@aaugustin
Copy link
Member

There are several options.

Option 1 - do nothing

"If you want to use mypy, use the full import paths, not the convenience imports from the root module."

If it weren't for the temporary .legacy package, this would likely be my favorite solutions.

Option 1bis - hide .legacy

Like option 1, but with a few imports added, like from .legacy.client import connect in websockets.client, etc. This would ensure import paths are clean.

Type checking would still fail for deprecated APIs, though: I can't bypass the lazy import system because it's also taking care of raising deprecation warnings.

I think this is my favorite option.

Option 2 - provide type stubs

The primary driver for the lazy import system is easier refactoring. Having to maintain type stubs would hinder that goal.

@aaugustin
Copy link
Member

Could you report whether a7b2400 works?

Supported uses should include:

import websockets.client
await websockects.client.connect(uri)

and

from websockets.client import connect
await connect(uri)

(This will have to be a bugfix release on top on 9.0.1, but I never did maintenance branches in websockets, so this isn't a request.)

@ltworf
Copy link

ltworf commented May 8, 2021

You could have at least put a changelog file to say that the api was changing.

Also semver would be nice.

@ltworf
Copy link

ltworf commented May 8, 2021

In any case, changing the import seems to work but doesn't satisfy mypy.

slackclient/client.py:63: error: Name 'websockets.client.WebSocketClientProtocol' is not defined

@gmjosack
Copy link

gmjosack commented May 8, 2021

You could have at least put a changelog file to say that the api was changing.

Also semver would be nice.

Didn't it change between 8.x and 9.x? That sounds like semver to me.

Anyways, it seems to me this was not an api change but just a change to the way imports work. The side effect of making type checking break was obviously unintentional.

@ltworf
Copy link

ltworf commented May 9, 2021

Well imports are code.

Specifically now I have the problem that distributions use one version, pip installs another and I don't know if I can make code compatible with both.

Also mypy fails even after fixing the imports so that doesn't help so much.

Didn't it change between 8.x and 9.x? That sounds like semver to me.

If you see the releases, the version number changes almost all the time, meaning that either it's not semver or it breaks API all the time.

@aaugustin
Copy link
Member

@ltworf -

You could have at least put a changelog file to say that the api was changing.

I did. That didn't help since you didn't read the changelog before complaining.

Also semver would be nice.

websockets follows semver. While semver doesn't work, some folks like it and and would complain otherwise; making them happy doesn't cost much.

either it's not semver or it breaks API all the time

Well that's just one of the reasons why semver doesn't work. Due to Hyrum's law, given enough users, many kinds of changes could be breaking change for some users, so the major version number changes often. I suspect some of these "possibly breaking" changes didn't actually affect anyone.

Please consider the effect that the patronizing tone of your comments has on my willingness to work for you for free.

The immediate effect is that my motivation to work on this bug, which doesn't affect me personally, dropped a lot 🤷

@gmjosack -

Thanks for pointing out that, even with the best intentions, mistakes can happen.

@ltworf
Copy link

ltworf commented May 9, 2021

I did. That didn't help since you didn't read the changelog before complaining.

I downloaded the .tar.gz and there is no changelog file, so unless you mean I clone the repo and check all the commits, there is no changelog.

Please consider the effect that the patronizing tone of your comments has on my willingness to work for you for free.

I use your library in a open source project that I maintain in my free time and for which I have never received a single donation.

So now because of your breaking changes I'm on a sunday debugging things because those changes completely break my project for my (non paying) users.

@scottbelden
Copy link

scottbelden commented May 9, 2021

@ltworf If this is really such a big issue for you then you should pin all of your dependencies. I too was affected by this and the solution was quite simple. Quickly make a commit to pin the dependency to <9.0 and then come back to it with a better fix once there is one.

I too maintain an open source project and it baffles me that you do as well but then choose to come harass the maintainer here. Since any change in a dependency could break end users, you should probably pin all of your dependencies or maintain forks of all of your dependencies so that you can properly vet all changes if a change like this is such a disruptive change for you.

@apljungquist
Copy link

apljungquist commented May 9, 2021

Option 1bis - hide .legacy ... I think this is my favorite option.

I agree that this is a good option and it appears to work for a simple example. However, if IDEs and other development tools prefer this pattern, then it could become widespread making the deprecation warnings less useful.

Edit: removed hasty suggestion

Edit: Another idea is to bypass lazy import system for type checking. It's cumbersome and kind of defeats the purpose of the lazy import, but it appears to work for mypy, pylint and pycharm alike:

from typing import TYPE_CHECKING
if TYPE_CHECKING:
    from websockets.legacy.client import connect
    from websockets.legacy.client import WebSocketClientProtocol
    # etc

@ltworf
Copy link

ltworf commented May 9, 2021

@scottbelden

pinning dependencies and embedding stuff is a sure way of making sure your project will never be included in a distribution. I have done that but I also need to make a minor release so that potential users downloading the latest release get something that works.

But long term this is no solution.

I too maintain an open source project and it baffles me that you do as well but then choose to come harass the maintainer here.

Stating that there is no changelog and that changing the import doesn't make mypy happy is facts, not harassment. It is providing relevant facts for the resolution of the issue.

And again pinning dependencies is not a good thing and when developing a library it is good to keep compatibility in mind, when possible.

I'm not saying all projects should be like the linux kernel and keep bugs for the sake of remaining compatible forever, but there is a middle ground between that and the opposite extreme of renaming all the functions every release.

@aaugustin
Copy link
Member

The conversation diverged quite a bit from the original issue so I'm going to lock it and go back to solving the issue.

@python-websockets python-websockets locked as off-topic and limited conversation to collaborators May 9, 2021
@aaugustin
Copy link
Member

@apljungquist -

Thanks for testing and providing feedback!

Also, yes, I briefly considered the if TYPE_CHECKING: plan and ended up with the same conclusion as you:

  • at this point, better scratch the dynamic import system as its benefits are lost (ease of moving things around to refactor);
  • also, websockets jumps through hoops in a couple places (e.g. imports at the bottom of files) to avoid if TYPE_CHECKING and I'd rather not introduce it.

@aaugustin
Copy link
Member

I just released 9.0.2 with the fix.

@python-websockets python-websockets unlocked this conversation May 15, 2021
@apljungquist
Copy link

apljungquist commented May 15, 2021 via email

@aaugustin
Copy link
Member

Apologies for the disruption!

@aaugustin
Copy link
Member

For whoever ends up here: try this:

from websockets.client import connect
from websockets.server import serve

if mypy or other tools that don't understand the dynamic nature of Python give you false positives.

@ThatXliner
Copy link

For those using pylint, I wrote a(n experimental) pylint plugin for this

@aaugustin aaugustin changed the title Release 9.0 breaks mypy Release 9.0 breaks mypy (and pylint etc.) Oct 13, 2021
tsusanka added a commit to trezor/trezor-user-env that referenced this issue Nov 24, 2021
tsusanka added a commit to trezor/trezor-user-env that referenced this issue Nov 24, 2021
@DevilXD
Copy link

DevilXD commented Nov 29, 2021

@aaugustin I'm on v10.1 and this doesn't work with MyPy nor Pylance: https://i.imgur.com/y1UDAPl.png

The only modules that work are lazy_import and version, everything else is hidden behind __init__.py and its lazy import system.

Using if TYPE_CHECKING imports is perfectly fine for typed code, not sure why are you trying so hard to avoid it. It has to be used at the top of the file, to import things for type checking purposes only, that'd otherwise create import cycles. Similarly, if your "lazy import" system is there only to avoid having to deal with import cycles, it's perfectly fine to import things within functions, like so: https://github.com/DevilXD/aRez/blob/master/arez/match.py#L469

Imports are persistent, meaning that once you import something once, any subsequent import is going to just link the already imported module from memory instead of executing it again. Alternatively, you can avoid import cycles by using just import XYZ instead of from XYZ import ABC form. Both solutions end up having negligible performance impact, equal to a single extra attribute lookup.

TYPE_CHECKING import example: https://github.com/DevilXD/aRez/blob/master/arez/match.py#L14

I'm extensively using typed code and MyPy for early error detection and type hinting purposes, and the way it is right now, makes it literally impossible, short of modifying sys.path to bypass your __init__.py and importing your classes directly from the package folder.

@aaugustin
Copy link
Member

@DevilXD There's a typo in that screenshot. Try from websockets.client import ... instead of from websocket.client import ....

@aaugustin
Copy link
Member

For the record, the lazy import mechanism is intended to avoid loading the server code when you're only going to use the client code and vice-versa. I am ready to admit that it's a nano-optimization no one cares about. The CPU and memory impact of loading an unused module isn't very high. That said, unless you're a Tidelift subscriber, you paid $0 for this software. For that price, you can probably bear with my quirks 🤷

I appreciate that you wanted to give me some context about how imports work in Python. I happen to have a bit of domain expertise in this area, especially import cycles.

Lazy imports don't really help with import cycles. If anything, they make the situation more confusing, as they make the import sequence less deterministic.

@ltworf
Copy link

ltworf commented Nov 30, 2021

Please consider the effect that the patronizing tone of your comments has on my willingness to work for you for free.

That said, unless you're a Tidelift subscriber, you paid $0 for this software. For that price, you can probably bear with my quirks shrug

To be fair, you are quite free to release this as proprietary software.

What I do is that I use GPL license, and I'm open to give LGPL exceptions for money (which has never once happened).

In this way I know that people who don't pay are doing libre software, so they are welcome to use my stuff for free.

Those who want to make proprietary stuff (and are most likely making money from it) generally end up not using my project.

@aaugustin
Copy link
Member

In fact I'm happy if people use websockets and make money with it (preferably in an environmentally responsible way). I'm not really trying to monetize it.

When I bring up the price, it's usually a signal that the discussion crossed a frontier and I don't think the feedback is going to help me improve websockets.

@DevilXD
Copy link

DevilXD commented Nov 30, 2021

@DevilXD There's a typo in that screenshot. Try from websockets.client import ... instead of from websocket.client import ....

Hmm, that appears to have worked. I'm not sure how I've missed that yesterday, I guess it's my mistake then.

For the record, the lazy import mechanism is intended to avoid loading the server code when you're only going to use the client code and vice-versa. I am ready to admit that it's a nano-optimization no one cares about. The CPU and memory impact of loading an unused module isn't very high. That said, unless you're a Tidelift subscriber, you paid $0 for this software. For that price, you can probably bear with my quirks 🤷

I see. I'm sorry if this came out harsh to you, but out of frustration, I seriously considered looking for a different package yesterday, one that would allow we to type check my code properly. If you really are happy that others use websockets, then it only makes sense to not make it unnecessarily difficult to use them. Am I right?

@aaugustin
Copy link
Member

Yes, it makes sense :-) I'm glad you're sticking with websockets :-)

@aaugustin
Copy link
Member

439dafa should solve the problem fully going forwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants