feat: add redis service and RPM traffic contract config and tracking - #259
noahpodgurski wants to merge 11 commits into
Conversation
| authorized_chat_request, result, time.perf_counter() - start_time | ||
| ) | ||
| record_chat_availability(authorized_chat_request, availability_reason) | ||
| await redis_service.update_contracts( |
There was a problem hiding this comment.
Suggestion:
maybe wrap it in try except
finally:
record_completion_latency(
authorized_chat_request, result, time.perf_counter() - start_time
)
record_chat_availability(authorized_chat_request, availability_reason)
try:
await redis_service.update_contracts(
service_type=authorized_chat_request.service_type, usage=usage
)
except Exception as exc:
logger.error(f"Traffic contract update failed: {exc}")
if not env.TRAFFIC_CONTRACT_FAIL_OPEN_ON_REDIS_ERROR:
raise
There was a problem hiding this comment.
forgot to add handling here, thank you 🙏
There was a problem hiding this comment.
I added the handling inside update_contracts and also converted the function to be ran as a task
asyncio.create_task(
redis_service.update_contracts(
service_type=authorized_chat_request.service_type,
usage=usage,
)
)
otherwise it blocks the data from being returned, so it's more efficient 👍
| app_attest_connected = True | ||
|
|
||
| if env.ENABLE_TRAFFIC_CONTRACT_ENFORCEMENT: | ||
| await redis_service.connect() |
There was a problem hiding this comment.
Comment: so now redis service will become a hard dependency, without it MLPA will not start
There was a problem hiding this comment.
Only if ENABLE_TRAFFIC_CONTRACT_ENFORCEMENT=true yes?
But LiteLLM won't work without it either
| elapsed_in_bucket = current_time % window_seconds | ||
| return window_seconds - elapsed_in_bucket | ||
|
|
||
| async def check_feature_traffic_contract( |
There was a problem hiding this comment.
Suspicious:
I think we can have a race condition ...
so I understood it as:
- requests comes and we run HGET to read current count, predicts count + 1
- we check if to allow the request or not
- we don't persists this predicts count + 1 value
- After LLM responds, we run increment_feature_traffic_contract runs and it does HINCRBY - pushing the value to Redis
but during the duration of the LLM response more calls will come...
Maybe we should persist the value on the before LLM call? 🤔
But for TPM calls it will not work
There was a problem hiding this comment.
That was my first approach, to check and increment in the same script. But like you said for TPM it doesn't work, that's why I split them both out. I figured it's better to have them unified in that way than for just the RPM check and increment be persisted pre-request, even if we have the capability for it
| service_type=authorized_chat_request.service_type, | ||
| model=authorized_chat_request.model, | ||
| ) | ||
| await enforce_traffic_contract(request, authorized_chat_request.service_type) |
There was a problem hiding this comment.
Suggestion: maybe register enforce_traffic_contract into MIDDLEWARE_EXECUTION_ORDER
There was a problem hiding this comment.
I added a bit about this in the description 🙏
The traffic_contract_enforcer runs as a handler in the request after the authorize_chat/search_request function has run. This is slightly less optimal (than in the middleware) but it keeps the code cleaner and reduces duplicated (service type, purpose, etc...) validation code and logic that fastapi already provides - plus it will make TPM tracking logic identical. A tradeoff with this is we do not count 401 requests towards the total.
What's new
tl;dr
allowed=Falsewithmode="borrowed"ormode="degraded", and logged, and are emitted on metricrequests_totalviatraffic_contract_rpm_modeandtraffic_contract_tpm_mode. No soft degradation is applied yet.normal: feature and basket are within contractborrowed: feature is over contract, but basket still has roomdegraded: basket is over contractRedis
There's a new
redis_servicewhich uses the same Redis instance as LiteLLM, with themlpa:traffic_contractkey prefix. There's a discussion to be had around the current eviction policy and whether we should change it. Although we've had no evictions since inception, but it's something to keep in mind. It uses small Lua scripts to check fixed-window feature/basket state and increment RPM/TPM counters.Traffic contract design:
Traffic contracts are defined on a per-feature basis, so SW, S2S, etc... RPM and TPM limits are defined for each (ex:
SMART_WINDOW_TRAFFIC_CONTRACT_RPM_LIMIT. RPM and TPM limits for all features (all requests/basket) combined is stored inTOTAL_TRAFFIC_CONTRACT_RPM_LIMIT. I also renamedenv.user_feature_budgettoservice_type_configsince it contains non budget info (feature info). Each budget/service type is combined into theenv.traffic_contract_configwhich defines the RPM/TPM based off the service type's feature's definition. Not to be confused with the per user rpm/tpm limits defined in theservice_type_config (old user_feature_budget)The traffic_contract_enforcer runs as a handler in the request after the authorize_chat/search_request function has run. This is slightly less optimal (than in the middleware) but it keeps the code cleaner and reduces duplicated (service type, purpose, etc...) validation code and logic that fastapi already provides - plus it will make TPM tracking logic identical. A tradeoff with this is we do not count 401 requests towards the total.
After the upstream response,
redis_service.update_contractsincrements RPM and, when token usage is available, TPM.
The Redis key is a fixed-window bucket:Detailed example flow (expand)
{TRAFFIC_CONTRACT_REDIS_KEY_PREFIX}:rpm:{bucket_start_epoch}For example:
mlpa:traffic_contract:rpm:1789057680The key is a Redis hash. Each feature gets its own field, and the shared basket
uses
__basket__.Example assumptions:
TRAFFIC_CONTRACT_RPM_WINDOW_SECONDS=60TRAFFIC_CONTRACT_TPM_WINDOW_SECONDS=60TRAFFIC_CONTRACT_COUNTER_TTL_SECONDS=120smart-windowfeature RPM limit =2smart-windowfeature TPM limit =5000410000aiandmemoriesboth map tosmart-windows2smaps tos2ss2s-androidmaps tos2s-androidtotal_tokensDiagram link
This means the whole shared basket is now over contract. The request still
continues for now, but this is the signal we can use later to degrade behavior:
reduce max tokens, skip retries, route differently, or apply a short wait/jitter.
When the next minute starts, Redis writes to a new key:
The previous bucket stays around briefly because the TTL is longer than the
window, which makes inspection/debugging easier without affecting enforcement.
If not enforcing limits, why not just create alerts based on this RPM/TPM data we already have in Grafana?
Even though we're not enforcing a 429 on requests that go over the traffic contract, the request metric now includes
traffic_contract_rpm_modeandtraffic_contract_tpm_mode, which provides insight into the percentage of traffic isnormal,borrowed, ordegradedand tune contract numbers from there.https://mozilla-hub.atlassian.net/browse/AIPLAT-1060
https://mozilla-hub.atlassian.net/browse/AIPLAT-1064