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

** 3.9 Beta testing ** #7675

Closed
7 of 8 tasks
Tracked by #422 ...
Dreamsorcerer opened this issue Oct 7, 2023 · 129 comments
Closed
7 of 8 tasks
Tracked by #422 ...

** 3.9 Beta testing ** #7675

Dreamsorcerer opened this issue Oct 7, 2023 · 129 comments

Comments

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Oct 7, 2023

3.9.0rc0 is now out, likely to be promoted as a stable release at the weekend.

We've introduced a few significant changes in 3.9, so it would be great to get some community testing before promoting it as a stable release. We've released 3.9.0b1 as a prerelease to help with this.

Can be installed with: pip install 'aiohttp<4' --pre --upgrade

In particular, these are things we are interested on testing or gathering feedback on:

Client-side

Server-side


TODO: Prepare aiohttp libraries for AppKey:

@Dreamsorcerer
Copy link
Member Author

One thing that bothers me about 3.12 is that our import time test always fails in the initial test run, and then passes in the rerun. I'm not sure what change in 3.12 would cause that, but it seems to happen 100% of the time. It's not a small margin either, it misses the target time by around 30-50%.

Maybe someone else has some ideas?

@h-vetinari
Copy link

h-vetinari commented Oct 8, 2023

I spent some time to enable the test suite on our conda-forge "feedstock" that repackages aiohttp - unfortunately this wasn't turned on so far, most likely because it's a somewhat non-trivial effort, especially as aiohttp is really cumbersome to build 100% from source (I gave up on that after ~5 onion layers and used a work-around that's... kinda acceptable, but bespoke).

Compared to the "existing" failures (w.r.t. our infrastructure) for 3.8.6, this is the delta for 3.9.0b0:

@@ -60,14 +60,17 @@ test:
     {% set tests_to_skip = "_not_a_real_test" %}
-    {% set tests_to_skip = tests_to_skip + " or test_feed_eof_no_err_brotli" %}
     {% set tests_to_skip = tests_to_skip + " or test_http_response_parser_bad_chunked_strict_c[pyloop]" %}
     {% set tests_to_skip = tests_to_skip + " or test_http_response_parser_bad_chunked_strict_py[pyloop]" %}
     {% set tests_to_skip = tests_to_skip + " or test_http_response_parser_strict_headers[c-parser-pyloop]" %}
-    {% set tests_to_skip = tests_to_skip + " or test_request_tracing_url_params[pyloop]" %}
     {% set tests_to_skip = tests_to_skip + " or test_tcp_connector_raise_connector_ssl_error[pyloop]" %}
     # test failing with python >=3.11
     {% set tests_to_skip = tests_to_skip + " or test_https_proxy_unsupported_tls_in_tls[pyloop-https]" %}   # [py>=311]
+    # fails to open socket at desired IP/port
+    {% set tests_to_skip = tests_to_skip + " or (test_web_server and test_handler_cancellation)" %}         # [linux]
+    {% set tests_to_skip = tests_to_skip + " or (test_web_server and test_no_handler_cancellation)" %}      # [linux]
+    # emulation is too slow for expected speed
+    {% set tests_to_skip = tests_to_skip + " or (test_imports and test_import_time)" %}                     # [aarch64 or ppc64le]
     # aio seems to hard-code expectation for cpython (i.e. not "pypy3.9" in SP_DIR path)
     {% set tests_to_skip = tests_to_skip + " or test_static_route_user_home" %}             # [python_impl == "pypy"]
     # not working with pypy (all platforms)
     [...]

The timeout failure is unsurprising, as we have no native hardware for aach64/ppc64le, and so those run through emulation in QEMU, which is obviously slower.

The other two look like this:

FAILED sources/tests/test_web_server.py::test_handler_cancellation - OSError: [Errno 99] error while attempting to bind on address ('::1', 51455, 0, 0): cannot assign requested address
FAILED sources/tests/test_web_server.py::test_no_handler_cancellation - OSError: [Errno 99] error while attempting to bind on address ('::1', 56597, 0, 0): cannot assign requested address

All in all, I think this looks pretty OK 👍

@haukex
Copy link

haukex commented Oct 8, 2023

I can see how the "It is recommended to use web.AppKey instances for keys" warning (here) is useful when upgrading to 3.9.0, but IMHO there also needs to be a way for it to be suppressed until libraries such as aiohttp-security can be updated, see e.g. this. I guess this could be done either by making a UserWarning subclass for which one could then make a filter, or by providing a configuration option.

@Dreamsorcerer
Copy link
Member Author

It's a warning, so normal warning filters can be used (CLI argument or warnings.filterwarnings()). I'll try it out shortly and see if we can add to the documentation.

@Dreamsorcerer
Copy link
Member Author

Dreamsorcerer commented Oct 8, 2023

Hmm, maybe I misunderstood the module parameter, it doesn't look like I can filter it with -Wignore::UserWarning:aiohttp.web_app for example. Maybe a subclass makes more sense then.

Also, the warning was not meant to be displayed by default (only when the user enables warnings with -Wdefault or -We, for example). So, I need to review that too.

@haukex
Copy link

haukex commented Oct 8, 2023

Ah, I see. I run my test suite with PYTHONWARNINGS=error which is why I noticed it immediately. I was apparently able to suppress it with warnings.filterwarnings('ignore', category=UserWarning, message='It is recommended to use web.AppKey instances for keys.'), but I feel like that's not optimal; if it's just meant to a help in upgrading then it should probably be opt-in.

agmmnn added a commit to agmmnn/syn that referenced this issue Oct 8, 2023
@Dreamsorcerer
Copy link
Member Author

Hmm, well ignoring it via CLI won't be so easy: python/cpython#66733
Doesn't look like there's any way to do it on CLI without message matching. Should be able to do it with runtime code though.

@Dreamsorcerer
Copy link
Member Author

Dreamsorcerer commented Oct 8, 2023

OK, subclassed and ignored by default in #7677.

Should be able to separately ignore it with -W 'ignore:It is recommended to use web.AppKey' or warnings.filterwarnings("ignore", category=web.NotAppKeyWarning).

@haukex
Copy link

haukex commented Oct 8, 2023

Thanks! Other than that no issues so far, my test suite passes on 3.9.0b0 and Python 3.12; I just switched over to using web.AppKey. Keeping a set of keys in an enum apparently requires a bit of extra code though due to python/typing#535, but at least it works:

class AppKeys(enum.Enum):
    FOO = web.AppKey('foo', Foo)
    BAR = web.AppKey('bar', Bar)
    @property
    def value(self) -> web.AppKey:
        return super().value  # type: ignore
...
app[AppKeys.FOO.value] = Foo()
# etc.

@haukex
Copy link

haukex commented Oct 8, 2023

Another place that makes use of string app keys and causes the warning is https://github.com/aio-libs/aiohttp-devtools/blob/master/aiohttp_devtools/runserver/serve.py

@Dreamsorcerer
Copy link
Member Author

Dreamsorcerer commented Oct 8, 2023

Keeping a set of keys in an enum apparently requires a bit of extra code though due to python/typing#535, but at least it works

That still throws away the typing information, making it pointless to use AppKey().

reveal_type(app[AppKeys.FOO.value]) # Revealed type is "Any"

Compared with:

foo_key = web.AppKey("foo", Foo)
reveal_type(app[foo_key])  # Revealed type is "foo.Foo"

@benz0li
Copy link

benz0li commented Oct 8, 2023

My JupyterLab Python docker stack seems to work fine.

Build chain:

  1. docker build --build-arg PYTHON_VERSION=3.12.0 -t python/ver:3.12.0 -f ver/latest.Dockerfile .
  2. cd base && docker build --build-arg BUILD_ON_IMAGE=python/ver --build-arg PYTHON_VERSION=3.12.0 -t jupyterlab/python/base:3.12.0 -f latest.Dockerfile .
    • git diff base/latest.Dockerfile
      diff --git a/base/latest.Dockerfile b/base/latest.Dockerfile
      index 1a8ff15..40effc9 100644
      --- a/base/latest.Dockerfile
      +++ b/base/latest.Dockerfile
      @@ -7,7 +7,7 @@ ARG CUDA_IMAGE_FLAVOR
       ARG NB_USER=jovyan
       ARG NB_UID=1000
       ARG JUPYTERHUB_VERSION=4.0.2
      -ARG JUPYTERLAB_VERSION=3.6.6
      +ARG JUPYTERLAB_VERSION=4.0.6
       ARG CODE_BUILTIN_EXTENSIONS_DIR=/opt/code-server/lib/vscode/extensions
       ARG CODE_SERVER_VERSION=4.17.1
       ARG GIT_VERSION=2.42.0
      @@ -227,12 +227,14 @@ RUN mkdir /opt/code-server \
       ## Install JupyterLab
       RUN export PIP_BREAK_SYSTEM_PACKAGES=1 \
         && pip install \
      +    aiohttp==3.9.0b0 \
           jupyter-server-proxy \
           jupyterhub==${JUPYTERHUB_VERSION} \
           jupyterlab==${JUPYTERLAB_VERSION} \
      -    jupyterlab-git \
      +    #jupyterlab-git \
           jupyterlab-lsp \
           notebook \
      +    nbclassic \
           nbconvert \
           python-lsp-server[all] \
         ## Include custom fonts

Run container:

docker run -it --rm -p 8888:8888 jupyterlab/python/base:3.12.0

Enjoy!

@Dreamsorcerer
Copy link
Member Author

One more issue I've found while testing is with using websockets. If we have open websockets, then the graceful shutdown will always wait for the timeout (as the websocket tasks won't end) before continuing with shutdown.

Should we add a pre_shutdown signal or something? Then websocket closing (as shown in https://docs.aiohttp.org/en/stable/web_advanced.html#graceful-shutdown) can be done in a pre-shutdown handler before the server waits for running tasks to complete.

@haukex
Copy link

haukex commented Oct 8, 2023

That still throws away the typing information

Hm, mypy doesn't see it, though the type hinting in VSCode does:
Unbenannt

@Dreamsorcerer
Copy link
Member Author

Dreamsorcerer commented Oct 8, 2023

Hm, mypy doesn't see it, though the type hinting in VSCode does:

Isn't that a hardcoded annotation? It should work without an annotation.
That issue you linked, and the workaround you implemented, only narrow the value types to AppKey. There's no way to get the precise values unless something is tracking the individual enum values.

@hyperlogic
Copy link

hyperlogic commented Oct 8, 2023

When trying to pip install -U --pre aiohttp on Windows 11 using Visual Studio 2022 and Python 3.12. I get the following error:

      aiohttp/_websocket.c(196): fatal error C1083: Cannot open include file: 'longintrepr.h': No such file or directory
      error: command 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.37.32822\\bin\\HostX86\\x64\\cl.exe' failed with exit code 2
      [end of output]

@Malogna
Copy link

Malogna commented Oct 8, 2023

When trying to pip install -U --pre aiohttp on Windows 11 using Visual Studio 2022 and Python 3.12. I get the following error:

      aiohttp/_websocket.c(196): fatal error C1083: Cannot open include file: 'longintrepr.h': No such file or directory
      error: command 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.37.32822\\bin\\HostX86\\x64\\cl.exe' failed with exit code 2
      [end of output]

See PyPI, aiohttp 4.0.0a1 is a pre-build from 2019. Use pip install aiohttp==3.9.0b0 to get Python 3.12 working with aiohttp

@haukex
Copy link

haukex commented Oct 8, 2023

It's not obvious from the screenshot but that annotation isn't hardcoded, it's auto generated by Pylance because I have its "Inlay Hints: Variable Types" featured turned on. Here's a better screenshot:

Unbenannt

In my actual app I have it split across multiple files so it really does pick up on it correctly.

But anyway, since mypy isn't picking up on it, I guess I'll have to switch away from an enum.

@hyperlogic

This comment has been minimized.

@nolar
Copy link

nolar commented Oct 9, 2023

Not directly related to the functionality, but to packaging: In 3.9.0b0, the tests directory is packaged into the wheels and installed into site-packages as a package. I guess, this is not intended. Besides, it makes PyCharm go crazy and navigate my code improperly.

@bdraco
Copy link
Member

bdraco commented Nov 13, 2023

I've beat this up as much as I can. From my perspective, 3.9.0 seems good to go.

@Dreamsorcerer
Copy link
Member Author

Dreamsorcerer commented Nov 14, 2023

I've pushed 3.9.0rc0, which contains some fixes and performance improvements.

One of the fixes required a few changes to some of our tests, I'm expecting this will only affect our internal tests, but if a couple of people can confirm their test suite is still passing that'd be helpful.

If no further issues are found, I'll promote it to a stable release over the weekend (when I have some time to make releases for the other libraries).

@bdraco
Copy link
Member

bdraco commented Nov 14, 2023

Perfect. I'll get some wider testing on rc0 once the wheels finish cooking & approval

home-assistant/core#103507 (that PR was stuck due to the issue above)

@salty-horse
Copy link

Please add classifiers for 3.11 and 3.12 to setup.cfg, as support for 3.12 is one of the reasons people are waiting for this release.

theCapypara added a commit to theCapypara/aiohttp that referenced this issue Nov 15, 2023
@Dreamsorcerer
Copy link
Member Author

See the duplicate issues and PRs that are all closed. It was decided not to add them until the test suite is stable and passing. It still doesn't pass for 3.11 or 3.12 yet, which means support is still considered somewhat experimental.

bdraco started to try and fix them in #7773.

@JrooTJunior
Copy link
Contributor

Any news about release date?

@Dreamsorcerer
Copy link
Member Author

Any news about release date?

#7675 (comment)

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

No branches or pull requests