feat: surface real root-cause errors on model launch failure - #5266
feat: surface real root-cause errors on model launch failure#5266OliverBryant wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances error reporting and handling during model deployment in Xinference by capturing, formatting, and propagating detailed backend tracebacks to the frontend. On the backend, a new DetailedHTTPException and error-formatting utilities are introduced to extract root causes and preserve them across actor boundaries. On the frontend, the launch dialog and request contexts are updated to display these detailed errors and tracebacks with collapsible sections and copy-to-clipboard functionality. The review feedback highlights three areas for improvement: adding a try-catch block around JSON.stringify in the error detail extraction to prevent potential crashes from circular references or BigInts, and checking the availability of navigator.clipboard before attempting copy operations in both the launch dialog and request context to avoid showing false success toasts.
qinxuye
left a comment
There was a problem hiding this comment.
Two issues need to be addressed before approval.
When a model launch failed, the Web UI showed a message that rarely pointed at the real cause. The backend already forwarded `str(e)` faithfully and xoscar preserved the exception type, message and traceback across actor boundaries, so the loss happened at six specific points: - The launch dialog's `.catch()` took no argument and discarded the error object entirely, relying solely on a global toast. - That toast used sonner's ~4s auto-dismiss at a 356px width, which is unreadable and uncopyable for a Python traceback. - `launch_model` sent only `str(e)`, dropping the traceback xoscar had already made available on `e.__traceback__`. `str(e)` is often the outermost wrapper's short message, with the root cause buried in `__cause__` and an `[address=..., pid=...]` prefix prepended. - The replica record written with `status=ERROR` was immediately overwritten to TERMINATING/TERMINATED by the cleanup that follows a failed launch, orphaning `error_message` on a row that looks like a normal shutdown. The frontend never rendered that field at all. - Multi-replica launches raised on the first failure, silently dropping the rest, so a benign error could mask the real one. - With `wait_ready=False` the exception was never retrieved from the task, so it was garbage collected with a warning and never reported anywhere. Backend: add `core/error_utils.py` to walk the cause chain to the root, strip xoscar's actor prefix, and format tracebacks (opt out with XINFERENCE_DISABLE_ERROR_TRACEBACK). Add `DetailedHTTPException`, which carries the traceback in a separate field so `detail` stays a plain string and existing clients are unaffected. Make ERROR a sticky replica status, record the cause both before and after cleanup, aggregate every replica failure via `AggregatedLaunchError` (re-raising the original exception when only one replica failed, so the 400/503 status mapping survives), consume the async-launch exception, and fail `/progress` outright once a launch has errored instead of reporting a dead launch as 0%. Record the root cause in `wait_for_load` too, the path where engines that initialise on a background thread (vLLM, SGLang) actually fail. Frontend: keep the dialog open on failure and show the cause inline with an expandable traceback and a copy button, render each replica's `error_message`, keep traceback-bearing toasts up until dismissed, and normalize FastAPI's array-shaped 422 `detail` so it no longer renders as "[object Object]". Verified end to end against a local server: a bogus `model_path` returns the real ValueError plus a traceback whose deepest frame is in the model loader; the replica reports ERROR with its message (TERMINATED before this change); all replicas are listed for a multi-replica failure; and a `wait_ready=false` failure is observable via /replicas with no "exception was never retrieved" warning.
Address review feedback. Launching a model only requires `models:write`, but reading logs requires `logs:list`. Returning a full traceback to every caller who may launch a model therefore handed internal filesystem paths and runtime details to operators who cannot reach them through the log center, bypassing that boundary. Gate the `traceback` field on `logs:list` (admins always qualify, and an unauthenticated cluster has no boundary to protect). The normalized root-cause `detail` still goes to anyone who may launch a model, so the UI remains useful without auth changes. API keys are restricted to model query and inference scopes and so never qualify. The check live-reads DB permissions, matching the auth service's own policy so a revoked permission takes effect immediately, and fails closed. Reuse the existing `copyToClipboard` helper for both copy affordances instead of calling `navigator.clipboard` directly: it reports real success or failure rather than always claiming success, and falls back to `execCommand` where the Clipboard API is unavailable (non-HTTPS origins). This also fixes a wrong i18n key on the toast action, which referenced a nonexistent `common.copy` and would have rendered the literal key path. Guard the remaining `JSON.stringify` in `extractDetail`'s array branch, which could throw on a BigInt or circular reference while the interceptor is already handling an error.
d8e463a to
25cc2e6
Compare
|
Pushed 25cc2e6 addressing all review feedback, and rebased onto latest main — the conflict is resolved and the PR is mergeable again. @qinxuye — permission boundary. You were right, and my original reasoning was wrong: I justified the default-on traceback by saying these paths are already exposed through the log center, without checking that the log center requires a different permission. The
@qinxuye — rebase. Both done-callbacks are kept at both call sites, including Gemini — all three comments addressed. The two clipboard ones now reuse the existing Validation: 129 tests pass (11 new for the permission gate, including the central case that Still unverified on my side: the successful-launch path, since this machine has no CUDA and a real launch cannot complete. |
mypy flagged that `payload.get("user_id")` is `Any | None` while
`get_user_by_id` expects an `int`. Treat a payload without a user id as
an unidentifiable caller and deny, rather than passing None through to
the database lookup.
Problem
When a model launch fails, the Web UI rarely shows the real cause. Users see a short, unhelpful line — or a toast that vanishes before it can be read.
Notably, this is not a case of the backend replacing errors with a generic message.
str(e)is forwarded faithfully at every layer, and xoscar preserves the exception type, message and traceback across actor boundaries. The information is lost at six specific points:handleLaunch's.catch()took no argument, so the component never sawdetailand relied entirely on a global toast.launch_modelsent onlystr(e). xoscar had already made the full remote traceback available one.__traceback__, andstr(e)is often just the outermost wrapper's short message — with the root cause buried in__cause__behind an[address=..., pid=...]prefix.{status: ERROR, error_message: ...}, then the cleanup that follows a failed launch (terminate_model→worker._update_model_state("stopping")) reset it to TERMINATING/TERMINATED, orphaningerror_messageon a row indistinguishable from a normal shutdown. The frontend never rendered that field at all.wait_ready=Falsethe error vanished completely.callback_for_async_launchnever calledtask.exception(), so the failure was garbage collected with an "exception was never retrieved" warning and reported nowhere.Changes
Backend
xinference/core/error_utils.py— walks the__cause__/__context__chain to the root cause, strips xoscar's actor prefix, and formats tracebacks. Opt out withXINFERENCE_DISABLE_ERROR_TRACEBACKfor hardened deployments (enabled by default: the operator hitting the failure is exactly who needs it, and these paths are already exposed via the log center).DetailedHTTPExceptioncarries the traceback in a separatetracebackfield.detailstays a plain string, so the Python client, the Web UI interceptor, and every existing consumer are unaffected — a dict-valueddetailwould have broken them all. Scoped tolaunch_modeland/progress; the ~90 otherstr(e)sites are untouched.remove_replica_statusclears it. Non-status keys such asmodel_statestill apply, so cleanup stays observable.AggregatedLaunchError. When exactly one replica fails the original exception object is re-raised, preserving its type so the existing 400/503/499 status mapping does not degrade to 500./progressnow fails outright once a launch has errored, instead of reporting a dead launch as 0% — this is what lets a polling client tell "failed" from "not started".wait_for_loadrecords the root cause too. This is the path where engines that initialise on a background thread (vLLM, SGLang) actually fail, and it previously set no error state at all.Frontend
error_message(the field existed in the types but had zero usages).detailis normalized, so it no longer renders as[object Object].Verification
Tested end to end against a local server:
model_pathdetail=ValueError: Invalid input. model_path: /nonexistent/... does not exist., plus a traceback whose deepest frame is in the model loader, not the supervisorstatus: ERRORwitherror_messagepopulated —TERMINATEDbefore this change/progressafter failuredetail; all three cards show ERRORwait_ready=false/replicasreports ERROR with the cause; no "exception was never retrieved" warning, and the traceback is in the logAssertionError: Torch not compiled with CUDA enabled— the true origin, fromxoscar/virtualenv/platform.py[object Object]Automated: 45 new tests across
test_error_utils.py,test_launch_error_reporting.py, andtest_launch_error_response.py; 163 tests pass in the affected suites.pre-commitclean (one pre-existing E231 in an unrelated docstring remains),tsc --noEmitclean, eslint at its existing 57-warning baseline with no new warnings.Not verified here: the successful-launch regression path. This machine has no CUDA, so a real launch cannot complete — it fails inside the virtualenv/torch probe. The failure paths above are fully covered; a GPU environment should confirm that a successful launch still returns
{"model_uid": ...}and redirects to/running-model.Compatibility
All API changes are additive:
tracebackon error bodies,model_stateon/replicas, anderror_messageonInstanceInfo.detailremains a string and no existing field changes type or disappears.