posit-dev / py-shiny

Shiny for Python
https://shiny.posit.co/py/
MIT License
1.32k stars 81 forks source link

websockets 14.0 support #1769

Closed jcheng5 closed 1 week ago

jcheng5 commented 1 week ago

See #1766

The websockets package released 14.0 which breaks us when using shiny run --reload.

Once we get this working, I think we also need to decide whether to require 14.0, or to allow older versions and adapt to whatever version is being used.

jcheng5 commented 1 week ago

If we're OK with requiring websockets>=13.0, then this is actually pretty straightforward, as the websockets.asyncio submodule hasn't really changed between 13 and 14. Going back to websockets<13.0 is harder, as then we need code for both legacy and asyncio implementations, and for type checking to pass in both cases is pretty annoying.

jcheng5 commented 1 week ago

Oh I forgot to mention how I solved the scary error message: by defaulting the websocket logger to CRITICAL. In theory this could squash legit error messages (that are specific to auto-reloading).

https://github.com/posit-dev/py-shiny/pull/1769/files#diff-be8712ed65f19637d0738c3495776b73a667e8f9e32a6ae4dea92328fe906259R173-R182

You can set SHINY_AUTORELOAD_LOG_LEVEL=DEBUG if you need to reveal those messages.

gadenbuie commented 1 week ago

Oh I forgot to mention how I solved the scary error message: by defaulting the websocket logger to CRITICAL. In theory this could squash legit error messages (that are specific to auto-reloading).

It's hard to set this environment variable when you're debugging in VS Code, but I spent a while looking at both sides and I think ultimately this is the right choice, at least for now. I doubt Shiny users need to see any of the websockets debug messages; in fact, I'd think these messages are more likely to be noise or intimidating than useful. And in the edge cases where we or power users need to see them we still can.

karangattu commented 1 week ago

LGTM, thank you!

@karangattu A note for QA: we should do some quick smoke tests in GitHub Codespaces, Posit Workbench, etc. before release just to be sure.

Yes, I can test it as soon as it is merged. Or. do you want me to checkout the branch and test it against that?

jcheng5 commented 1 week ago

@gadenbuie Yeah, I ended up setting the environment variable in my ~/.zshrc. I was thinking about just leaving it on but it really is so distracting to have two stack traces appear every time you launch an app 😞

Wonder if websockets would be open to a PR that silences this particular case...

gadenbuie commented 1 week ago

Wonder if websockets would be open to a PR that silences this particular case...

They might be open to it. It's very similar to another case where this kind of error appears that was fixed in 14.1 released last night. https://github.com/python-websockets/websockets/issues/1513