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

Replace current WSGIMiddleware implementation by a2wsgi one #1825

Merged
merged 15 commits into from Jan 16, 2023

Conversation

humrochagf
Copy link
Contributor

@humrochagf humrochagf commented Dec 31, 2022

@humrochagf
Copy link
Contributor Author

I kept most of wsgi tests as a backwards compatibility validation, and that's also why I kept the wsgi module, to avoid breaking someone importing it directly.

If that's not needed I can drop the module and the tests for it and move it inside the config as the previous PR was.

I also added a test for the missing module case since in #177 there was no adhesion to push towards wsgi feature so I understood it as an optional feature, that is part of the [standard] package and not the minimal one.

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 31, 2022

I know I said this, but did you check what is needed for us to just solve #371 without adding a2wsgi as optional dependency?

I'd like to explore that option if it's not that complicated.


btw, I invited you to encode. Thanks for all the help here. 👍

@humrochagf
Copy link
Contributor Author

Hey, sure what is required for the fix is a rework of how the body is handled to stream the payload instead of loading it into memory.

@euri10 did this change in #1049 PR.

And thanks for inviting me to encode looking forward to help more with uvicorn 😃

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 31, 2022

@euri10 did this change in #1049 PR.

Hmm... That has a footprint that I believe it would be worth it just replacing to a2wsgi. Is all that needed?

And thanks for inviting me to encode looking forward to help more with uvicorn 😃

😎 👍

@humrochagf
Copy link
Contributor Author

Yup, all there, also tested running a django/takahe instance replacing fully gunicorn by uvicorn and doing some image uploads and everything is running fine

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 31, 2022

I'm not sure if I was clear. I mean: is it possible to achieve a fix with less footprint?

@humrochagf
Copy link
Contributor Author

Oh okay, I can try some things, but for sure we need a body that streams content on demand, I'll write a small test scenario with a bigfile that is more reproducible than trying things manually and try to reduce the scope of this change.

@humrochagf
Copy link
Contributor Author

Yeah, there is not much that can be done to reduce footprint.

WSGI expects a file like structure in the wsgi.input, so we need to have that bridge between the async reader and the sync file like stream to deliver the body without preload the content in memory and explode the pod with big file uploads.

Another viable path to follow is a deprecation warning on wsgi interface + adding a pluggable interface option + docs on how to plug a2wsgi if maintaining wsgi isn't a focus of uvicorn.

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 31, 2022

I'll see this tomorrow. Thanks!

Feliz ano novo Humberto! 😁

@humrochagf
Copy link
Contributor Author

Alright!

Feliz ano novo pra vc tb 😁

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abersheeran If you have time, can you check if this is fine?

pyproject.toml Outdated
@@ -36,6 +36,7 @@ dependencies = [

[project.optional-dependencies]
standard = [
"a2wsgi>=1.6.0,<2.0.0",
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a2wsgi as uvicorn dependency, and change the documentation as mentioned on #1303 (comment).

Notes about this decision:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, is fine. 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the change will be the move from optional dependencies to the base dependecies, is that it?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and to add on the documentation that Uvicorn is also a WSGI server.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Et voilà 🙂

Since WSGIMiddleware converts WSGI into ASGI I described it as an ASGI first web server what do you think?

@Kludex Kludex added the wsgi label Jan 6, 2023
@Kludex Kludex added this to the Version 0.21.0 milestone Jan 6, 2023
@Kludex
Copy link
Sponsor Member

Kludex commented Jan 7, 2023

I gave a bit more thought about this.

This is what I suggest us to do:

  1. Don't add a2wsgi as a dependency. Instead, ask the user to download a2wsgi if needed.
  2. Don't remove uvicorn's WSGIMiddleware, but deprecate it, mentioning that people should install a2wsgi.
  3. Add a logic to check if a2wsgi is installed (just import), and if it's use it. If not, use the one from uvicorn.
  4. Don't change documentation in regard to Uvicorn being a WSGI server.

The idea here is to not add another minimal dependency - as we always want to avoid this as much as possible - and to not break our users.

What do you think @humrochagf ?

@Kludex Kludex added the waiting author Waiting for author's reply label Jan 7, 2023
@humrochagf
Copy link
Contributor Author

Sounds good 🙂

Comment on lines 199 to 202
try:
from a2wsgi import WSGIMiddleware # type: ignore # noqa
except ImportError:
pass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the most attractive solution, but it's the best option to deprecate it without touching the config module since for testing purposes we can't reload the config module import table without breaking unrelated tests that share the config module state.

It also keeps backward compatibility with someone importing it directly in code.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
try:
from a2wsgi import WSGIMiddleware # type: ignore # noqa
except ImportError:
pass
try:
from a2wsgi import WSGIMiddleware # type: ignore # noqa
except ModuleNotFound:
WSGIMiddleware = _WSGIMiddleware

@humrochagf
Copy link
Contributor Author

humrochagf commented Jan 8, 2023

I think it's all there, the only weird thing is that when running uvicorn command without a2wsgi I don't see the deprecation warning, but pytest detects it.

Is there any kind of filter to not log it in terminal?

Update: If I change the filter in my client code to show it, then I can see the deprecation warning:

import warnings
warnings.simplefilter("always")

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure about the documentation.

Can we just add a small note on https://www.uvicorn.org/settings/#application-interface mentioning that the user should install a2wsgi for WSGI support, and that the Uvicorn's native WSGI integration is deprecated?

uvicorn/middleware/wsgi.py Outdated Show resolved Hide resolved
uvicorn/middleware/wsgi.py Outdated Show resolved Hide resolved
Comment on lines 199 to 202
try:
from a2wsgi import WSGIMiddleware # type: ignore # noqa
except ImportError:
pass
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
try:
from a2wsgi import WSGIMiddleware # type: ignore # noqa
except ImportError:
pass
try:
from a2wsgi import WSGIMiddleware # type: ignore # noqa
except ModuleNotFound:
WSGIMiddleware = _WSGIMiddleware

Comment on lines 17 to 35
def environ_switcher(
async_test: Callable[[], Awaitable[None]]
) -> Callable[[bool], Awaitable[None]]:
from uvicorn.middleware import wsgi

async def test_wrapper(use_a2wsgi: bool) -> None:
if use_a2wsgi:
await async_test()
else:
with mock.patch.dict(sys.modules, {"a2wsgi": None}):
reload(wsgi)

await async_test()

reload(wsgi)

return test_wrapper


Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we do this, and avoid the @environ_switcher magic, and the individual parametrize on each test.

Suggested change
def environ_switcher(
async_test: Callable[[], Awaitable[None]]
) -> Callable[[bool], Awaitable[None]]:
from uvicorn.middleware import wsgi
async def test_wrapper(use_a2wsgi: bool) -> None:
if use_a2wsgi:
await async_test()
else:
with mock.patch.dict(sys.modules, {"a2wsgi": None}):
reload(wsgi)
await async_test()
reload(wsgi)
return test_wrapper
@pytest.fixtures(param=wsgi._WSGIMiddleware, a2wsgi.WSGIMiddleware)
def wsgi_middleware_class(request):
return request.param

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, much better now 😄

@@ -34,7 +55,7 @@ def echo_body(environ: Environ, start_response: StartResponse) -> List[bytes]:
return [output]


def raise_exception(environ: Environ, start_response: StartResponse) -> RuntimeError:
def raise_exception(environ: Environ, start_response: StartResponse) -> List[bytes]:
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you did this, but I don't think it's the right type. It should be NoReturn 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the typechecker perspective WSGIMiddleware expects a function that has List[bytes] as return type, if it aways fails into exception is not that important.

This type error started to be more clear because the a2wsgi WSGIMiddleware is less permissive in terms of functions it accepts.

docs/index.md Outdated
Comment on lines 465 to 466
Uvicorn offer support to WSGI applications throught `WSGIMiddleware` which converts
your WSGI into ASGI application seamlessly just by running it with `--interface wsgi`
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the first mention to the WSGIMiddleware on the documentation? I think this is just an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also cut it and put just the mention in the settings like you said

docs/index.md Outdated
Comment on lines 469 to 470
The present WSGIMiddleware will be deprecated in favour of [a2wsgi](https://github.com/abersheeran/a2wsgi).
If you are using Uvicorn to run WSGI applications, please add it as part of your project requirements.
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be removed, but this PR already deprecates it.

Suggested change
The present WSGIMiddleware will be deprecated in favour of [a2wsgi](https://github.com/abersheeran/a2wsgi).
If you are using Uvicorn to run WSGI applications, please add it as part of your project requirements.
The present WSGIMiddleware is deprecated in favour of [a2wsgi](https://github.com/abersheeran/a2wsgi).
If you are using Uvicorn to run WSGI applications, please add it as part of your project requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, updated the docs

docs/index.md Outdated
Run the server:

```shell
$ uvicorn --interface wsgi example:app
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the --interface needed, or does uvicorn infers it? I never used WSGI with uvicorn. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fails to infer it, maybe a bug but it falls into asgi2 case that also uses a function instead of a coroutine in the config code:

...
            elif inspect.isfunction(self.loaded_app):
                use_asgi_3 = asyncio.iscoroutinefunction(self.loaded_app)
...

@humrochagf
Copy link
Contributor Author

ok, I'll need to stop for today, tomorrow I'll fix what is missing

@humrochagf
Copy link
Contributor Author

Alright!
I think I checked everything 🙂

@humrochagf
Copy link
Contributor Author

Just saw your test simplification, didn't knew fixtures could behave like parametrize

@humrochagf
Copy link
Contributor Author

Done, updated the tests with the code simplification

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 10, 2023

I'm on the phone, but I can comment some stuff.

There are two things that I think we can improve before merging:

  1. Improve the warning message. I think the user shouldn't be aware that uvicorn has an entity called "WSGIMiddleware". Maybe something like "uvicorn's native WSGI implementation is deprecated in favor of a2wsgi package. Please install it with pip install a2wsgi." - or something like that.
  2. We can specify which "# type: ignore[here]". There's an option on mypy to make this mandatory, that we have on starlette already. And the noqa I believe it can be removed? 🤔

Besides this, everything is cool here. Sorry for the round-trips here.

@humrochagf
Copy link
Contributor Author

Oh, no worries and all done 🙂

Had to ignore 2 kinds of type error codes:

uvicorn/middleware/wsgi.py:202: error: Cannot assign to a type  [misc]
uvicorn/middleware/wsgi.py:202: error: Incompatible types in assignment (expression has type "Type[_WSGIMiddleware]", variable has type "Type[WSGIMiddleware]")  [assignment]

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @humrochagf 🙏

Feel free to merge it :)

@humrochagf humrochagf merged commit 2351d5f into encode:master Jan 16, 2023
@humrochagf
Copy link
Contributor Author

Yay, can I delete the branch after the merge as well?

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 16, 2023

It's in your fork anyway 👀

@humrochagf
Copy link
Contributor Author

Oh right, the way the ui presented it I thought it had a branch copy in encode/uvicorn side 😜

@humrochagf humrochagf deleted the a2wsgi branch January 16, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting author Waiting for author's reply wsgi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WSGI middleware should stream request body, rather than loading it all at once.
3 participants