feat(models): add configurable inference slowdown - #674
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds configurable virtual-model slowdown factors. The change validates and persists values, resolves them by alias and policy, delays inference and streaming responses, exposes admin API and dashboard controls, and documents the behavior. ChangesVirtual-model slowdown support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestModelResolution
participant VirtualModelsService
participant GatewayInference
participant Provider
Client->>RequestModelResolution: submit model request
RequestModelResolution->>VirtualModelsService: resolve slowdown
VirtualModelsService-->>RequestModelResolution: return factor
RequestModelResolution->>GatewayInference: execute with factor
GatewayInference->>Provider: call inference provider
Provider-->>GatewayInference: return result or stream
GatewayInference-->>Client: return after configured delay
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/audio_service.go (1)
91-105: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd table-driven tests for audio slowdown behavior.
Add coverage for speech, transcription, and translation. Verify the resolved route factor, delay after a successful provider call, and cancellation before the response is sent.
As per coding guidelines, “Add or update table-driven tests for behavior changes.”
Also applies to: 176-193, 233-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/audio_service.go` around lines 91 - 105, Add table-driven tests covering speech, transcription, and translation handlers, using the corresponding route slowdown configuration. Verify each test resolves the expected slowdown factor, applies the delay after a successful provider call, and handles context cancellation before sending the response; cover the success paths around CreateSpeech and the analogous transcription/translation provider calls.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/openapi.json`:
- Around line 11055-11058: Constrain the slowdown schema to accept only 0 or
numeric factors from 0.1 through 10. Apply this same OpenAPI constraint at
docs/openapi.json lines 11055-11058, 14398-14401, and 14454-14457, covering the
request, view response, and persisted virtual-model response definitions.
In `@internal/gateway/request_model_resolution_test.go`:
- Around line 115-140: Refactor
TestResolveRequestModelCarriesResolvedSlowdownFactor into table-driven cases
covering an alias factor, a direct model factor, and a resolver that does not
implement ModelSlowdownResolver. For each case, assert the expected resolved
slowdown and request-resolution result, while preserving the existing alias
resolver input assertions where applicable.
In `@internal/gateway/slowdown_test.go`:
- Around line 11-29: Refactor the waitForInferenceSlowdown tests into a
table-driven test covering nil workflow, zero slowdown factor, positive slowdown
factor, and canceled context cases. Store each case’s expected delay and error
in the table, then run them through the existing waitForInferenceSlowdown symbol
while preserving the current timing tolerance and error assertions.
In `@internal/streaming/slowdown_stream.go`:
- Line 87: Update the queueing logic in the slowdown stream implementation
around s.queue to enforce a bounded byte or chunk capacity before appending
upstream chunks. Apply backpressure or a clearly defined overflow failure
policy, and ensure blocked producers are released when the stream context is
cancelled.
- Line 33: Update the stream lifecycle around the goroutine started by go
s.drain() so parent-context cancellation closes the upstream source, ensuring
drain exits even when the provider reader ignores its request context. Wire
s.ctx.Done() to the stream/source close path while preserving normal reads, and
add a regression test verifying source.Close() is called and drain terminates
after cancellation.
In `@internal/virtualmodels/slowdown_test.go`:
- Around line 10-130: Convert the resolution and validation coverage in
internal/virtualmodels/slowdown_test.go (lines 10-130) into table-driven
subtests, covering valid boundaries, disabled zero, math.NaN(), positive and
negative infinity, alias precedence, and user-path scope; retain the existing
assertions for each case. Add table-driven configuration conversion cases in
internal/virtualmodels/config_overlay_test.go (lines 12-40), using the existing
test symbols and helpers; both sites require direct changes.
In `@internal/virtualmodels/slowdown.go`:
- Around line 25-33: Preserve explicit alias slowdown values by distinguishing
an omitted setting from configured zero across configuration, API, persistence,
and VirtualModel representations. Update the redirect lookup in the slowdown
resolution flow so it returns the alias slowdown whenever configured, including
zero, before evaluating matchingPolicy for the resolved concrete model.
In `@web/dashboard/src/pages/models/VirtualModelEditor.svelte`:
- Around line 182-192: Update VirtualModelEditor.svelte (182-192) to accept
slowdown 0 and state that zero or empty disables it; update .env.template
(205-206), config/config.example.yaml (64), and docs/features/virtual-models.mdx
(40-44, 166-167) with the same contract, including active values 0.1–10. Replace
duplicated cases in internal/admin/handler_virtualmodels_test.go (402-438) with
a table-driven endpoint test covering 0, both active boundaries, and rejected
values. Update web/dashboard/tests/models-virtual-models.test.js (483-504) to
verify numeric zero omits slowdown from the save payload.
---
Outside diff comments:
In `@internal/server/audio_service.go`:
- Around line 91-105: Add table-driven tests covering speech, transcription, and
translation handlers, using the corresponding route slowdown configuration.
Verify each test resolves the expected slowdown factor, applies the delay after
a successful provider call, and handles context cancellation before sending the
response; cover the success paths around CreateSpeech and the analogous
transcription/translation provider calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab97eeba-1940-4c6d-9d58-eee1f9c0b1ca
⛔ Files ignored due to path filters (3)
internal/admin/dashboard/static/dist/assets/index-BoW-V4GJ.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-D05Km9Si.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (33)
.env.templateconfig/config.example.yamlconfig/virtualmodels.godocs/features/virtual-models.mdxdocs/openapi.jsoninternal/admin/handler_virtualmodels.gointernal/admin/handler_virtualmodels_test.gointernal/core/request_model_resolution.gointernal/gateway/inference_execute.gointernal/gateway/interfaces.gointernal/gateway/request_model_resolution.gointernal/gateway/request_model_resolution_test.gointernal/gateway/slowdown.gointernal/gateway/slowdown_test.gointernal/server/audio_service.gointernal/server/model_slowdown.gointernal/streaming/slowdown_stream.gointernal/streaming/slowdown_stream_test.gointernal/virtualmodels/config.gointernal/virtualmodels/config_overlay_test.gointernal/virtualmodels/service.gointernal/virtualmodels/slowdown.gointernal/virtualmodels/slowdown_test.gointernal/virtualmodels/store_mongodb.gointernal/virtualmodels/store_sql.gointernal/virtualmodels/store_test.gointernal/virtualmodels/types.gointernal/virtualmodels/validation.goweb/dashboard/src/pages/models/ModelRow.svelteweb/dashboard/src/pages/models/VirtualModelEditor.svelteweb/dashboard/src/pages/models/virtualModels.svelte.jsweb/dashboard/src/pages/models/virtualModelsLogic.jsweb/dashboard/tests/models-virtual-models.test.js
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Confidence Score: 3/5Not ready to merge until cancellation reliably interrupts the upstream stream reader or ownership guarantees an immediate close on every abandoned-stream path. The affected cancellation path was exercised with a blocking upstream reader and showed that context cancellation alone leaves the source open and the drainer blocked. Focused checks also confirmed policy precedence and dashboard range handling behave as intended. Files Needing Attention: internal/streaming/slowdown_stream.go needs cancellation-aware upstream cleanup; its lifecycle behavior should be covered with a blocked-reader regression test.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the slowdown review feedback in dad5a66. Most importantly, usage and cost observers now consume provider events before client-facing slowdown buffering. When the provider finishes, the observed source is closed and usage is flushed even if the client disconnects before the queued final usage event is delivered. This is covered for both Chat Completions and Responses with a regression test that waits for upstream completion, cancels the client while delivery remains delayed, and verifies usage is recorded exactly once. Also included:
The delayed stream remains intentionally unbounded because full upstream draining and in-memory accumulation are part of the requested mode; the memory tradeoff is documented. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/virtualmodels/validation.go (1)
137-145: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd direct tests for non-finite slowdown values.
The endpoint test cannot send
NaNor infinities through JSON. Add a table-driven unit test forvalidateSlowdownwith nil, zero, both limits, out-of-range values,math.NaN(), and positive and negative infinity. The coverage summary reports partial virtual-model validation coverage.As per coding guidelines, “Add or update table-driven tests for behavior changes, including … error handling.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/virtualmodels/validation.go` around lines 137 - 145, Add a table-driven unit test for validateSlowdown covering nil, zero, MinSlowdownFactor, MaxSlowdownFactor, values below and above the allowed range, math.NaN(), positive infinity, and negative infinity; assert valid inputs return nil and invalid inputs return an error.Source: Coding guidelines
internal/virtualmodels/store_test.go (1)
14-50: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd table-driven coverage for slowdown replacement.
The test only covers inserts. Add an upsert-and-get case that changes an existing
Slowdownfrom a configured value tonil. This verifies that stores clear an alias override instead of retaining stale slowdown data. Include nil, zero, minimum, and maximum values in the table.As per coding guidelines, “Add or update table-driven tests for behavior changes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/virtualmodels/store_test.go` around lines 14 - 50, The existing store test only verifies inserts and must cover replacing an existing slowdown value. Add a table-driven upsert-and-get test around the VirtualModel store flow, updating an existing model’s Slowdown from a configured value to nil and verifying the stored result clears the stale override; include nil, zero, minimum, and maximum slowdown cases.Source: Coding guidelines
web/dashboard/tests/models-virtual-models.test.js (1)
533-539: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative cases for unsupported policy scopes.
vmFormSupportsSlowdownexplicitly rejects the global selector"/"and provider selectors ending in"/", but these tests cover only supported forms. Add assertions for both exclusions to protect this scope contract.Suggested assertions
+ assert.equal(vmFormSupportsSlowdown({ source: "/" }), false); + assert.equal(vmFormSupportsSlowdown({ source: "openai/" }), false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/dashboard/tests/models-virtual-models.test.js` around lines 533 - 539, Extend the test case for vmFormSupportsSlowdown with negative assertions covering the unsupported global selector "/" and provider selector ending in "/" policy scopes, confirming both return false while preserving the existing supported-form assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/audio_service_test.go`:
- Around line 36-38: Add a separate provider-completion signal in the test
setup, close it only after waitForAudioResponse finishes, and await it before
triggering cancellation. Update the affected test paths around providerCalled
and waitForAudioResponse so cancellation occurs during the post-inference delay
rather than while provider execution is still active.
In `@internal/streaming/slowdown_stream_test.go`:
- Around line 58-76: Convert TestSlowdownStreamCancellationClosesBlockedUpstream
into a table-driven test covering parent-context cancellation and explicit
stream.Close() while the upstream Read is blocked. For each case, start the
stream, wait for source.readStarted, trigger the case-specific shutdown action,
and assert source.closed occurs within the existing timeout; retain cleanup and
isolate fresh context/source/stream instances per test case.
In `@internal/virtualmodels/types.go`:
- Around line 50-53: Update VirtualModel.clone to deep-copy the Slowdown pointer
when it is non-nil, while preserving nil values; ensure Service.Get returns the
cloned value without sharing Slowdown storage with the active snapshot,
consistent with the existing SessionAffinity copy.
---
Outside diff comments:
In `@internal/virtualmodels/store_test.go`:
- Around line 14-50: The existing store test only verifies inserts and must
cover replacing an existing slowdown value. Add a table-driven upsert-and-get
test around the VirtualModel store flow, updating an existing model’s Slowdown
from a configured value to nil and verifying the stored result clears the stale
override; include nil, zero, minimum, and maximum slowdown cases.
In `@internal/virtualmodels/validation.go`:
- Around line 137-145: Add a table-driven unit test for validateSlowdown
covering nil, zero, MinSlowdownFactor, MaxSlowdownFactor, values below and above
the allowed range, math.NaN(), positive infinity, and negative infinity; assert
valid inputs return nil and invalid inputs return an error.
In `@web/dashboard/tests/models-virtual-models.test.js`:
- Around line 533-539: Extend the test case for vmFormSupportsSlowdown with
negative assertions covering the unsupported global selector "/" and provider
selector ending in "/" policy scopes, confirming both return false while
preserving the existing supported-form assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b808d6fd-d0ee-4633-80a5-00afb608cdc0
⛔ Files ignored due to path filters (2)
internal/admin/dashboard/static/dist/assets/index-DiApg2WK.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (32)
.env.templateconfig/config.example.yamlconfig/virtualmodels.godocs/features/virtual-models.mdxdocs/openapi.jsoninternal/admin/handler_virtualmodels.gointernal/admin/handler_virtualmodels_test.gointernal/gateway/inference_execute.gointernal/gateway/inference_orchestrator.gointernal/gateway/request_model_resolution_test.gointernal/gateway/slowdown_test.gointernal/server/audio_service_test.gointernal/server/messages_handler.gointernal/server/stream_slowdown_test.gointernal/server/translated_inference_service.gointernal/streaming/slowdown_stream.gointernal/streaming/slowdown_stream_test.gointernal/virtualmodels/config_overlay_test.gointernal/virtualmodels/service.gointernal/virtualmodels/slowdown.gointernal/virtualmodels/slowdown_test.gointernal/virtualmodels/store_mongodb.gointernal/virtualmodels/store_sql.gointernal/virtualmodels/store_test.gointernal/virtualmodels/types.gointernal/virtualmodels/validation.gotools/openapi-postprocess.mjsweb/dashboard/src/pages/models/ModelRow.svelteweb/dashboard/src/pages/models/VirtualModelEditor.svelteweb/dashboard/src/pages/models/virtualModels.svelte.jsweb/dashboard/src/pages/models/virtualModelsLogic.jsweb/dashboard/tests/models-virtual-models.test.js
|
Follow-up P2 findings are addressed:
The terminal-state regression is table-driven for EOF and a custom upstream error and performs two terminal reads with a timeout to guard against the prior hang. |
2 participants
Description
Adds a model-specific slowdown setting configurable from YAML, the environment, admin API, and dashboard.
Streaming slowdown intentionally buffers delayed upstream chunks in an unbounded in-memory queue, so large factors on long or high-volume streams can increase memory use. Realtime WebSocket sessions are not slowed.
Validation
AI Generated
Implementation and PR summary were prepared with Codex and reviewed through the repository test, lint, race, performance, documentation, and generated-asset checks.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes