Repository navigation
fix(client): report the error message of a failed async task #776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| import ai.docling.serve.api.task.request.TaskResultRequest; | ||
| import ai.docling.serve.api.task.request.TaskStatusPollRequest; | ||
| import ai.docling.serve.api.task.response.TaskStatusPollResponse; | ||
| import ai.docling.serve.api.util.Utils; | ||
| import ai.docling.serve.api.util.ValidationUtils; | ||
|
|
||
| /** | ||
|
|
@@ -128,8 +129,11 @@ private <O> CompletionStage<O> pollTaskStatus(TaskStatusPollResponse statusRespo | |
| } | ||
|
|
||
| case FAILURE -> { | ||
| var errorMessage = Optional.ofNullable(statusResponse.getTaskStatusMetadata()) | ||
| .map(metadata -> "Task failed: %s".formatted(metadata)) | ||
| // Prefer the reason reported by Docling Serve, as the task metadata only holds document counts | ||
| var errorMessage = Optional.ofNullable(statusResponse.getErrorMessage()) | ||
| .filter(Utils::isNotNullOrBlank) | ||
| .or(() -> Optional.ofNullable(statusResponse.getTaskStatusMetadata()).map(Object::toString)) | ||
| .map(reason -> "Task failed: %s".formatted(reason)) | ||
| .orElse("Task failed"); | ||
|
Comment on lines
+132
to
137
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flagging this so you don't wonder whether I skipped it: it's correct as written, including dropping the I'd actually drafted a comment asking you to keep the counts, on the theory that a failed batch would be reporting something like "3 of 10 documents failed." It isn't. |
||
|
|
||
| yield CompletableFuture.failedStage( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field shape is right, and I want to say so explicitly because the alternative is tempting. Nullable
Stringplus@Nullablematches the four fields already on this type; returningOptionalwould follow our general "Optional over null" guidance but would make this one field behave unlike its neighbors inside a Lombok wire-format DTO. Consistency is the better call here.And on what's deliberately not here, so it doesn't look like an oversight: upstream also carries
failure: PublicFailureInfo(category/message/retryable/phase). Leaving it out is correct for this PR — it's Ray-only and v1.25.0+, whileerror_messageworks everywhere we support. I've opened #778 for it, which also captures an unknown-enum-value policy question we need to settle before addingFailureCategoryas a Java enum.