Update ASGI requirements #911

dantownsend · 2023-12-12T11:29:45Z

Related to #907

Removing jinja2 as it's already a direct dependency of Piccolo
Removing requests as I don't think it's needed any more (I think this was for Starlette at some point)
Not installing piccolo_admin for esmerald

Piccolo has `jinja2` as a direct dependency, so this shouldn't be needed.

I think this was needed for Starlette at some point, but it has since transitioned to `httpx`, so I don't think it's needed any more.

We don't use it for `esmerald` so don't include it by default for all routers.

codecov-commenter · 2023-12-12T11:31:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4fb169d) 87.91% compared to head (c0d6413) 78.71%.
Report is 6 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
- Coverage   87.91%   78.71%   -9.21%     
==========================================
  Files         107      108       +1     
  Lines        8145     8199      +54     
==========================================
- Hits         7161     6454     -707     
- Misses        984     1745     +761

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dantownsend · 2023-12-12T11:32:43Z

piccolo/apps/asgi/commands/new.py

+ADMIN_DEPENDENCIES = ["piccolo_admin>=1.0.0"]
 ROUTER_DEPENDENCIES = {
-    "fastapi": ["fastapi>=0.100.0"],
+    "starlette": ["starlette", *ADMIN_DEPENDENCIES],
+    "fastapi": ["fastapi>=0.100.0", *ADMIN_DEPENDENCIES],
+    "blacksheep": ["blacksheep", *ADMIN_DEPENDENCIES],
+    "litestar": ["litestar", *ADMIN_DEPENDENCIES],
+    # We don't currently use Piccolo Admin with esmerald, due to dependency
+    # conflicts with FastAPI. We'll revisit this at some point in the future
+    # when newer versions of FastAPI are released (DT - 12th December 2023).
+    "esmerald": ["esmerald"],
 }


I decided to omit piccolo_admin from esmerald this way instead of having this in requirements.txt.jinja:

{% if "esmerald" not in router_dependencies %} piccolo_admin {% endif %}

Because at some point I'd like to version pin the routers a bit more. So it might be that we have esmerald>X.X.X in which case the if statement would fail.

dantownsend · 2023-12-12T11:34:30Z

piccolo/apps/asgi/commands/templates/app/requirements.txt.jinja

-piccolo[postgres]
-piccolo_admin
-requests
+piccolo[postgres]>=1.0.0


Not sure if this will cause any problems ... i.e. it means that all of routers must support Pydantic v2. Will see when the CI runs.

sinisaos · 2023-12-12T14:18:51Z

@dantownsend This should work because all router frameworks, used by Piccolo, support Pydantic v2, but I want to try each router template to be sure. I'll do it tonight because I can't right now, if you're okay with that?

dantownsend · 2023-12-12T16:04:36Z

@sinisaos No worries - thanks!

sinisaos

The changes from this PR are good and don't affect how the Piccolo asgi template works, but I found two things that don't work. First is that Litestar Swagger docs don't work, but that's not on our side. Fix for that is made in Litestar main branch but not released yet (when they release new version everything will be fine - see issue). Second thing is for Esmerald. We have to remove "piccolo_admin.piccolo_app" only for Esmerald template from here

piccolo/piccolo/apps/asgi/commands/templates/app/piccolo_conf.py.jinja

Line 17 in 4fb169d

apps=["home.piccolo_app", "piccolo_admin.piccolo_app"]

because we don't use Piccolo Admin in Esmerald template at the moment. If we don't remove Piccolo Admin from APP_REGISTRY Unable to connect to database exception is raised and we can't run migrations for home app. We can use something like this.

{% if router == 'esmerald' %}
APP_REGISTRY = AppRegistry(
    apps=["home.piccolo_app"]
)
{% else %}
APP_REGISTRY = AppRegistry(
    apps=["home.piccolo_app", "piccolo_admin.piccolo_app"]
)
{% endif %}

It is also necessary to somehow notify the user that they cannot currently run session_auth and user migrations for Esmerald (dependency conflicts with FastAPI)

dantownsend · 2023-12-13T16:53:22Z

@sinisaos Good points. Let's not worry about litestar, as you say - it'll be fixed when they release the next version.

With esmerald, omitting Piccolo Admin is definitely trickier than I imagined. I can add the if block to piccolo_conf.py.jinja like you suggest. And maybe just put something in the docs about support being beta at the moment.

sinisaos · 2023-12-13T20:02:44Z

With esmerald, omitting Piccolo Admin is definitely trickier than I imagined. I can add the if block to piccolo_conf.py.jinja like you suggest. And maybe just put something in the docs about support being beta at the moment.

That would be great.

I found another small issue with Blacksheep and Pydantic v2. If we create a Pydantic model with BaseModel everything works, but when we create a Piccolo model with create_pydantic_model, the request body (and example value) is empty like this

I think it's not a Piccolo problem, but a problem with the old version of SwaggerUI in BlackSheep. If I change [email protected] for CSS and JS to swagger-ui-dist@5 everything works fine like this

I know you have been in contact with the author of BlackSheep for mounting the ASGI apps to BlackSheep (for Piccolo Admin). If you could raise issue in their repo, referring to this issue, that would be great.

sinisaos · 2023-12-13T20:15:57Z

Litestar problem is solved with newest version (v2.4.4)

sinisaos · 2023-12-15T19:16:12Z

BlackSheep issue will also be resolved with the next BlackSheep release (SwaggerUI has been upgraded in the BlackSheep main branch)

dantownsend · 2023-12-15T19:28:06Z

@sinisaos Great, thanks a lot for checking that 👍

sinisaos · 2023-12-27T07:54:47Z

@dantownsend FastAPI has been upgraded (Starlette >=0.29.0,<0.33.0) and we can now add Piccolo Admin to the Esmerald template. The integration test passed, but when I try to make a new Esmerald project in a fresh virtualenv, I had problems pip installing awesome-slugify (an Esmerald dependency) @tarsil, can you check it?

tarsil · 2023-12-27T07:57:27Z

@dantownsend FastAPI has been upgraded (Starlette >=0.29.0,<0.33.0) and we can now add Piccolo Admin to the Esmerald template. The integration test passed, but when I try to make a new Esmerald project in a fresh virtualenv, I had problems pip installing awesome-slugify (an Esmerald dependency) @tarsil, can you check it?

Thank you @sinisaos I created actually some venvs in the past with all python versions and no issues were found. What error are you receiving?

sinisaos · 2023-12-27T08:21:41Z

@tarsil I'm not at my machine right now, but when I can, I'll post the error log here. I tried Py3.9, Py3.10, Py3.11 and always got the same error (can't install awesome-slugify) when I try to pip install esmerald in fresh virualenv.

tarsil · 2023-12-27T08:23:48Z

@tarsil I'm not at my machine right now, but when I can, I'll post the error log here. I tried Py3.9, Py3.10, Py3.11 and always got the same error (can't install awesome-slugify) when I try to pip install esmerald in fresh virualenv.

Also away for a couple of days and then I will be back at my machine but it is a first like this

sinisaos · 2023-12-27T13:50:42Z

@tarsil I'm very sorry, but the problem was my machine (very strange, but somehow setuptools was not installed when I created a new virtualenv and that caused problems with installing Esmerald). After restarting the laptop, everything is fine.
@dantownsend I can confirm that you can add Piccolo Admin support to Esmerald template, everything works fine.

tarsil · 2023-12-27T14:34:12Z

@tarsil I'm very sorry, but the problem was my machine (very strange, but somehow setuptools was not installed when I created a new virtualenv and that caused problems with installing Esmerald). After restarting the laptop, everything is fine. @dantownsend I can confirm that you can add Piccolo Admin support to Esmerald template, everything works fine.

Thank you @sinisaos for confirming. This was indeed odd but it could have happened

dantownsend · 2023-12-27T16:31:54Z

@sinisaos Cool, thanks for checking. It'll be much easier to not exclude Piccolo Admin.

sinisaos · 2023-12-27T16:47:57Z

@dantownsend Now that FastAPI is upgraded there is no need to exclude Piccolo Admin. Just need to add ADMIN_DEPENDENCIES to esmerald and add create_admin to Esmerald application class like this

# in _esmerald_app.py.jinja
from piccolo_admin.endpoints import create_admin
...
app = Esmerald(
    routes=[
        Gateway("/", handler=home),
        Gateway("/tasks", handler=TaskAPIView),
        Include(
            "/admin/",
            create_admin(
                tables=APP_CONFIG.table_classes,
                # Required when running under HTTPS:
                # allowed_hosts=['my_site.com']
            ),
        ),
    ],
    static_files_config=StaticFilesConfig(path="/static", directory=Path("static")),
    on_startup=[open_database_connection_pool],
    on_shutdown=[close_database_connection_pool],
)

and we can use Piccolo Admin in Esmerald template.

dantownsend added 3 commits December 12, 2023 11:26

remove jinja2

d7d054c

Piccolo has `jinja2` as a direct dependency, so this shouldn't be needed.

remove requeste

985513c

I think this was needed for Starlette at some point, but it has since transitioned to `httpx`, so I don't think it's needed any more.

move piccolo_admin to ROUTER_DEPENDENCIES

3d9b881

We don't use it for `esmerald` so don't include it by default for all routers.

dantownsend added the enhancement New feature or request label Dec 12, 2023

dantownsend commented Dec 12, 2023

View reviewed changes

dantownsend requested a review from sinisaos December 12, 2023 11:42

sinisaos reviewed Dec 12, 2023

View reviewed changes

sinisaos mentioned this pull request Dec 15, 2023

Upgrade SwaggerUI to version 5 Neoteroi/BlackSheep#447

Closed

dantownsend added 2 commits January 23, 2024 20:59

no longer omit piccolo_admin for esmerald

f06384d

add piccolo_admin to _esmerald_app.py.jinja

c0d6413

dantownsend merged commit 6ec06e7 into master Jan 23, 2024
46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ASGI requirements #911

Update ASGI requirements #911

dantownsend commented Dec 12, 2023

codecov-commenter commented Dec 12, 2023 •

edited

Loading

dantownsend Dec 12, 2023 •

edited

Loading

dantownsend Dec 12, 2023

sinisaos commented Dec 12, 2023

dantownsend commented Dec 12, 2023

sinisaos left a comment •

edited

Loading

dantownsend commented Dec 13, 2023

sinisaos commented Dec 13, 2023

sinisaos commented Dec 13, 2023

sinisaos commented Dec 15, 2023

dantownsend commented Dec 15, 2023

sinisaos commented Dec 27, 2023

tarsil commented Dec 27, 2023

sinisaos commented Dec 27, 2023

tarsil commented Dec 27, 2023

sinisaos commented Dec 27, 2023

tarsil commented Dec 27, 2023

dantownsend commented Dec 27, 2023

sinisaos commented Dec 27, 2023

Update ASGI requirements #911

Update ASGI requirements #911

Conversation

dantownsend commented Dec 12, 2023

codecov-commenter commented Dec 12, 2023 • edited Loading

Codecov Report

dantownsend Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

dantownsend Dec 12, 2023

Choose a reason for hiding this comment

sinisaos commented Dec 12, 2023

dantownsend commented Dec 12, 2023

sinisaos left a comment • edited Loading

Choose a reason for hiding this comment

dantownsend commented Dec 13, 2023

sinisaos commented Dec 13, 2023

sinisaos commented Dec 13, 2023

sinisaos commented Dec 15, 2023

dantownsend commented Dec 15, 2023

sinisaos commented Dec 27, 2023

tarsil commented Dec 27, 2023

sinisaos commented Dec 27, 2023

tarsil commented Dec 27, 2023

sinisaos commented Dec 27, 2023

tarsil commented Dec 27, 2023

dantownsend commented Dec 27, 2023

sinisaos commented Dec 27, 2023

codecov-commenter commented Dec 12, 2023 •

edited

Loading

dantownsend Dec 12, 2023 •

edited

Loading

sinisaos left a comment •

edited

Loading