-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix websockets deprecation warning #3749
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
base: main
Are you sure you want to change the base?
Conversation
kclowes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! This is an (admittedly pretty small) breaking change with the type from WebSocketClientProtocol -> ClientConnection, so we'll need to wait until v8 to get this in. I am going to start working on v8 changes in the next few weeks, so will be able to get these changes in then. I left a couple comments that I can either address when I pull it in under v8, or if you want/have bandwidth to update, feel free!
I didn't know that you could use the Union | before Python 3.10, that's awesome!
| async def _provider_specific_disconnect(self) -> None: | ||
| # this should remain idempotent | ||
| if self._ws is not None and not self._ws.closed: | ||
| if self._ws is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need to figure out how to check for self._ws.closed, even if it's not that same method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I did check this and I found out that self._ws.close() is idempotent already, what I didn't realise though is that web3.py still supports websockets >=10.0,<13.0, meaning me importing directly from websockets.asyncio would break that compatibility.
So I would need to rework my PR to take that into account.
I also noticed this issue #3679 which plans to move websockets bottom pin to >=14 #3530, which would essentially remove this problem completely.
Should I just do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could either go down the path of supporting all the variations (aka <13; >=13,<14; >=14), i.e.
import websockets
websockets_version = tuple(int(x) for x in websockets.__version__.split(".") if x.isdigit())
if websockets_version < (13, 0):
from websockets.legacy.client import (
WebSocketClientProtocol as ClientConnection, # we are safe to steal the name as the scope of interface we use is the same
connect,
)
else:
# python3.8 supports up to version 13,
# which does not default to the asyncio implementation yet.
# For this reason connect and ClientConnection need to be imported
# from asyncio.client explicitly.
# When web3.py stops supporting python3.8,
# it'll be possible to use `from websockets import connect, ClientConnection`.
from websockets.asyncio.client import (
ClientConnection,
connect,
)or simply drop support for at least version <13.
Given the amount of usage of the websockets library interface either solution is fine, not a big deal.
|
Hey @kclowes ! I agree this is a potentially breaking change, we might even not need it in this shape, depending on when it is planned to sunset the About your comments, I can definitely take care of them 💪 , thanks for those. |
What was wrong?
Importing
from web3 import Web3was causing a DeprecationWarning to be raised.This can be easily replicated by doing:
This PR aims at resolving that once for all.
How was it fixed?
websockets.legacyin the legacy providerwebsocketspackageTodo:
Notes:
The pre-commit hook changed quite a lot the file, I ended up committing those changes in a separate commit so that the relevant changes can be seen in isolation.
Cute Animal Picture