Fix worker status for unusable terminal backend responses#615
Conversation
|
This introduce worker dependency on GenerationResponse, not sure if that's the right way to go, |
|
Hmm, yeah I am not a fan of depending on |
|
Done |
sjmonson
left a comment
There was a problem hiding this comment.
Can you rebase and get rid of the merge commits?
|
@ushaket, this project requires a linear history on feature branches. You can do this by running: |
60e1abc to
d35b823
Compare
d35b823 to
d82eb51
Compare
Signed-off-by: Uri Shaket <ushaket@redhat.com>
Signed-off-by: Uri Shaket <ushaket@redhat.com>
d82eb51 to
4388149
Compare
dbutenhof
left a comment
There was a problem hiding this comment.
I have really mixed feelings about this. I don't like the idea of hacking this into each request handler separately -- and you've only handled a subset of the http request handlers in this PR. Maybe that's OK, but it feels weird.
I like your original idea of triggering this through the worker, perhaps by delegating to a backend validate_response method (which could, if necessary, delegate to individual request handlers where more specialized knowledge is required). Your check, for example, is pretty much generic and might even be a default implementation on the base Backend class that everyone would get by default.
(The risk, of course, is that such a default might break some weird special handlers that don't have "normal" text/token outputs; which means a generalization would require extensive testing through all the validations.)
| def _validate_compiled_response(self, response: GenerationResponse) -> None: | ||
| """Raise when endpoint produced a terminal payload with no usable output.""" | ||
| has_text = bool(response.text and response.text.strip()) | ||
| output_tokens = response.output_metrics.total_tokens or 0 | ||
| if not has_text and output_tokens <= 0: | ||
| raise ValueError( | ||
| "[UNUSABLE_BACKEND_RESPONSE] backend resolved without a usable " | ||
| "terminal response payload" | ||
| ) |
There was a problem hiding this comment.
Instead of "without a usable terminal response payload", we should be more specific. Something like "empty response payload" would help.
| {"choices": [], "usage": {}}, | ||
| ], | ||
| ) | ||
| def test_non_streaming_raises_for_unusable_terminal_payload( |
There was a problem hiding this comment.
These tests (and the ones starting at 671) feel like duplicates of each other. But that's an issue for a different PR.
Tests are working for me, and I don't really have any additional complaints to add onto what the others have said.
Signed-off-by: Uri Shaket <ushaket@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@dbutenhof Thanks for the feedback, I agree the per-handler approach is a bit awkward, and it currently only covers text/chat. I prototyped extending this to Responses (same text / tool_calls / tokens check) and Embeddings (raw data[].embedding check, since those responses never have text). Happy to include either or both in this PR if that’s useful. How would you like to proceed? Keep this PR scoped to text/chat and take broader coverage in a follow-up I’m fine with any of these, just want to align before I push more surface area into this PR. |
Summary
This PR fixes a scheduler correctness bug where requests could be marked as
completedeven when the backend resolved without a usable terminal response. It adds explicit terminal-response validation in the worker so malformed/empty terminal results are surfaced aserroredwith a clear diagnostic instead of being counted as successful requests.Details
WorkerProcessto guard final status transitions.errored[UNUSABLE_BACKEND_RESPONSE] backend resolved without a usable terminal response payloadGenerationResponse-aware usability criteria:textoroutput_metrics.total_tokens > 0Noneterminal response is always unusableGenerationResponsefallback remainsbool(response)for generic/test compatibilitytests/unit/scheduler/test_worker.pyfor:erroredGenerationResponse->erroredGenerationResponsewith empty text ->completedTest Plan
uv run pytest -q tests/unit/scheduler/test_worker.py -k "terminal_response or empty_generation_response or generation_response_with_tokens or invalid_initialization"completedGenerationResponseis not markedcompletedoutput_tokens > 0) is accepted ascompletedRelated Issues
Use of AI
## WRITTEN BY AI ##)git log
commit 368abbd
Author: Uri Shaket ushaket@redhat.com
Date: Mon Mar 2 23:55:49 2026 +0200
commit 4388149
Author: Uri Shaket ushaket@redhat.com
Date: Mon Mar 2 23:57:34 2026 +0200
commit 9db04b6
Author: Uri Shaket ushaket@redhat.com
Date: Wed Jul 15 11:25:12 2026 +0300
commit c55e6a4
Author: Uri Shaket ushaket@redhat.com
Date: Wed Jul 15 13:23:05 2026 +0300
commit 509ce95
Author: Uri Shaket ushaket@redhat.com
Date: Thu Jul 16 17:28:58 2026 +0300
Co-authored-by: Cursor cursoragent@cursor.com
Signed-off-by: Uri Shaket ushaket@redhat.com