Conversation
|
Closes #70 |
# Conflicts: # aqueduct/gateway/views/decorators.py
There was a problem hiding this comment.
Mostly complaints about AI-generated comments and docstrings, which are a PITA to read, so sorry if they come across as somewhat brusque. Nothing personal here :'D
The one with sync_to_async may be something more substantial, if we use Redis for caching (I guess it doesn't make any difference with the in-memory cache?) - but have a look at it yourself.
Oh, and speaking of AI-generated gibberish: the PR description mentions "the hot path no longer touches management_request", which - depending on what is actually meant by "the hot path" - may be wrong or misleading, because processing of a request still saves the Request object to the database at least once.
Which leads me to a (genuine) question, what the point of an AI-generated PR description is. If I, as a reviewer, need one, I can ask an LLM to generate it for me; it's much less time and effort than actually reading through one. This description is unnecessarily verbose, uses weird words and expressions, and, as I said, is a pain to read. I would find a shorter, but human-written (or at least human-polished) description much more helpful.
If that description is meant for agents and not humans, then just ignore the previous paragraph. 😁 Otherwise, I wouldn't mind discussing this topic.
Summary
Moves per-request rate limiting off SQL aggregates and onto the Django cache API, adds configurable hourly/daily quota backstops, and surfaces live quota usage in the UI (tokens page + admin). Implements #70 / #102.
Background
check_limitspreviously ran 2 DB aggregate queries overRequest(Count/Sum, plus a per-model count for the weighted request budget) on every gateway request. This PR replaces those queries with fixed minute/hour/day cache buckets keyed per token. TheRequesttable now serves usage analytics only — the hot path no longer touchesmanagement_request.What changed
gateway/rate_limiting.py(new) — cache-backed buckets ({"req", "in", "out"}) per(token, window), TTL2 × window. The weighted request budget (req, scaled by per-model multipliers) is reserved at check time; input/output tokens are recorded at completion (non-streaming inlog_request, streaming in_openai_stream). All writes run under a per-token advisorycache_lock; buckets are stored as pickled dicts, so the Redis float-incrlimitation is avoided entirely.hourly_limit_multiplier/daily_limit_multiplierfields onLimitMixin(Org/Team/UserProfile), resolved through the existing hierarchy (specific → org → settings default60/1440). Hour/day limits are derived from the per-minute limits × multiplier, so aNoneper-minute cap implies no hour/day cap.LimitMixin.clean()bounds multipliers to1–60/1–1440and enforcesdaily ≤ 24 × hourly. Migration0011_org_daily_limit_multiplier_and_more._rate_usage.html), fetched in onecache.get_manyper page (get_per_token_usage).AQUEDUCT_RATE_LIMIT_ENABLED(True),AQUEDUCT_RATE_LIMIT_LOCK_TTL_SECONDS(5),AQUEDUCT_HOURLY_LIMIT_MULTIPLIER(60),AQUEDUCT_DAILY_LIMIT_MULTIPLIER(1440).CACHESnow falls back toLocMemCacheunderDEBUGtoo.Request limit (6/min)style is preserved).Behavior changes to review
Requestrow).cache_lockcontention the check fails open and token recording is skipped (both logged) — no worse than today's read races.Testing
gateway/tests/test_rate_limiting.py— bucket write/read, weighted budget across models, hour/day rollover (60 reqs / 6 min → 61st blocked …/hour), hierarchy + settings-default multiplier resolution, lock-contention fail-open/skip.TokenLimitTest—cache.clear()isolation (DB deletion no longer resets counts); patched multiplier tests retargeted torate_limiting.get_model_request_limit_multiplier.management/tests—LimitMixin.clean()bounds validation (0/61/1441,daily > 24×hourlyrejected), tokens-page usage bars, admin hourly/daily columns.mypy+ pre-commit green.