fix(processor): propagate async ResultMessage HTTP status to batch#590
Conversation
| DISPATCHER_VERSION="${DISPATCHER_VERSION:-v0.7.3}" | ||
| _ASYNC_MOD_VERSION=$(grep 'llm-d-async/api' "${REPO_ROOT}/go.mod" | awk '{print $2}') | ||
| DISPATCHER_VERSION="${DISPATCHER_VERSION:-${_ASYNC_MOD_VERSION}}" | ||
| DISPATCHER_IMAGE="${DISPATCHER_IMAGE:-ghcr.io/llm-d-incubation/llm-d-async:${DISPATCHER_VERSION}}" |
There was a problem hiding this comment.
chart and image not changed after graduation?
There was a problem hiding this comment.
The v0.7.4 container image hasn't been published yet — the release workflow failed The chart OCI path at ghcr.io/llm-d/charts/async-processor also returns 404 currently.
Once the upstream image is available at ghcr.io/llm-d/llm-d-async:v0.7.4, I'll update the script to point there. Keeping it at the working v0.7.3 incubation path for now so dev-deploy and e2e don't break.
There was a problem hiding this comment.
it seems chart has moved
# helm show chart oci://ghcr.io/llm-d-incubation/charts/async-processor --version 0.7.4
Error: failed to perform "FetchReference" on source: ghcr.io/llm-d-incubation/charts/async-processor:0.7.4: not found
# helm show chart oci://ghcr.io/llm-d/charts/async-processor --version 0.7.4
Pulled: ghcr.io/llm-d/charts/async-processor:0.7.4
Digest: sha256:34023f82d2a40f245b1f1ebebfaeaa5e721190582d8016594fa6a6ff7989e195
apiVersion: v2
appVersion: v0.7.4
description: Helm chart for Async-Processor (AP) - Asynchronous Processor for LLM
inference workloads
name: async-processor
type: application
version: 0.7.4
There was a problem hiding this comment.
maybe stay with 0.7.3 and we merge this
once they fixed 0.7.4, we upgrade
There was a problem hiding this comment.
yes if we can stay on 0.7.3 (pre-rename?) the diff should be smaller as swell
There was a problem hiding this comment.
Thanks @evacchi @yizhaodev I've dropped the v0.7.4 upgrade commit from this branch.
This PR now only contains the status-code propagation fix + e2e test. Will open a follow-up PR for the module migration once the upstream image is published.
evacchi
left a comment
evacchi
left a comment
There was a problem hiding this comment.
only a nit, other than that 👍
| result.Error = &OutputError{Code: "server_error", Message: "async response has no body"} | ||
|
|
||
| // Non-HTTP failure: StatusCode == 0 with error metadata from llm-d-async. | ||
| if resp.StatusCode == 0 && (resp.ErrorCode != "" || resp.ErrorMessage != "") { |
There was a problem hiding this comment.
maybe let's turn this into a predicate on ResponseItem
There was a problem hiding this comment.
Thanks! added IsNonHTTPFailure() on GenerateResponse and switched the callsite to use it.
…tput Async dispatch discarded ResultMessage.StatusCode and hardcoded 200, so auth failures (e.g. 403 with empty body) became parse_error instead of preserving the real status. Propagate status/error fields through GetResult and asyncResult, and cover with unit + dispatcher e2e. Fixes #589 Signed-off-by: Jooyeon Mok <jmok@redhat.com>
b310e20 to
f458e6b
Compare
3 participants
Why is this PR needed?
When
dispatchMode: async, HTTP error statuses from the async-processor (e.g. 403 with an empty body) were lost on the batch-gateway consumer side. Batch output then reportedparse_errorinstead of preservingstatus_code: 403, which broke sync/async parity for authorization failures.Upstream llm-d-async already returns
ResultMessage.StatusCode/ error fields (llm-d/llm-d-async#298). This PR consumes those fields.What does this PR do?
StatusCode,ErrorCode, andErrorMessagetoGenerateResponseResultMessageinGetResult()asyncResult()to:StatusCode == 0+ error metadata) to output errorsHTTPErrorStatusPreserved) using a consumer-lesssim-pool-injectpool so a syntheticResultMessage{StatusCode: 403, Payload: ""}can be injected without racing the async-processor (Kind has no AuthPolicy)How was this tested?
Checklist
git commit -s) per DCOmake pre-commit)make test)make test-e2e)Related Issues
Fixes #589