Change the feast serve endpoint to be sync rather than async. #2119
Conversation
|
Hi @nossrannug. Thanks for your PR. I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report
@@ Coverage Diff @@
## master #2119 +/- ##
=======================================
Coverage 84.49% 84.49%
=======================================
Files 101 101
Lines 8107 8107
=======================================
Hits 6850 6850
Misses 1257 1257
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, nossrannug The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| app = get_app(store) | ||
| click.echo( | ||
| "This is an " | ||
| + click.style("experimental", fg="yellow", bold=True, underline=True) | ||
| + " feature. It's intended for early testing and feedback, and could change without warnings in future releases." | ||
| ) | ||
| uvicorn.run(app, host=host, port=port) | ||
| uvicorn.run(app, host=host, port=port, access_log=(not no_access_log)) |
There was a problem hiding this comment.
Hey @nossrannug , thanks for your PR. But I'm confused, what's the benefit of moving to the synchronous interface if we'll continue to use uvicorn as our server and at the end of the day we'll have a single-threaded app.
There was a problem hiding this comment.
Did you use some other server in your tests?
There was a problem hiding this comment.
Hey @pyalex,
We're not running a single-threaded app. FastAPI supports mixing async and sync methods(endpoints) as much as you want and it will take care of running it correctly. When a method is async it will be executed using the event loop, but when a method is synchronous it will be executed in a thread pool.
sync:
async:
|
I totally agree with raised problem, but the proposed solution looks strange to me. I think we either should remove async frameworks ( |
FastAPI does what you're suggesting when the method isn't async. |
|
Thanks for this clarification @nossrannug . In that case, this approach makes sense. Can you please fix conflicts? |
sdk/python/feast/registry.py
Outdated
| @@ -104,6 +104,7 @@ def __init__(self, registry_config: RegistryConfig, repo_path: Path): | |||
| repo_path: Path to the base of the Feast repository | |||
| or where it will be created if it does not exist yet. | |||
| """ | |||
| self.lock = Lock() | |||
There was a problem hiding this comment.
Should we have a more descriptive name for this variable, like refresh_lock. And can you please add _ as a prefix?
nossrannug
force-pushed
the
remove-feast-serve-async
branch
from
03d7acc
to
8a79c63
Compare
Resolved |
When an endpoint is async, everything happens on the event loop thread. This is great for functionality that is IO heavy since the job can be awaited and other requests processed in the meantime. But if there are no awaits, then everything happens synchronously on one thread which means that while we're waiting for a response from a DB or API call, nothing else gets processed. To address this I've removed the async keyword from the endpoint. This makes FastAPI process each request on a thread in a thread pool. Signed-off-by: Gunnar Sv Sigurbjörnsson <gunnar.sigurbjornsson@gmail.com>
nossrannug
force-pushed
the
remove-feast-serve-async
branch
from
8a79c63
to
a3b1798
Compare
|
/lgtm |
Signed-off-by: Gunnar Sv Sigurbjörnsson gunnar.sigurbjornsson@gmail.com
What this PR does / why we need it:
When an endpoint is async, everything happens on the event loop thread.
This is great for functionality that is IO heavy since the job can be
awaited and other requests processed in the meantime. But if there are
no awaits, then everything happens synchronously on one thread which
means that while we're waiting for a response from a DB or API call,
nothing else gets processed.
To address this I've removed the async keyword from the endpoint.
This makes FastAPI process each request on a thread in a thread pool.
Testing this with postgres as the online store, running everything locally,
and 1 worker, req/sec went from 200 to 800.
Does this PR introduce a user-facing change?: