feat(examples): add OpenAI-compatible benchmark sample - #686
Open
FamousDirector wants to merge 3 commits into
Open
feat(examples): add OpenAI-compatible benchmark sample#686FamousDirector wants to merge 3 commits into
FamousDirector wants to merge 3 commits into
Conversation
Add controllable OpenAI endpoints for SDK and load testing. NO-REF Signed-off-by: jcameron <jcameron@nvidia.com>
|
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 selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds an OpenAI-compatible Go HTTP server with JSON and SSE endpoints, validation tests, compatibility checks, Docker packaging, and a k6 Responses SSE load test with calibration and sustained-load profiles. ChangesOpenAI-compatible sample
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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
🧹 Nitpick comments (4)
examples/function-samples/openai-compatible-sample/http-server/main_test.go (1)
245-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset the shared gauge in cleanup.
activeRequestsis package-level state shared by every test. This test resets it only at the start. If an assertion fails while the first request still holds a slot, the gauge stays non-zero for later tests. Add a cleanup reset.Proposed refactor
activeRequests.Store(0) + t.Cleanup(func() { activeRequests.Store(0) }) router := newRouter()🤖 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 `@examples/function-samples/openai-compatible-sample/http-server/main_test.go` around lines 245 - 247, Update TestConcurrencyLimit to register a cleanup reset for the shared activeRequests gauge, ensuring it is restored to zero even when the test fails before its normal completion. Keep the existing initial reset and use the test cleanup mechanism so later tests cannot inherit stale state.examples/function-samples/openai-compatible-sample/http-server/main.go (3)
880-883: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the SSE header setup.
The same three-header block appears in
streamResponses(lines 881-883),streamChatCompletion(lines 964-966), andstreamCompletion(lines 1045-1047). Extract one helper so SSE header changes apply in one place.Also note that Go's HTTP server manages
Connectionitself and strips it for HTTP/2, so that header has no effect. Consider dropping it from the helper.Proposed refactor
+func setSSEHeaders(w http.ResponseWriter) { + w.Header().Set("Cache-Control", "no-cache") + w.Header().Set("Content-Type", "text/event-stream") +} + func streamResponses(ctx context.Context, w http.ResponseWriter, response responsesResponse, chunks []string, tuning benchmarkTuning) { - w.Header().Set("Cache-Control", "no-cache") - w.Header().Set("Content-Type", "text/event-stream") - w.Header().Set("Connection", "keep-alive") + setSSEHeaders(w)Apply the same replacement in
streamChatCompletionandstreamCompletion.🤖 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 `@examples/function-samples/openai-compatible-sample/http-server/main.go` around lines 880 - 883, Extract the repeated SSE header setup from streamResponses, streamChatCompletion, and streamCompletion into a shared helper so all three call the same header-setting logic. Update that helper to set the common SSE headers in one place and remove the Connection header from the shared setup, since net/http manages it and it has no effect here. Keep the existing streaming behavior unchanged aside from centralizing the header configuration.
655-665: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused error return from
outputChunks.
outputChunksnever returns a non-nil error. Validation already happens inresolveBenchmarkTuning. The current signature creates three unreachable 400 branches inhandleResponses,handleChatCompletions, andhandleCompletions.Proposed refactor
-func outputChunks(tuning benchmarkTuning) ([]string, error) { +func outputChunks(tuning benchmarkTuning) []string { chunks := make([]string, tuning.OutputChunks) for index := range chunks { chunk := tuning.Chunk if tuning.ChunkBytes > 0 { chunk = randomText(tuning.ChunkBytes) } chunks[index] = chunk } - return chunks, nil + return chunks }Then in each handler:
chunks := outputChunks(tuning)🤖 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 `@examples/function-samples/openai-compatible-sample/http-server/main.go` around lines 655 - 665, Update outputChunks to return only []string, removing its unused error result and adjusting the return statement. Update handleResponses, handleChatCompletions, and handleCompletions to call outputChunks without error handling, removing the unreachable 400-response branches while preserving their existing chunk-processing behavior.
272-280: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound body reads and idle connections.
The server sets only
ReadHeaderTimeout. A client that sends headers and then stalls the body, or that keeps an idle connection open, retains a connection and a goroutine without limit. AddReadTimeoutandIdleTimeout.Do not add
WriteTimeout. SSE responses can run for minutes becauseX-Load-Tester-ITL-MsandX-Load-Tester-Output-Chunkscontrol the stream duration, andWriteTimeoutwould abort valid streams.Proposed fix
server := &http.Server{ Addr: ":8000", Handler: newRouter(), ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 30 * time.Second, + IdleTimeout: 120 * time.Second, }🤖 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 `@examples/function-samples/openai-compatible-sample/http-server/main.go` around lines 272 - 280, Update the http.Server initialization in main to add ReadTimeout for bounding request body reads and IdleTimeout for limiting keep-alive connections. Preserve ReadHeaderTimeout and do not add WriteTimeout, so long-running SSE responses controlled by X-Load-Tester-ITL-Ms and X-Load-Tester-Output-Chunks continue uninterrupted.Source: Linters/SAST tools
🤖 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 `@examples/function-samples/load-tester-supreme/README.md`:
- Line 22: Update the message field documentation in the load-tester README to
specify protocol-specific requirements: require message for HTTP requests, while
allowing it to be omitted for gRPC and documenting that omission returns an
empty string.
In `@examples/function-samples/openai-compatible-sample/Dockerfile`:
- Around line 24-30: Update the final alpine stage after copying the binary to
create an unprivileged user named app, assign ownership of
/app/openai-compatible-sample to that user, and set USER app before CMD so the
server runs without root privileges.
In
`@examples/function-samples/openai-compatible-sample/http-server/openai_client_check.py`:
- Line 41: Replace every assertion in openai_client_check.py, including the
checks around response.output_text and the other listed validation points, with
explicit conditional checks that raise an error when expectations fail. Ensure
all compatibility checks still execute under python3 -O and success is printed
only after every condition passes.
- Around line 24-28: Update the OpenAI client initialization in the CLIENT
configuration to disable redirects for credentialed requests by supplying an
httpx.Client with follow_redirects set to false through http_client, while
preserving the existing base URL and authentication settings.
- Around line 24-28: In the OpenAI client initialization around BASE_URL and
OPENAI_API_KEY, validate that any non-placeholder API key is only used with an
HTTPS base URL. Preserve the local HTTP URL for the default "not-needed" key,
and reject or fail fast when a real key is configured with an HTTP URL before
constructing CLIENT.
In `@examples/load-tests/functions/oai_compatible_responses_sse_load_test.js`:
- Around line 219-224: Remove the unsupported timeout configuration from the
load-test options and eliminate the related OPENAI_RESPONSES_TIMEOUT
environment-variable usage, unless upgrading xk6-sse and implementing verified
timeout behavior through the sse.open call. Ensure the stream setup does not
pass a silently ignored timeout value.
- Around line 283-296: Update the request configuration returned by the OpenAI
Responses SSE load-test request builder to reject redirects before sending
credentialed requests with TOKEN. Since xk6-sse v0.1.7 does not expose a
redirects option, apply the required client/extension configuration rather than
adding an unsupported request field, while preserving the existing POST body,
headers, timeout, and tags.
- Around line 272-274: Update the header setup around config.token so the
OAI_COMPAT_URL is validated before assigning headers.Authorization. Reject HTTP
endpoints unless they target an explicit loopback host, while allowing HTTPS
endpoints, and only add the Bearer token after this validation succeeds.
---
Nitpick comments:
In `@examples/function-samples/openai-compatible-sample/http-server/main_test.go`:
- Around line 245-247: Update TestConcurrencyLimit to register a cleanup reset
for the shared activeRequests gauge, ensuring it is restored to zero even when
the test fails before its normal completion. Keep the existing initial reset and
use the test cleanup mechanism so later tests cannot inherit stale state.
In `@examples/function-samples/openai-compatible-sample/http-server/main.go`:
- Around line 880-883: Extract the repeated SSE header setup from
streamResponses, streamChatCompletion, and streamCompletion into a shared helper
so all three call the same header-setting logic. Update that helper to set the
common SSE headers in one place and remove the Connection header from the shared
setup, since net/http manages it and it has no effect here. Keep the existing
streaming behavior unchanged aside from centralizing the header configuration.
- Around line 655-665: Update outputChunks to return only []string, removing its
unused error result and adjusting the return statement. Update handleResponses,
handleChatCompletions, and handleCompletions to call outputChunks without error
handling, removing the unreachable 400-response branches while preserving their
existing chunk-processing behavior.
- Around line 272-280: Update the http.Server initialization in main to add
ReadTimeout for bounding request body reads and IdleTimeout for limiting
keep-alive connections. Preserve ReadHeaderTimeout and do not add WriteTimeout,
so long-running SSE responses controlled by X-Load-Tester-ITL-Ms and
X-Load-Tester-Output-Chunks continue uninterrupted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 007b31ca-4460-4a32-9404-6b5c5c9e701f
📒 Files selected for processing (11)
examples/README.mdexamples/function-samples/load-tester-supreme/Dockerfileexamples/function-samples/load-tester-supreme/README.mdexamples/function-samples/openai-compatible-sample/Dockerfileexamples/function-samples/openai-compatible-sample/README.mdexamples/function-samples/openai-compatible-sample/http-server/go.modexamples/function-samples/openai-compatible-sample/http-server/main.goexamples/function-samples/openai-compatible-sample/http-server/main_test.goexamples/function-samples/openai-compatible-sample/http-server/openai_client_check.pyexamples/load-tests/README.mdexamples/load-tests/functions/oai_compatible_responses_sse_load_test.js
Signed-off-by: jcameron <jcameron@nvidia.com>
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.
TL;DR
Add a controllable OpenAI-compatible function sample for SDK and load-test benchmarking.
Additional Details
For the Reviewer
Review benchmark header validation and stream termination behavior in the sample server. The client check exercises strict response validation through the public OpenAI Python SDK.
For QA
QA Needed: No. Sample-only change with automated and local SDK coverage.
Issues
NO-REF
Checklist
Summary by CodeRabbit