Assert what the served endpoints return, not just a 200 - #420
Open
nicknikolakakis wants to merge 2 commits into
Open
Assert what the served endpoints return, not just a 200#420nicknikolakakis wants to merge 2 commits into
nicknikolakakis wants to merge 2 commits into
Conversation
The mock engine returned one canned response to any POST and ignored the request body. That caps what the e2e can check at a status code: a reply that never reached the engine, or one a proxy invented, looks the same as a real one, and there is nothing to assert about streaming or bad input because the mock has no such behaviour. It now echoes the last user turn and the requested model, so a caller can tell its body arrived intact; serves an SSE stream of chat.completion.chunk frames terminated by [DONE] when a request sets stream; and rejects an unparseable body or one with no messages with a 400, and an unknown path with a 404, instead of answering 200 either way. It serves HTTP/1.1 so the stream is chunked the way an engine's is, which means keep-alive, so it also moves to ThreadingHTTPServer: a single-threaded server would hold the probes and the endpoint-picker behind one client. The cost is size. The ModelDeployment container template is a curated subset of name, image, command, args and env, so there is no volume to load a file from and the server stays inline in args, now about twice as long. Loading it from a ConfigMap through env.valueFrom is possible if it grows further. Towards modelplaneai#368. Signed-off-by: Nick Nikolakakis <nonicked@protonmail.com>
The local e2e curls /v1/chat/completions and /v1/messages and checks the status code. That proves the path is wired, and nothing else: a regression that answers 200 with a wrong shape, drops the request body on the way to the engine, or buffers a stream into one blob passes unnoticed. This replaces the two curl pods with a stdlib unittest suite under e2e/verify, pointed at the ModelService through MODELPLANE_ADDRESS. It asserts the OpenAI and Anthropic response shapes, that a marker in the last user turn comes back from the engine, that a streamed request returns event-stream frames whose content deltas reassemble into the message and end in [DONE], that a malformed body and a body with no messages are refused, and that /v1/models lists the served model. It waits for the route to serve before the first assertion, since the address publishes before the path carries traffic. The address is on the kind Docker subnet, so the suite runs from a pod on the control plane like the curls did. It goes in as a ConfigMap and runs under the same python image the mock engine uses: urllib and unittest only, so there is nothing to build and no install step. run.sh polls the pod's phase rather than using kubectl wait, which has no single condition for "ran, either way", prints the logs so a red run says which assertion failed, and exits non-zero unless the pod succeeded. The ruff and license checks grow a path to cover it, since both were scoped to functions/ and the docs validator and would otherwise leave new Python unchecked. Fixes modelplaneai#368. Signed-off-by: Nick Nikolakakis <nonicked@protonmail.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.
Fixes #368.
The local e2e brings the whole path up and then checks one thing about it: that
/v1/chat/completionsand/v1/messagesreturn 200. A regression that answers with the wrong shape, loses the request body between the gateway and the engine, or buffers a stream into a single blob passes that check.The mock engine is what limited it: one canned response to any POST, body ignored, so there was nothing else to assert. It now echoes the last user turn and the requested model, streams
chat.completion.chunkframes terminated by[DONE]when a request setsstream, and returns 400 for an unparseable body or one with nomessagesand 404 for an unknown path. On top of that, the two curl pods become a stdlibunittestsuite ine2e/verify, run from a pod on the control plane because the address is on the kind Docker subnet. It ships as a ConfigMap and runs under the same python image the mock uses, so nothing is built or installed.The suite fails 5 of its 8 cases against the pre-change mock, which is what says it tests behaviour rather than reachability. I verified the pod wiring on a throwaway kind cluster: green against the new mock with the pod reaching
Succeeded, and a deliberately wrongMODELPLANE_MODELputting it inFailed, which is what makesrun.shexit non-zero. I have not run the full two-cluster e2e, so this needs thetest-e2elabel to be proven end to end.The mock is about twice as long and still inline in
args, since the ModelDeployment container template is a curated subset with no volume to load a file from.env.valueFromfrom a ConfigMap is the way out if it grows further.