[ENH]: add rate limiting #1728
[ENH]: add rate limiting #1728
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
nicolasgere
requested review from
jeffchuber,
atroyn,
beggers and
tazarov
and removed request for
atroyn
| @@ -142,6 +143,7 @@ def __init__(self, settings: Settings): | |||
| allow_methods=["*"], | |||
| ) | |||
| self._app.add_exception_handler(QuotaError, self.quota_exception_handler) | |||
| self._app.add_exception_handler(RateLimitError, self.rate_limit_exception_handler) | |||
There was a problem hiding this comment.
Do we need to define a separate exception handler for each exception? Can the existing one be reused?
There was a problem hiding this comment.
I am following fastApi way of doing it, can be easily change if needed.
There was a problem hiding this comment.
Does it make sense to add this if it does not provide any value? I understand adding a simple implementation that does something bare minimum.
| @patch('chromadb.quota.test_provider.QuotaProviderForTest.get_for_subject', mock_get_for_subject) | ||
| @patch('chromadb.rate_limiting.test_provider.RateLimitingTestProvider.is_allowed', lambda self, key, quota, point=1: False) | ||
| def test_rate_limiting_should_raise(rate_limiting_gym: RateLimitingGym): | ||
| with pytest.raises(Exception) as exc_info: |
There was a problem hiding this comment.
a simple syntax for this is:
with pytest.raises(Exception,matches="FAKE_RESOURCE") as exc_info:
...| from chromadb.quota import QuotaProvider, Resource | ||
|
|
||
|
|
||
| class RateLimitError(Exception): |
There was a problem hiding this comment.
As a follow up to the new errors I think we should handle that on client-side with some sort of exponential back-off.
There was a problem hiding this comment.
Agreed but I don't think it needs to be done as part of this PR. In general I see client work as significantly less important than server work, especially as we move towards Cloud launch (I expect many customers to roll their own clients)
There was a problem hiding this comment.
I see client work as significantly less important than server work, especially as we move towards Cloud launch (I expect many customers to roll their own clients)
I don't agree with this. The client is part of chromas experience. Cloud is targeted at developers who are in the same usage bracket and the client is a first-party part of that experience.
There was a problem hiding this comment.
I agree with @HammadB here - our initial target user for cloud is not likely to roll their own.
There was a problem hiding this comment.
Adding a task to discuss client implication.
| from chromadb.quota import QuotaProvider, Resource | ||
|
|
||
|
|
||
| class RateLimitError(Exception): |
There was a problem hiding this comment.
Agreed but I don't think it needs to be done as part of this PR. In general I see client work as significantly less important than server work, especially as we move towards Cloud launch (I expect many customers to roll their own clients)
nicolasgere
force-pushed
the
nicolas/rate-limit
branch
from
b494eaf
to
70b8bf1
Compare
## Description of changes *Summarize the changes made by this PR.* - New functionality - Add rate limiting service. If no rate limit service is provided, it will not do anything. ## Test plan *How are these changes tested?* Unit test on rate limiting service. --------- Co-authored-by: nicolas <nico@trychroma.com>
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
Unit test on rate limiting service.
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?