Add support for Esmerald

[tarsil]

• Adding support for Esmerald with Piccolo.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b8026da) 92.03% compared to head (f103f6e) 92.03%.
Report is 1 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     #907   +/-   ##
=======================================
  Coverage   92.03%   92.03%           
=======================================
  Files         107      107           
  Lines        8145     8145           
=======================================
  Hits         7496     7496           
  Misses        649      649           

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

[dantownsend]

@tarsil Thanks for this.

If you add esmerald to this list:

piccolo/piccolo/apps/asgi/commands/new.py

Line 13 in b8026da

ROUTERS = ["starlette", "fastapi", "blacksheep", "litestar"]

Then when someone uses piccolo asgi new it will show esmerald as an option.

If you need to lock esmerald to a certain version, you can do so using this:

piccolo/piccolo/apps/asgi/commands/new.py

Lines 14 to 16 in b8026da

	ROUTER_DEPENDENCIES = {
	"fastapi": ["fastapi>=0.100.0"],
	}

We run integration tests against all of the ASGI frameworks that Piccolo supports:

https://github.com/piccolo-orm/piccolo/blob/master/tests/apps/asgi/commands/test_new.py

We basically test that the generated Python code is valid, and we run a really simplistic ASGI server to make sure the app runs.

[tarsil]

Hi @dantownsend thank you for your message. I will have a look at all of this in the next days (not in my machine at the moment) but I was also wondering where I could do that test you mentioned. Great to know. Great ORM btw 🙂

EDIT: I was able to add the missing one. Is this ok now?

[dantownsend]

@tarsil Yep, that's it. The test will run in our CI now. If you ever need to run it locally you can use ./scripts/test-integration.sh.

[tarsil]

@dantownsend not sure what happened with that flow. Can we re-run it? Because the rest is passing

[dantownsend]

@tarsil It looks like there's a dependency issue. Piccolo Admin has FastAPI as a dependency, which itself depends on Starlette. FastAPI has very narrowly defined dependency versions which means it sometimes doesn't play well with other libraries which depend on Starlette etc. Which version of Starlette does esmerald need?

[tarsil]

@dantownsend I see. I know that limitation from Fast.

Esmerald uses the version 0.32.0.post1. Besides the lastest released today basically

[tarsil]

@dantownsend let me know your thoughts how to go from here then if you don't mind

[dantownsend]

@tarsil I think the problem is that esmerald needs starlette==0.32.0:

https://github.com/dymmond/esmerald/blob/34cb7cc77f42b40c7a9ef8566dc659439be750d6/pyproject.toml#L58C10-L58C10

But the latest FastAPI only supports starlette==0.28.0:

https://github.com/tiangolo/fastapi/blob/8cf2fa0fe4f8701411db779176cc4cfac68d4216/pyproject.toml#L43

So when trying to pip install all the dependencies, it's using really old versions of Piccolo and FastAPI (the older versions specify their dependencies looser, so are considered OK by pip, but actually don't run correctly together).

I think the only solutions are:

• esmerald supporting a broader range of versions for starlette
• fastapi releasing a new version which supports a recent version of starlette (it will happen eventually, but we're not in control of when)
• Remove fastapi from Piccolo Admin, but that would be quite a big refactor.
• Don't use Piccolo Admin in the esmerald template - which removes fastapi as a dependency

It's a shame that FastAPI is like this, because it makes it makes it harder for libraries based on starlette to interoperate.

[tarsil]

@dantownsend thank you for this detailed explanation. I will be releasing a version soon and I can lose some tight versions and put it in a range. This way it will allow the same version of Starlette to be installed 🙂.

I will let you know once it's released and maybe we can rerun the CI?

It's great piccolo admin to use FastAPI actually. While I do this change for a release, if you want to have a look at Esmerald just for the knowledge, feel free and I would appreciate any constructive feedback anyway 🙂

[dantownsend]

@tarsil ok, cool - let me know when the new version is out, and I'll rerun the CI. I'll have a look at the docs 👍

[tarsil]

@dantownsend I did a new release today and made the requirements more broad, from >=0.28.0,<=0.32.0.post1. I think this should now do the trick?

[dantownsend]

@tarsil Thanks, I'll re-run the CI now.

[tarsil]

@dantownsend is still the same issue happening?

[dantownsend]

@tarsil Yeah, it's another dependency clash.

If you make a virtualenv, and make this requirements.txt file:

esmerald==2.4.3
piccolo[postgres]>1.0.0
piccolo_admin>1.0.0

pip install -r requirements.txt can't resolve it - I think the problem now is Pydantic being too tightly defined.

[sinisaos]

@dantownsend Sorry to interfere into the conversation, but I think you're right that we'll have to wait for FastAPI to upgrade the Starlette version (and there's nothing we can do about that). I had no problems with Pydantic and the only thing I had to do was remove the Piccolo admin from the Esmerald template which removes FastAPI as a dependency (and some other small changes I wrote suggestions for) and then the integration tests passed locally for me.

@tarsil I left some suggestions for PR. You should also add Esmerald to endpoints.py.jinja like this.

{% if router in ['fastapi', 'starlette'] %}
    {% include '_starlette_endpoints.py.jinja' %}
{% elif router == 'blacksheep' %}
    {% include '_blacksheep_endpoints.py.jinja' %}
{% elif router == 'litestar' %}
    {% include '_litestar_endpoints.py.jinja' %}
{% elif router == 'esmerald' %}
    {% include '_esmerald_endpoints.py.jinja' %}
{% endif %}

After these suggested changes (you should also remove the Piccolo Admin import and Include Piccolo Admin in the routes), the integration tests should pass.
Hope this helps.

[tarsil]

@sinisaos thank you very much for the suggestions above. Odd as my local version has something like you suggested. Maybe I actually didn't push what I needed before but either way, this is great. Thank you so much. I will do the changes in the next few days

[tarsil]

@sinisaos i applied your suggestions. I confirm that in my local I actually had similar but not pushed since the MediaType is actually inside the enums but we actually don't use it here. The last suggestion, if I'm not mistaken it was already in the PR but I appreciate that. @dantownsend i think with these changes suggested by @sinisaos , we can try again after I ran these locally, if you don't mind? I will come back you soon and maybe we could try again.

Although it won't hurt trying again here anyway in the meantime.

[sinisaos]

@tarsil No problem. I'm always glad if I can help somehow. As for Piccolo Admin, unfortunately, I think we have to wait for the FastAPI upgrade, which is not always very fast.

The last suggestion, if I'm not mistaken it was already in the PR but I appreciate that.

You're right. I wrote it wrong (but I thought), you have to add Esmerald here as well

Also remove the Piccolo Admin import and Include the Piccolo Admin in the routes in here, otherwise it won't work and the integration test will fail.

[tarsil]

@tarsil No problem. I'm always glad if I can help somehow. As for Piccolo Admin, unfortunately, I think we have to wait for the FastAPI upgrade, which is not always very fast.

The last suggestion, if I'm not mistaken it was already in the PR but I appreciate that.

You're right. I wrote it wrong (but I thought), you have to add Esmerald here as well

Also remove the Piccolo Admin import and Include the Piccolo Admin in the routes in here, otherwise it won't work and the integration test will fail.

Will definitely do @sinisaos thank you once again

[tarsil]

@sinisaos I'm actually still having some issues locally. How it does look like in your final esmerald template? Maybe I missed something, if you don't mind sharing.

In theory I did remove the Include and the create_admin but my local still blows. Unless you were referring to something else? I have pushed if you want to have a look here. Appreciate the help, really 😄

[sinisaos]

@tarsil You also need to add Emserald here like this

{% if router == 'fastapi' %}
    {% include '_fastapi_app.py.jinja' %}
{% elif router == 'starlette' %}
    {% include '_starlette_app.py.jinja' %}
{% elif router == 'blacksheep' %}
    {% include '_blacksheep_app.py.jinja' %}
{% elif router == 'litestar' %}
    {% include '_litestar_app.py.jinja' %}
{% elif router == 'esmerald' %}
    {% include '_esmerald_app.py.jinja' %}
{% endif %}

to add app.py for Esmerald template. After that run ./scripts/test-integration.sh

[tarsil]

@tarsil You also need to add Emserald here like this

{% if router == 'fastapi' %}
    {% include '_fastapi_app.py.jinja' %}
{% elif router == 'starlette' %}
    {% include '_starlette_app.py.jinja' %}
{% elif router == 'blacksheep' %}
    {% include '_blacksheep_app.py.jinja' %}
{% elif router == 'litestar' %}
    {% include '_litestar_app.py.jinja' %}
{% elif router == 'esmerald' %}
    {% include '_esmerald_app.py.jinja' %}
{% endif %}

to add app.py for Esmerald template. After that run ./scripts/test-integration.sh

Ahhhh, yes, totally missed that one and yet you mentioned but I was mistaken with another file. Done! If we could run the CI here to see, that would be great @sinisaos @dantownsend .

Hopefully now we can merge this and once FastAPI moves some of the versions up, we can add the admin as well. Thank you very much for your help @sinisaos , you are a ⭐ . I used Piccolo with Esmerald a few versions ago (of Esmerald) and I simply loved it.

The tests passed now locally.

[sinisaos]

@tarsil I'm glad you made it 😃 . For re-run in CI you must wait for @dantownsend. Just two more small thing.
First, you must replace

static_files_config=StaticFilesConfig(path="/", directory=Path("static")),

to

static_files_config=StaticFilesConfig(path="/static", directory=Path("static")),

in Esmerald application class, to make static files work.
The other thing is that you should shorten the path in the TaskAPIView methods to something like this @put("/{task_id}"),
because /tasks/tasks/1 looks a little weird.
Thanks.

[tarsil]

@tarsil I'm glad you made it 😃 . For re-run in CI you must wait for @dantownsend. Just two more small thing. First, you must replace

static_files_config=StaticFilesConfig(path="/", directory=Path("static")),

to

static_files_config=StaticFilesConfig(path="/static", directory=Path("static")),

in Esmerald application class, to make static files work. The other thing is that you should shorten the path in the TaskAPIView methods to something like this @put("/{task_id}"), because /tasks/tasks/1 looks a little weird. Thanks.

Oh, true, I was doing an experiment in the static files config and forgot to add back the /static, nice catch (been doing so many things that I miss a lot sometimes).

The tasks path should now be correct. Nice suggestions @sinisaos .

I'm actually happily impressed how clear you are with Esmerald 😃

[sinisaos]

@tarsil Great. Thanks for the kind words.

[tarsil]

@tarsil Great. Thanks for the kind words.

I'm the one who needs to thank you. Without your help I would take longer to get this done 🙂.

I hope you like Esmerald to be honest 🙂

[dantownsend]

It looks like the tests are passing. @sinisaos thanks for your help with this.

So it looks like Piccolo Admin was omitted to get around the dependency issues, which I think makes sense.

But as Piccolo Admin is in requirements.txt.jinja:

https://github.com/tarsil/piccolo/blob/master/piccolo/apps/asgi/commands/templates/app/requirements.txt.jinja

It still gets installed. I checked the logs for the integration tests, and it installed a really old version of Piccolo Admin. As esmerald isn't using Piccolo Admin, it no longer throws an error when running the esmerald app. However, I think it's just better to remove piccolo_admin from requirements.txt.jinja.

We could do something simple like:

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

There might be a cleaner way of doing this. In here instead:

https://github.com/tarsil/piccolo/blob/b8026da68112686952a10ff2db3f37f4d0fdf1bd/piccolo/apps/asgi/commands/new.py#L13-L16

It could be:

ROUTERS = ["starlette", "fastapi", "blacksheep", "litestar"]
ROUTER_DEPENDENCIES = {
    "starlette": ["starlette", "piccolo_admin"],
    "fastapi": ["fastapi>=0.100.0", "piccolo_admin"],
    "blacksheep": ["blacksheep", "piccolo_admin"],
    "litestar": ["litestar", "piccolo_admin"],
    "esmerald": ["esmerald"],
}

I noticed that requirements.txt.jinja has jinja2 and requests listed.

https://github.com/tarsil/piccolo/blob/b8026da68112686952a10ff2db3f37f4d0fdf1bd/piccolo/apps/asgi/commands/templates/app/requirements.txt.jinja#L5-L8

We might be able to remove both of these. jinja2 is already a requirement of piccolo, and I think requests was in there for an older version of starlette.

[tarsil]

@dantownsend based what you have said, which makes sense, what do you say you create a code change suggestion and we can simply merge it? I'm sure you are more familiar with this procedure than I am and this way we can move forward?

Or we simply merge this and open another PR to simply do those changes since it might imply some other aspects that we might have not caught up so far

[sinisaos]

@dantownsend Unfortunately this does not run the tests for Esmerald

It could be:

ROUTERS = ["starlette", "fastapi", "blacksheep", "litestar"]
ROUTER_DEPENDENCIES = {
    "starlette": ["starlette", "piccolo_admin"],
    "fastapi": ["fastapi>=0.100.0", "piccolo_admin"],
    "blacksheep": ["blacksheep", "piccolo_admin"],
    "litestar": ["litestar", "piccolo_admin"],
    "esmerald": ["esmerald"],
}

I think this is much better:

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

so requirements.txt.jinja should look something like this

{%- for router_dependency in router_dependencies -%}
{{ router_dependency }}
{% endfor -%}
{{ server }}
piccolo[postgres]
{% if not 'esmerald' in router_dependencies %}
piccolo_admin
{% endif %}

This runs the tests but does not install Piccolo Admin for Esmerald.

[tarsil]

If @dantownsend is happy with that, @sinisaos you can add as suggestion for change as well

[sinisaos]

@tarsil You'll have to wait for @dantownsend . If that's good enough, you or @dantownsend need to add it because requirements.txt.jinja is not in your PR and I can't add suggestions.

[tarsil]

@sinisaos of course! Thank you, if @dantownsend agrees, I can add it to the PR.

[dantownsend]

@tarsil @sinisaos I'll merge this in now - will make the changes to requirements.txt in another PR. Thanks both 👍