feat(llm-api-gateway): add input/output token rate limit dimensions - #1658
Draft
Max-NV wants to merge 4 commits into
Draft
feat(llm-api-gateway): add input/output token rate limit dimensions#1658Max-NV wants to merge 4 commits into
Max-NV wants to merge 4 commits into
Conversation
Adds independent input (prompt) and output (completion) token rate limits alongside the existing combined tokenRateLimit, in the same "<value>-<unit>" format across all S/M/H/D/W windows. Reads the new ModelSpec.inputTokenRateLimit/outputTokenRateLimit fields returned by the LlmGateway auth response and enforces them per-request; a companion NVCF API (nvcf-core) change to set those fields is tracked separately. Relates to NVIDIA#1456
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe gateway adds separate input- and output-token rate limits across model specifications, resource enforcement, limit resolution, admission-key generation, and token-stat selection. ChangesToken rate-limit support
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This adds independent input- and output-token quota support while preserving combined-token reporting precedence. Production behavior remains unchanged until upstream model specifications populate the new fields, and no merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant ModelSpec
participant CallerLimitResolver
participant doResourceLimit
participant chooseTokenStats
ModelSpec->>CallerLimitResolver: input/output token rate-limit values
CallerLimitResolver->>doResourceLimit: populated ResourceLimit dimensions
doResourceLimit->>doResourceLimit: evaluate input/output token quotas
doResourceLimit->>chooseTokenStats: token-limit results
chooseTokenStats->>chooseTokenStats: aggregate by period and select statistics
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… headers chooseTokenStats returned on the first configured input/output period (S/M/H/D/W) with any result, so a loose finer-period limit (e.g. 1000-S) could hide a tighter, actually-binding coarser one (e.g. 100-M) from the X-RateLimit-Tokens-* response headers, even though the coarser one still enforces server-side. Pick whichever configured period pair has the smallest remaining value instead, so the headers always reflect the constraint closest to throttling the caller. Found in adversarial review of NVIDIA#1658. Relates to NVIDIA#1456
parseLLMModelString and parseLLMModelUpdateString rejected inputTokenRateLimit/outputTokenRateLimit with "unknown llm model field", so the CLI had no way to set the fields added for NVIDIA#1456 short of hand-writing the REST JSON body. Adds the two fields through the full path: CLI flag parsing, validation (reusing the existing <value>-<unit> format check, now with the field name in error text), and the client DTOs sent to the NVCF API. Found in adversarial review of NVIDIA#1658. Relates to NVIDIA#1456
Approximates a month as a fixed 30 days, same as the existing D/W units, rather than a calendar-aligned window: the leaky-bucket limiter only understands time.Duration, not calendar dates. Applies to the combined tokenRateLimit and both input/output limits, so it fans out through the same dimension/struct/switch pattern as the S/M/H/D/W units: resources.go dimensions and struct fields, doResourceLimit switch cases, the 3 ResourceLimitFrom* mappers, parseTokenRateLimit, CallerLimitResolver, and chooseTokenStats. nvcf-cli's --llm-model / --llm-model-update token rate limit validation gets the same "MO" unit. Testing a month-long window doesn't need to wait a month: the limiter takes an injectable clock (see ratelimit_test.go's TestBucketsRefillOverTime for the existing pattern), so TestDoResourceLimitMonthlyLimitResetsAfterPeriodElapses just fast-forwards the fake clock by 31 days. Relates to NVIDIA#1456
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Issue #1456 asks for input (prompt) and output (completion) token rate
limits, separate from the existing combined
tokenRateLimit, usingthe same format. The gateway already had partial plumbing for an
input/output split (per-minute-only dimensions, never populated from
config), but nothing let a model actually configure it, and nothing
enforced anything but a per-minute window.
What changed
ratelimit/resources.go: extends the input/output token dimensionsfrom per-minute-only to the full second/minute/hour/day/week set,
mirroring the existing combined
TokensPer*dimensions.api/ratelimit.go:CallerLimitResolvernow parsesModelSpec.InputTokenRateLimit/OutputTokenRateLimit(same"<value>-<unit>"format astokenRateLimit) and applies themindependently of the combined limit;
chooseTokenStatspickswhichever configured input/output period is closest to being
exhausted for the
X-RateLimit-Tokens-*response headers, insteadof always the finest configured period (which could hide a tighter,
actually-binding coarser one).
nvcf/pb/llm_gateway.proto(+ regenerated stubs),nvcf/types.go,nvcf/client.go: addinputTokenRateLimit/outputTokenRateLimitto
ModelSpec, read from theAuthLlmInvokeResponse.MO(month) added as a rate-limit unit, approximated as a fixed30 days (the limiter only understands
time.Duration, not calendardates) - applies to the combined limit and both input/output limits,
fanned out through the same dimension/struct/switch pattern as
S/M/H/D/W.
nvcf-cligets the same unit.src/clis/nvcf-cli:--llm-model/--llm-model-updatenow acceptinputTokenRateLimitandoutputTokenRateLimit, through parsing,validation, and the client DTOs sent to the NVCF API. Previously the
CLI rejected these fields outright with "unknown llm model field".
The NVCF API (nvcf-core) side that lets a function actually set these
two fields is a separate companion change: #1661. Until that lands,
the new
ModelSpecfields are simply always empty and this change isa no-op in production.
How the three limits interact
tokenRateLimit,inputTokenRateLimit, andoutputTokenRateLimitare independent constraints, not alternatives. All three can be
configured at once, each is tracked as its own counter/key, and a
request must satisfy all of them (AND, not OR) to be admitted:
combined
input+output <= tokenRateLimit, and separatelyinput <= inputTokenRateLimit,output <= outputTokenRateLimit.Whichever is hit first throttles the request.
Any of the three left unset is skipped entirely, not treated as a
limit of zero (
doResourceLimitonly checks a dimension when itsconfigured limit is non-zero). So a model can set only
tokenRateLimit(today's behavior, unchanged), only the input/output pair, or all
three together.
Testing
go test ./...insrc/invocation-plane-services/llm-api-gatewayandin
src/clis/nvcf-cli, both fully green, including new unit andend-to-end coverage for every new dimension, the header-stats
tightest-period fix, and the CLI field parsing/validation/mapping.
A month-long window doesn't need a month to test: the limiter takes an
injectable clock, so
TestDoResourceLimitMonthlyLimitResetsAfterPeriodElapsesjust fast-forwards a fake clock by 31 days.
References
Relates to #1456