Name the broken model setting, at boot and in the 500 - #2777
Conversation
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
| validate_model(db, current_user, purpose) | ||
| except ModelConfigurationError as e: | ||
| raise _convert_model_error_to_http_exception(e, "execution") | ||
| except (ValueError, ImportError) as e: |
There was a problem hiding this comment.
[Improvement] Catching (ValueError, ImportError) here and converting it to a “deployment default … DEFAULT_*_MODEL” 500 seems risky: validate_model() can raise ImportError for org-configured models too (e.g. _build_configured_model only wraps ValueError, so an optional-dep ImportError from get_model() will escape). That would misclassify a user-selected model failure as a broken deployment default.
Fix: either wrap
ImportError(and any other “SDK construction” exceptions you consider user-config errors) intoModelConfigurationErrorin_build_configured_model, or narrow this handler so it only triggers when you know you’re actually building the deployment default (and adjust_deployment_model_error’s docstring accordingly).
There was a problem hiding this comment.
Good catch fix 👍. With _build_configured_model now catching (ValueError, ImportError) and re-raising ModelConfigurationError, this (ValueError, ImportError) handler should only apply to deployment defaults as intended. Looks good to me now.
| global _default_models_checked | ||
| if _default_models_checked: | ||
| return | ||
| _default_models_checked = True |
There was a problem hiding this comment.
[Improvement] _default_models_checked is set to True before any of the work runs. If get_model_settings() (or something unexpected in the loop) ever raises before you hit the except Exception around get_model(), the check will be permanently disabled for the process.
Fix: set
_default_models_checked = Trueaftersettings = get_model_settings()(or at the end), or only flip it once the loop has started successfully.
There was a problem hiding this comment.
Yep—moving _default_models_checked = True to after get_model_settings() (and adding the regression test) avoids permanently disabling the check on an early failure. Thanks for the update.
d4c8289 to
ccd160b
Compare
There was a problem hiding this comment.
Main concern still open from the earlier inline threads:
validate_execution_model()catching(ValueError, ImportError)and always turning it into a “deployment default DEFAULT_*_MODEL is broken” 500 can misclassify org-configured model failures (e.g.ImportErrorfromget_model()isn’t wrapped intoModelConfigurationError, and some org/user-paths can raiseValueError). This will return the wrong status/message and misdirect operators.
Fix: ensure org-configured model construction errors (incl.
ImportError, and any other “misconfig” exceptions you consider) are wrapped intoModelConfigurationErrorin_build_configured_model, or narrow the new handler so it only triggers when you’ve positively identified the deployment default path.
warn_on_unbuildable_default_models()sets_default_models_checked = Truebeforeget_model_settings()/ the loop runs; an unexpected exception there would permanently disable the check for the process.
Fix: flip the flag after successfully loading settings (or at the end / via
try/finallythat only marks success when the check actually ran).
Found 2 issues (0 critical, 2 improvements).
_build_configured_model caught only ValueError, so an ImportError from a provider module escaped it. huggingface raises one at import time when torch is absent, and _require_own_credentials lets such a row through as soon as it has an endpoint. Escaping, it reached _deployment_model_error and was reported as a broken DEFAULT_*_MODEL: the deployment blamed for a model the organization picked, which is the misdirection this branch exists to remove. Catching it alongside ValueError answers 400 naming their own model instead, on the generic branch, an ImportError carrying no message the classifier can read. Also sets the once-per-process flag in warn_on_unbuildable_default_models after the settings load rather than before, so a failure to read them leaves the check able to run again. Both found by peqy on #2777.
|
Looks good. The boot-time warnings + worker check and the 500’s public_detail naming the specific DEFAULT_*_MODEL should make this failure mode much easier to diagnose without leaking exception text. Ship it. Found 0 issues (0 critical, 0 improvements). |
_call_polyphemus_with_delegation raised a bare ValueError for an inactive or unverified account. That reached _deployment_model_error and answered "this deployment's default evaluation model could not be built", blaming the server for something the user can act on. Reachable: has_own_credentials exempts polyphemus rows from _require_own_credentials, so a keyless row builds; with no platform key and no RHESIS_API_KEY the delegation branch runs; and nothing in the auth layer gates on is_active or is_verified, so such a user reaches the dependency. RHESIS_API_KEY is unset by design on Rhesis-hosted. ModelConfigurationError instead. It subclasses ValueError, so existing handlers catch it unchanged, but execution_validation matches it first and answers 400 naming the account state. Found by peqy on #2777, in the part of its review about org paths that raise ValueError.
|
Both numbered points were addressed in c8d4e77, which landed after this review was submitted. Your own follow-ups on the two inline threads agree, so I won't restate them. The part worth answering is the parenthetical in (1): "and some org/user-paths can raise
Fixed in ba75413: it raises |
_build_configured_model caught only ValueError, so an ImportError from a provider module escaped it. huggingface raises one at import time when torch is absent, and _require_own_credentials lets such a row through as soon as it has an endpoint. Escaping, it reached _deployment_model_error and was reported as a broken DEFAULT_*_MODEL: the deployment blamed for a model the organization picked, which is the misdirection this branch exists to remove. Catching it alongside ValueError answers 400 naming their own model instead, on the generic branch, an ImportError carrying no message the classifier can read. Also sets the once-per-process flag in warn_on_unbuildable_default_models after the settings load rather than before, so a failure to read them leaves the check able to run again. Both found by peqy on #2777.
_call_polyphemus_with_delegation raised a bare ValueError for an inactive or unverified account. That reached _deployment_model_error and answered "this deployment's default evaluation model could not be built", blaming the server for something the user can act on. Reachable: has_own_credentials exempts polyphemus rows from _require_own_credentials, so a keyless row builds; with no platform key and no RHESIS_API_KEY the delegation branch runs; and nothing in the auth layer gates on is_active or is_verified, so such a user reaches the dependency. RHESIS_API_KEY is unset by design on Rhesis-hosted. ModelConfigurationError instead. It subclasses ValueError, so existing handlers catch it unchanged, but execution_validation matches it first and answers 400 naming the account state. Found by peqy on #2777, in the part of its review about org paths that raise ValueError.
ba75413 to
f981a98
Compare
A deployment whose DEFAULT_*_MODEL cannot be built gave no signal until the first execute returned a 500, long after whoever set the environment could act on it. Try building each of the four defaults at startup and log a warning naming the setting, the model string and the reason. A warning, not a failed boot: a deployment that never resolves a model still has to start. Catches every exception type rather than ValueError, because huggingface raises ImportError when torch is absent and a boot diagnostic that takes the app down is worse than the problem it reports. Runs on the worker as well as the API. A metric evaluation resolves a model there, and where the two environments differ it is the only place the worker's own gap shows up. Once per process: deployment settings cannot change within one, and the test suite starts a fresh app lifespan per test. Refs #2671 Signed-off-by: Harry Cruz <harry@rhesis.ai>
Test execution on a backend that cannot build its own default model answered "An unexpected error occurred", leaving the cause in the logs alone. The body now names the DEFAULT_*_MODEL at fault. Status code unchanged, as decided on #2681: the request was fine, the server is not, and telling an API caller to check their model settings would misdirect at a setting they cannot reach. Routed through internal_error's public_detail, so the exception text still stays out of the response and in the log with its traceback. Only a deployment default gets here. Every failure to build an org's own configured model is raised as ModelConfigurationError by _build_configured_model and still answered with the existing 400. Catches ImportError too, for a provider missing an optional dependency, but not bare Exception: QuotaExceededError has to reach its own handler to become a 402. validate_execution_model loops over the two purposes so the message names the one that failed rather than both. test_validate_execution_model_generic_error asserted the plain ValueError propagates, which is exactly what changes here, so it is replaced by tests for the status, the type, the named setting and the exception text staying out of the body. Closes #2671 Signed-off-by: Harry Cruz <harry@rhesis.ai>
_build_configured_model caught only ValueError, so an ImportError from a provider module escaped it. huggingface raises one at import time when torch is absent, and _require_own_credentials lets such a row through as soon as it has an endpoint. Escaping, it reached _deployment_model_error and was reported as a broken DEFAULT_*_MODEL: the deployment blamed for a model the organization picked, which is the misdirection this branch exists to remove. Catching it alongside ValueError answers 400 naming their own model instead, on the generic branch, an ImportError carrying no message the classifier can read. Also sets the once-per-process flag in warn_on_unbuildable_default_models after the settings load rather than before, so a failure to read them leaves the check able to run again. Both found by peqy on #2777. Signed-off-by: Harry Cruz <harry@rhesis.ai>
_call_polyphemus_with_delegation raised a bare ValueError for an inactive or unverified account. That reached _deployment_model_error and answered "this deployment's default evaluation model could not be built", blaming the server for something the user can act on. Reachable: has_own_credentials exempts polyphemus rows from _require_own_credentials, so a keyless row builds; with no platform key and no RHESIS_API_KEY the delegation branch runs; and nothing in the auth layer gates on is_active or is_verified, so such a user reaches the dependency. RHESIS_API_KEY is unset by design on Rhesis-hosted. ModelConfigurationError instead. It subclasses ValueError, so existing handlers catch it unchanged, but execution_validation matches it first and answers 400 naming the account state. Found by peqy on #2777, in the part of its review about org paths that raise ValueError. Signed-off-by: Harry Cruz <harry@rhesis.ai>
f981a98 to
d8127bc
Compare
There was a problem hiding this comment.
Looks good. The boot/worker warnings and the 500 public_detail that names the specific DEFAULT_*_MODEL should make this failure mode much easier to diagnose, and the follow-up fixes keep org/account errors from being misreported as deployment defaults. Ship it.
There was a problem hiding this comment.
Looks good. The startup/worker warnings and the 500 public_detail now point operators to the exact DEFAULT_*_MODEL setting while keeping exception text out of responses, and the follow-up fixes correctly prevent org/account-state failures from being misreported as broken deployment defaults. Ship it.
There was a problem hiding this comment.
Looks good. The follow-up fixes (wrapping provider ImportError and polyphemus account-state failures into ModelConfigurationError) keep the new (ValueError, ImportError) → deployment-default 500 path from misclassifying org/user errors. Boot + worker warnings and the named DEFAULT_*_MODEL 500 are solid. Ship it.
|
Looks good. The follow-up fixes (wrapping org-side |
1 participant
Purpose
A deployment that cannot build its own
DEFAULT_*_MODELgives the operator nothing to work with. Nothing at boot, and on the first execute a 500 reading{"detail": "An unexpected error occurred."}. The cause appears only in the backend logs. That is what #2671 was actually reporting, once the two questions it raised were settled: #2681 decided the 500 is correct (the model that fails is the deployment default, which an API caller cannot fix) and fixed the[Test] Java SDKfailure by configuring the key on thejavacompose profile. What was left is the diagnostics.What Changed
warn_on_unbuildable_default_models()(user_model_utils.py) tries to build each of the fourDEFAULT_*_MODELsettings and logs a warning naming the setting, the model string and the reason. A warning rather than a failed boot, because a deployment that never resolves a model still has to start. It catches every exception type, not justValueError, since huggingface raisesImportErrorwithout torch and a boot diagnostic that takes the app down would be worse than the problem it reports. Once per process: deployment settings cannot change within one, and the test suite starts a fresh app lifespan per test.worker_ready. A metric evaluation resolves a model on the worker, so where the API and worker environments differ that signal is the only place the worker's own gap shows up._deployment_model_error()(execution_validation.py) replaces the generic 500 body on this path with one that names the setting. Status code unchanged. It goes throughinternal_error'spublic_detail, so the exception text still stays out of the response and lands in the log with its traceback.validate_execution_modelloops over its two purposes so the message names the one that failed rather than both.Only a deployment default can reach the new branch, and two commits exist to keep that true. Both change user-visible behaviour:
_build_configured_modelnow catchesImportErroralongsideValueError. It caught onlyValueError, so anImportErrorfrom a provider module escaped it.huggingfaceraises one at import time without torch, and_require_own_credentialslets such a row through as soon as it has an endpoint. Escaping, it reached the new handler and was reported as a brokenDEFAULT_*_MODEL: the deployment blamed for a model the organization picked. It now answers 400 naming their own model instead of an opaque 500._call_polyphemus_with_delegationraisesModelConfigurationError, not a bareValueError, for an inactive or unverified account. That path is reachable:has_own_credentialsexemptspolyphemusrows from_require_own_credentials, with no platform key and noRHESIS_API_KEYthe delegation branch runs, and nothing in the auth layer gates onis_activeoris_verified.RHESIS_API_KEYis unset by design on Rhesis-hosted, so that condition holds there. It now answers 400 naming the account state rather than the deployment 500.ModelConfigurationErrorsubclassesValueError, so every existing handler catches it unchanged.The handler catches
(ValueError, ImportError)rather than bareException, soQuotaExceededErrorstill reaches its own handler and becomes a 402.Additional Context
test_validate_execution_model_generic_errorasserted that the plainValueErrorpropagates, which is exactly what this changes, so it is replaced by tests for the status, the type, the named setting and the exception text staying out of the body.test_raises_when_construction_failscoversresolve_default_hosted_modeland is untouched.handle_execution_error'sValueErrorbranch still answers 400 with the raw message. That is a different path, inside route bodies rather than the pre-flight dependency, and out of scope here.ImportError, and once for theValueErrorpaths its parenthetical mentioned.Testing
Verified for real, not only against mocks.
With
RHESIS_API_KEYunset, the boot check logs:Silent with the key set.
The response, through the actual
http_exception_handlerrather than an assertion on the exception:New file
tests/backend/app/test_default_model_startup_check.py, 7 tests: warns per setting, survives anImportError, silent when everything builds, runs once per process, does not burn that once-per-process flag when the settings load raises, uses the right model type per setting (the embedding default is a different SDK type), and the worker runs the same check. Three new tests intest_execution_validation.pyfor the 500, the purpose it names, andQuotaExceededErrornot being swallowed.For the two misclassification fixes:
test_model_override.pygains a case for an org's huggingface model failing on a missing optional dependency, checked to fail with the rawImportErrorwhen the fix is reverted, andtest_llm_delegation.pyasserts the new type on both account states plus that it is still aValueErrorsubclass, which is the property that keeps existing handlers working.cd apps/backend uv run pytest ../../tests/backend/app ../../tests/backend/routes ../../tests/backend/security ../../tests/backend/services ../../tests/backend/utils ../../tests/backend/jobs7074 passed, 48 skipped, 1 xfailed. All skips pre-existing. Ruff clean on every file touched.