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

TechEmpower benchmarks #31

Closed
tomchristie opened this issue Oct 13, 2018 · 16 comments
Closed

TechEmpower benchmarks #31

tomchristie opened this issue Oct 13, 2018 · 16 comments
Labels
good first issue Good for newcomers

Comments

@tomchristie
Copy link
Contributor

This’d be a good one for a contributor to jump on.

Adding TechEmpower benchmarks for Responder. I’d suggest copying the starlette case, and adapting it accordingly. https://github.com/TechEmpower/FrameworkBenchmarks/tree/master/frameworks/Python/starlette

PR would be against the TechEmpower repo, not here. They run continuos benchmarks against the suite, so the performance section could be updated once there’s a run that includes Responder.

You’ll want to use an async DB connector, as per the starlette case.

@5hirish
Copy link

5hirish commented Oct 13, 2018

Working on it, except I am getting 404s for requests defined following the first API request.

import responder

api = responder.API()

@api.route("/hello")
async def greet_world(req, resp):
    resp.text = f"Hello, world!"

@api.route("/bye")
async def bye_world(req, resp):
    resp.media = {"data": "Bye, world!"}

if __name__ == '__main__':
    api.run()

Here /hello returns 200 but /bye returns 404. If I change the sequence of declaration then /bye works.

@kennethreitz
Copy link
Owner

that's because you named your function the same thing twice

@5hirish
Copy link

5hirish commented Oct 13, 2018

@kennethreitz sorry, that was copy-paste typo, fixed it, but still, the issue is reproducible, can try running the app.py from my fork. Or am I declaring something wrong here?

@kennethreitz kennethreitz added the good first issue Good for newcomers label Oct 14, 2018
@5hirish
Copy link

5hirish commented Oct 14, 2018

@tomchristie I have created a PR on the TechEmpower repo: (Still, haven't resolved the issue mentioned above. Maybe it is happening only for my system...?) TechEmpower/FrameworkBenchmarks#4122

@sheb
Copy link
Contributor

sheb commented Oct 14, 2018

@5hirish I had the same problem with v.0.0.3, but seems to be fixed on master

@5hirish
Copy link

5hirish commented Oct 14, 2018

Thanks for informing... BTW I am getting this responder: ERROR: Framework is not accepting requests from client machine in the CI console might be due to the above issue I guess...In my local, at least the first API was working...Will have to wait for the next release.

@5hirish
Copy link

5hirish commented Oct 16, 2018

Tried running the docker file in local, to pin point the error in docker run. I executed the the gunicorn task separately
gunicorn app:app --log-level debug -k uvicorn.workers.UvicornWorker -c responder_conf.py
I am getting the following error:

Exception in 'lifespan' protocol
Traceback (most recent call last):
  File "/home/felix/.pyenv/versions/3.6.5/envs/pyw365/lib/python3.6/site-packages/uvicorn/lifespan.py", line 29, in run
    await self.asgi(self.receive, self.send)
  File "/home/felix/.pyenv/versions/3.6.5/envs/pyw365/lib/python3.6/site-packages/uvicorn/middleware/message_logger.py", line 51, in __call__
    inner = self.app(self.scope)
  File "/home/felix/.pyenv/versions/3.6.5/envs/pyw365/lib/python3.6/site-packages/responder/api.py", line 100, in __call__
    path = scope["path"]
KeyError: 'path'

@tomchristie
Copy link
Contributor Author

Make sure to update to the latest uvicorn.

You’re seeing an integration issue there as uvicorn is sending ASGI lifespan messages that responder doesn’t yet handle. However the latest version of uvicorn should deal with that gracefully.

@5hirish
Copy link

5hirish commented Oct 17, 2018

Will update the benchmarks when the PR is approved and merged. @tomchristie responder doesn't need Jinja dependency but the api.template() call was not taking template file path as an argument, hence reverted to external Jinja dependency and usage. If this will affect the performance any form then let me know.

@tomchristie
Copy link
Contributor Author

@5hirish good work. They have a continuous benchmarking suite running, tho it has a turnover time of 2 or 3 days. Check https://tfb-status.techempower.com/ and you should see their latest results soon enough.

@therumbler
Copy link

the Tech Empower benchmark has completed, and includes Responder's results now.
https://www.techempower.com/benchmarks/#section=test&runid=a0d523de-091b-4008-b15d-bd4c8aa25066

@5hirish
Copy link

5hirish commented Oct 25, 2018

@tomchristie can close this issue. PR: Update ReadMe #130

@kennethreitz
Copy link
Owner

@therumbler we were included in the last batch -- any idea why?

@tomchristie
Copy link
Contributor Author

I guess it must have just missed the round 17 cutoff.

@waghanza
Copy link
Contributor

waghanza commented Nov 8, 2018

@tomchristie I have tested tfb

leads me to a socket.gaierror: [Errno -2] Name or service not known

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

No branches or pull requests

6 participants