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

Port Datasette to ASGI #272

Closed
simonw opened this issue May 17, 2018 · 42 comments

Comments

Projects
None yet
2 participants
@simonw
Copy link
Owner

commented May 17, 2018

Datasette doesn't take much advantage of Sanic, and I'm increasingly having to work around parts of it because of idiosyncrasies that are specific to Datasette - caring about the exact order of querystring arguments for example.

Since Datasette is GET-only our needs from a web framework are actually pretty slim.

This becomes more important as I expand the plugins #14 framework. Am I sure I want the plugin ecosystem to depend on a Sanic if I might move away from it in the future?

If Datasette wasn't all about async/await I would use WSGI, but today it makes more sense to use ASGI. I'd like to be confident that switching to ASGI would still give me the excellent performance that Sanic provides.

https://github.com/django/asgiref/blob/master/specs/asgi.rst

@simonw simonw added the medium label May 17, 2018

@simonw

This comment has been minimized.

Copy link
Owner Author

commented May 22, 2018

I think I can do this almost entirely within my existing BaseView class structure.

First, decouple the async data() methods by teaching them to take a querystring object as an argument instead of a Sanic request object. The get() method can then send that new object instead of a request.

Next teach the base class how to obey the ASGI protocol.

I should be able to get support for both Sanic and uvicorn/daphne working in the same codebase, which will make it easy to compare their performance.

@simonw

This comment has been minimized.

Copy link
Owner Author

commented May 25, 2018

Thinking about this further, maybe I should embrace ASGI turtles-all-the-way-down and teach each datasette view class to take a scope to the constructor and act entirely as an ASGI component. Would be a nice way of diving deep into ASGI and I can add utility helpers for things like querystring evaluation as I need them.

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

Results of an extremely simple micro-benchmark comparing the two shows that uvicorn is at least as fast as Sanic (benchmarks a little faster with a very simple payload): https://gist.github.com/simonw/418950af178c01c416363cc057420851

simonw added a commit that referenced this issue Jun 5, 2018

Test client wrapper removing need for gather_request - refs #272
As part of decoupling from Sanic, this will make it easier to run tests
against ASGI instead.
@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 5, 2018

https://github.com/encode/uvicorn/blob/572b5fe6c811b63298d5350a06b664839624c860/uvicorn/run.py#L63 is how you start a Uvicorn server from code as opposed to the uvicorn CLI

from uvicorn.run import UvicornServer
UvicornServer().run(app, host=host, port=port)
@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 26, 2018

This looks VERY relevant: https://github.com/encode/starlette

@tomchristie

This comment has been minimized.

Copy link

commented Jun 27, 2018

I’m up for helping with this.

Looks like you’d need static files support, which I’m planning on adding a component for. Anything else obviously missing?

For a quick overview it looks very doable - the test client ought to me your test cases stay roughly the same.

Are you using any middleware or other components for the Sanic ecosystem? Do you use cookies or sessions at all?

@simonw simonw added the feature label Jul 10, 2018

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jul 10, 2018

No cookies or sessions - no POST requests in fact, Datasette just cares about GET (path and querystring) and being able to return custom HTTP headers.

@tomchristie

This comment has been minimized.

Copy link

commented Jul 12, 2018

Okay. I reckon the latest version should have all the kinds of components you'd need:

Recently added ASGI components for Routing and Static Files support, as well as making few tweaks to make sure requests and responses are instantiated efficiently.

Don't have any redirect-to-slash / redirect-to-non-slash stuff out of the box yet, which it looks like you might miss.

@simonw simonw modified the milestones: Datasette 1.0, Next release Jul 24, 2018

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jul 26, 2018

I'm now hacking around with an initial version of this in the starlette branch.

Here's my work in progress, deployed using datasette publish now fixtures.db -n datasette-starlette-demo --branch=starlette --extra-options="--asgi"

https://datasette-starlette-demo.now.sh/

Lots more work to do - the CSS isn't being served correctly for example, it's showing this error when I hit /-/static/app.css:

INFO: 127.0.0.1 - "GET /-/static/app.css HTTP/1.1" 200
ERROR: Exception in ASGI application
Traceback (most recent call last):
  File "/Users/simonw/Dropbox/Development/datasette/venv/lib/python3.6/site-packages/uvicorn/protocols/http/httptools_impl.py", line 363, in run_asgi
    result = await asgi(self.receive, self.send)
  File "/Users/simonw/Dropbox/Development/datasette/venv/lib/python3.6/site-packages/starlette/staticfiles.py", line 91, in __call__
    await response(receive, send)
  File "/Users/simonw/Dropbox/Development/datasette/venv/lib/python3.6/site-packages/starlette/response.py", line 180, in __call__
    {"type": "http.response.body", "body": chunk, "more_body": False}
  File "/Users/simonw/Dropbox/Development/datasette/venv/lib/python3.6/site-packages/uvicorn/protocols/http/httptools_impl.py", line 483, in send
    raise RuntimeError("Response content shorter than Content-Length")
RuntimeError: Response content shorter than Content-Length
@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jul 26, 2018

It looks like that's a bug in Starlette - filed here: encode/starlette#32

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jul 26, 2018

Tom shipped my fix for that bug already, so https://datasette-starlette-demo.now.sh/ is now serving CSS!

@simonw simonw changed the title Decouple Datasette from Sanic (with ASGI) Port Datasette to ASGI Jul 27, 2018

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jul 27, 2018

@tomchristie

This comment has been minimized.

Copy link

commented Sep 5, 2018

Some notes:

  • Starlette just got a bump to 0.3.0 - there's some renamings in there. It's got enough functionality now that you can treat it either as a framework or as a toolkit. Either way the component design is all just here's an ASGI app all the way through.
  • Uvicorn got a bump to 0.3.3 - Removed some cyclical references that were causing garbage collection to impact performance. Ought to be a decent speed bump.
  • Wrt. passing config - Either use a single envvar that points to a config, or use multiple envvars for the config. Uvicorn could get a flag to read a .env file, but I don't see ASGI itself having a specific interface there.

russss added a commit to russss/datasette that referenced this issue Apr 28, 2019

Add inspect and prepare_sanic hooks
This adds two new plugin hooks:

The `inspect` hook allows plugins to add data to the inspect
dictionary.

The `prepare_sanic` hook allows plugins to hook into the web
router. I've attached a warning to this hook in the docs in light
of simonw#272 but I want this hook now...

On quick inspection, I don't think it's worthwhile to try and make
this hook independent of the web framework (but it looks like Starlette
would make the hook implementation a bit nicer).

Ref simonw#14

russss added a commit to russss/datasette that referenced this issue Apr 29, 2019

Add inspect and prepare_sanic hooks
This adds two new plugin hooks:

The `inspect` hook allows plugins to add data to the inspect
dictionary.

The `prepare_sanic` hook allows plugins to hook into the web
router. I've attached a warning to this hook in the docs in light
of simonw#272 but I want this hook now...

On quick inspection, I don't think it's worthwhile to try and make
this hook independent of the web framework (but it looks like Starlette
would make the hook implementation a bit nicer).

Ref simonw#14

@simonw simonw removed this from the Next release milestone May 13, 2019

@simonw

This comment has been minimized.

Copy link
Owner Author

commented May 21, 2019

Wow, this issue has been open for a full year now!

I've been thinking about this a lot. I've decided I want Datasette to use ASGI 3.0 internally with no dependencies on anything else - then I want the option to run Datasette under both daphne and uvicorn - because uvicorn doesn't support Python 3.5 but Datasette still needs to (primarily for Glitch), and daphne works with 3.5.

So I'm going to try to go the following route:

  • Every Datasette view becomes an ASGI app
  • The Datasette application itself is an ASGI app that routes to those views
  • When you pip install datasette you get Daphne as a dependency (I'd like you to be able to opt-out of installing Daphne, I'm not yet sure how that would work)
  • A new asgi_serve plugin hook allows a plugin to serve Datasette using uvicorn (or hypercorn) instead
@simonw

This comment has been minimized.

Copy link
Owner Author

commented May 21, 2019

I said earlier that I only need to support GET - I actually need to be able to support POST too, mainly to support plugins (e.g. a plugin that allows authenticated login before you can view Datasette, but potentially also plugins that let you write data directly to SQLite as well).

simonw added a commit that referenced this issue Jun 23, 2019

Include "asgi": "3.0" in /-/versions, refs #272
Mainly so you can tell if a Datasette instance is running
on ASGI or not.

simonw added a commit that referenced this issue Jun 23, 2019

simonw added a commit that referenced this issue Jun 23, 2019

Database download works again, refactored utils.py #272
Refactored utils.py into a datasette/utils package, refactored some of
the ASGI helper code into datasette/utils/asgi.py

simonw added a commit that referenced this issue Jun 23, 2019

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 23, 2019

All of the tests are now passing!

I still need a solution for this:

datasette/datasette/app.py

Lines 706 to 714 in 5bd510b

app = DatasetteRouter(routes)
# First time server starts up, calculate table counts for immutable databases
# TODO: re-enable this mechanism
# @app.listener("before_server_start")
# async def setup_db(app, loop):
# for dbname, database in self.databases.items():
# if not database.is_mutable:
# await database.table_counts(limit=60 * 60 * 1000)

I think the answer is ASGI lifespan, which is supported by Uvicorn. https://asgi.readthedocs.io/en/latest/specs/lifespan.html#startup

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 23, 2019

I also need to actually take advantage of raw_path such that pages like https://fivethirtyeight.datasettes.com/fivethirtyeight/twitter-ratio%2Fsenators can be correctly served.

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 23, 2019

Tests are failing on Python 3.5: https://travis-ci.org/simonw/datasette/jobs/549380098 - error is TypeError: the JSON object must be str, not 'bytes'

simonw added a commit that referenced this issue Jun 23, 2019

simonw referenced this issue Jun 23, 2019

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 23, 2019

And now the tests are all passing!

Still to do:

  • Use raw_path so table names containing / can work correctly
  • Get ?_trace=1 working again
  • Replacement for @app.listener("before_server_start")
  • Replace Sanic request object with my own request class, so I can remove Sanic dependency
@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 23, 2019

I'm going to move the remaining work into a pull request.

@simonw simonw referenced this issue Jun 23, 2019

Merged

Port Datasette from Sanic to ASGI + Uvicorn #518

8 of 8 tasks complete

@simonw simonw added large and removed medium labels Jun 23, 2019

@simonw simonw added this to the Datasette 1.0 milestone Jun 23, 2019

simonw added a commit that referenced this issue Jun 23, 2019

simonw added a commit that referenced this issue Jun 23, 2019

simonw added a commit that referenced this issue Jun 23, 2019

Implemented ASGI lifespan #272
Also did a little bit of lint cleanup

simonw added a commit that referenced this issue Jun 24, 2019

Port Datasette from Sanic to ASGI + Uvicorn (#518)
Datasette now uses ASGI internally, and no longer depends on Sanic.

It now uses Uvicorn as the underlying HTTP server.

This was thirteen months in the making... for full details see the issue:

#272

And for a full sequence of commits plus commentary, see the pull request:

#518

simonw referenced this issue Jun 24, 2019

Install test dependencies so deploy can work
python tests/fixtures.py needs asgiref or it fails with an error
@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 24, 2019

It's alive! Here's the first deployed version: https://a559123.datasette.io/

You can confirm it's running under ASGI by viewing https://a559123.datasette.io/-/versions and looking for the "asgi" key.

Compare to the last version of master running on Sanic here: http://aa91112.datasette.io/

@simonw simonw closed this Jun 24, 2019

@simonw simonw unpinned this issue Jun 24, 2019

@simonw

This comment has been minimized.

Copy link
Owner Author

commented Jun 24, 2019

simonw added a commit that referenced this issue Jul 7, 2019

Revert "New encode/decode_path_component functions"
Refs #272

This reverts commit 9fdb47c.

Now that ASGI supports raw_path we don't need our own encoding scheme!

simonw added a commit that referenced this issue Jul 7, 2019

Port Datasette from Sanic to ASGI + Uvicorn (#518)
Datasette now uses ASGI internally, and no longer depends on Sanic.

It now uses Uvicorn as the underlying HTTP server.

This was thirteen months in the making... for full details see the issue:

#272

And for a full sequence of commits plus commentary, see the pull request:

#518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.