From 533e06a05bdbfafb7e93a7e5da733264e33f4b35 Mon Sep 17 00:00:00 2001 From: Jan Kott <51777660+boostvolt@users.noreply.github.com> Date: Sat, 10 Oct 2026 10:47:27 +0200 Subject: [PATCH] fix: return structured AIP-193 solve errors --- proto/buf.md | 9 ++ proto/fanscore/optimizer/v1/optimizer.proto | 4 +- pyproject.toml | 2 + .../optimizer/v1/optimizer_pb2_grpc.py | 1 + src/optimizer/errors.py | 58 +++++++++ src/optimizer/grpc_service.py | 57 +++++++-- src/optimizer/solver.py | 58 +++++++-- tests/test_service.py | 112 +++++++++++++++++- uv.lock | 30 +++++ 9 files changed, 307 insertions(+), 24 deletions(-) create mode 100644 src/optimizer/errors.py diff --git a/proto/buf.md b/proto/buf.md index 12528b9..4c7ca03 100644 --- a/proto/buf.md +++ b/proto/buf.md @@ -9,6 +9,15 @@ - Consume solutions only when `result.status` is `OPTIMAL` or `FEASIBLE`. - Set an RPC deadline. The server caps solve time, workers, concurrency, and message size. +## Errors + +`Solve` application errors carry `google.rpc.Status` in the `grpc-status-details-bin` +trailer ([AIP-193](https://google.aip.dev/193)). Its `ErrorInfo.domain` is +`optimizer.fanscore.ch`; `ErrorInfo.reason` identifies the error. +Invalid input returns `INVALID_ARGUMENT`, including native `MODEL_INVALID` results. +`BadRequest.field_violations` identifies invalid fields when known. +gRPC-generated errors may omit the rich status trailer. + ## Client SDKs Install generated SDKs for your language from [the BSR module](https://buf.build/fanscore-ch/optimizer). diff --git a/proto/fanscore/optimizer/v1/optimizer.proto b/proto/fanscore/optimizer/v1/optimizer.proto index 68e69e9..c62614d 100644 --- a/proto/fanscore/optimizer/v1/optimizer.proto +++ b/proto/fanscore/optimizer/v1/optimizer.proto @@ -9,6 +9,7 @@ import "ortools/sat/sat_parameters.proto"; // Executes caller-defined OR-Tools CP-SAT models within server resource limits. service OptimizerService { // Solves a caller-defined model. No application data is loaded or persisted. + // Invalid input returns INVALID_ARGUMENT, with field violations when known. rpc Solve(SolveRequest) returns (SolveResponse); } @@ -27,7 +28,8 @@ message SolveRequest { // The solver's status, solution, and execution statistics. message SolveResponse { - // Includes OPTIMAL, FEASIBLE, INFEASIBLE, MODEL_INVALID, or UNKNOWN. + // Includes OPTIMAL, FEASIBLE, INFEASIBLE, or UNKNOWN. + // MODEL_INVALID is returned as an INVALID_ARGUMENT RPC error instead. // A solution may only be consumed for OPTIMAL or FEASIBLE. operations_research.sat.CpSolverResponse result = 1; } diff --git a/pyproject.toml b/pyproject.toml index 2ef665e..cc1d187 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,6 +6,8 @@ requires-python = ">=3.14,<3.15" dependencies = [ "grpcio>=1.81.1,<1.85", "grpcio-health-checking>=1.81.1,<1.82", + "grpcio-status>=1.81.1,<1.82", + "googleapis-common-protos>=1.72,<2", "ortools==9.15.6755", ] diff --git a/src/fanscore/optimizer/v1/optimizer_pb2_grpc.py b/src/fanscore/optimizer/v1/optimizer_pb2_grpc.py index 949e1dc..5dba865 100644 --- a/src/fanscore/optimizer/v1/optimizer_pb2_grpc.py +++ b/src/fanscore/optimizer/v1/optimizer_pb2_grpc.py @@ -28,6 +28,7 @@ class OptimizerServiceServicer: def Solve(self, request, context): """Solves a caller-defined model. No application data is loaded or persisted. + Invalid input returns INVALID_ARGUMENT, with field violations when known. """ context.set_code(grpc.StatusCode.UNIMPLEMENTED) context.set_details('Method not implemented!') diff --git a/src/optimizer/errors.py b/src/optimizer/errors.py new file mode 100644 index 0000000..07935aa --- /dev/null +++ b/src/optimizer/errors.py @@ -0,0 +1,58 @@ +from typing import NoReturn + +import grpc +from google.rpc import error_details_pb2, status_pb2 +from grpc_status import rpc_status + +ERROR_DOMAIN = "optimizer.fanscore.ch" + + +class ServiceError(Exception): + """An application error with a stable, machine-readable identity.""" + + def __init__( + self, + code: grpc.StatusCode, + message: str, + reason: str, + *, + metadata: dict[str, str] | None = None, + violations: dict[str, str] | None = None, + ) -> None: + super().__init__(message) + self.code = code + self.reason = reason + self.metadata = metadata or {} + self.violations = violations or {} + + def abort(self, context: grpc.ServicerContext) -> NoReturn: + status = status_pb2.Status(code=self.code.value[0], message=str(self)) + status.details.add().Pack( + error_details_pb2.ErrorInfo( + reason=self.reason, domain=ERROR_DOMAIN, metadata=self.metadata + ) + ) + if self.violations: + bad_request = error_details_pb2.BadRequest() + for field, description in self.violations.items(): + bad_request.field_violations.add(field=field, description=description) + status.details.add().Pack(bad_request) + context.abort_with_status(rpc_status.to_status(status)) + + +class InvalidArgument(ServiceError, ValueError): + def __init__( + self, + message: str, + reason: str, + *, + metadata: dict[str, str] | None = None, + violations: dict[str, str] | None = None, + ) -> None: + super().__init__( + grpc.StatusCode.INVALID_ARGUMENT, + message, + reason, + metadata=metadata, + violations=violations, + ) diff --git a/src/optimizer/grpc_service.py b/src/optimizer/grpc_service.py index 1a9a10d..299b336 100644 --- a/src/optimizer/grpc_service.py +++ b/src/optimizer/grpc_service.py @@ -7,6 +7,7 @@ from fanscore.optimizer.v1 import optimizer_pb2, optimizer_pb2_grpc from optimizer.config import Config +from optimizer.errors import InvalidArgument, ServiceError from optimizer.solver import Solver, prepare_parameters logger = logging.getLogger(__name__) @@ -19,33 +20,65 @@ def __init__(self, config: Config) -> None: def Solve( self, request: optimizer_pb2.SolveRequest, context: grpc.ServicerContext + ) -> optimizer_pb2.SolveResponse: + try: + return self._solve(request, context) + except ServiceError as error: + error.abort(context) + except Exception: + logger.exception("unexpected solve failure") + ServiceError( + grpc.StatusCode.INTERNAL, + "The solver failed unexpectedly. Contact the service operator.", + "INTERNAL_ERROR", + ).abort(context) + + def _solve( + self, request: optimizer_pb2.SolveRequest, context: grpc.ServicerContext ) -> optimizer_pb2.SolveResponse: started = time.monotonic() if not request.HasField("model"): - context.abort(grpc.StatusCode.INVALID_ARGUMENT, "model: value is required") + message = "model: value is required" + raise InvalidArgument( + message, + "MODEL_REQUIRED", + violations={"model": message}, + ) if not self._slots.acquire(blocking=False): - context.abort( - grpc.StatusCode.RESOURCE_EXHAUSTED, "solver capacity exhausted" + raise ServiceError( + grpc.StatusCode.RESOURCE_EXHAUSTED, + "solver capacity exhausted", + "SOLVER_CAPACITY_EXHAUSTED", + metadata={ + "maxConcurrentSolves": str(self._config.max_concurrent_solves) + }, ) try: - try: - parameters = prepare_parameters(request.parameters, self._config) - solver = Solver(request.model, parameters) - except ValueError as error: - context.abort(grpc.StatusCode.INVALID_ARGUMENT, str(error)) + parameters = prepare_parameters(request.parameters, self._config) + solver = Solver(request.model, parameters) if not context.add_callback(solver.cancel): - context.abort(grpc.StatusCode.CANCELLED, "request is no longer active") + raise ServiceError( + grpc.StatusCode.CANCELLED, + "request is no longer active", + "REQUEST_CANCELLED", + ) remaining = parameters.max_time_in_seconds - (time.monotonic() - started) deadline = context.time_remaining() if deadline is not None: remaining = min(remaining, deadline) if remaining <= 0: - context.abort( - grpc.StatusCode.DEADLINE_EXCEEDED, "solve deadline elapsed" + raise ServiceError( + grpc.StatusCode.DEADLINE_EXCEEDED, + "solve deadline elapsed", + "SOLVE_DEADLINE_EXCEEDED", ) if not context.is_active(): - context.abort(grpc.StatusCode.CANCELLED, "request is no longer active") + raise ServiceError( + grpc.StatusCode.CANCELLED, + "request is no longer active", + "REQUEST_CANCELLED", + ) result = solver.solve(remaining) logger.info( diff --git a/src/optimizer/solver.py b/src/optimizer/solver.py index 90df35f..c76d664 100644 --- a/src/optimizer/solver.py +++ b/src/optimizer/solver.py @@ -6,6 +6,7 @@ from ortools.sat.python import cp_model_helper from optimizer.config import Config +from optimizer.errors import InvalidArgument SUPPORTED_PARAMETERS = frozenset( { @@ -26,19 +27,27 @@ def prepare_parameters( fields = {field.name for field, _ in requested.ListFields()} unsupported = fields - SUPPORTED_PARAMETERS if unsupported: - raise ValueError(f"unsupported parameters: {', '.join(sorted(unsupported))}") + names = ", ".join(sorted(unsupported)) + message = f"unsupported parameters: {names}" + raise InvalidArgument( + message, + "UNSUPPORTED_PARAMETERS", + metadata={"parameters": names}, + violations={f"parameters.{name}": message for name in sorted(unsupported)}, + ) for name in ("max_time_in_seconds", "max_deterministic_time"): if requested.HasField(name): value = getattr(requested, name) if not math.isfinite(value) or value <= 0: - raise ValueError(f"{name} must be finite and greater than zero") + _invalid_parameter(name, value, "finite and greater than zero") for name in ("relative_gap_limit", "absolute_gap_limit"): value = getattr(requested, name) if not math.isfinite(value) or value < 0: - raise ValueError(f"{name} must be finite and nonnegative") + _invalid_parameter(name, value, "finite and nonnegative") for name in ("num_workers", "random_seed"): - if getattr(requested, name) < 0: - raise ValueError(f"{name} must be nonnegative") + value = getattr(requested, name) + if value < 0: + _invalid_parameter(name, value, "nonnegative") parameters = sat_parameters_pb2.SatParameters() parameters.CopyFrom(requested) @@ -50,6 +59,16 @@ def prepare_parameters( return parameters +def _invalid_parameter(name: str, value: float | int, requirement: str) -> None: + message = f"{name} must be {requirement}" + raise InvalidArgument( + message, + "INVALID_PARAMETER", + metadata={"parameter": name, "value": str(value), "requirement": requirement}, + violations={f"parameters.{name}": message}, + ) + + class Solver: def __init__( self, @@ -60,12 +79,22 @@ def __init__( # through the exposed text-format API. self._model = cp_model_helper.CpModelProto() if not self._model.parse_text_format(text_format.MessageToString(model)): - raise ValueError("model could not be parsed by OR-Tools") + message = "model could not be parsed by OR-Tools" + raise InvalidArgument( + message, + "MODEL_PARSE_ERROR", + violations={"model": message}, + ) self._parameters = cp_model_helper.SatParameters() if not self._parameters.parse_text_format( text_format.MessageToString(parameters) ): - raise ValueError("parameters could not be parsed by OR-Tools") + message = "parameters could not be parsed by OR-Tools" + raise InvalidArgument( + message, + "PARAMETERS_PARSE_ERROR", + violations={"parameters": message}, + ) # Construct the wrapper before registering cancellation. StopSearch then # also applies when the RPC ends immediately before Solve starts. self._wrapper = cp_model_helper.SolveWrapper() @@ -84,4 +113,19 @@ def solve(self, remaining_seconds: float) -> cp_model_pb2.CpSolverResponse: self._finished.set() result = cp_model_pb2.CpSolverResponse() text_format.Parse(str(native_result), result) + if result.status == cp_model_pb2.MODEL_INVALID: + # CP-SAT uses MODEL_INVALID for parameter errors too. Only name the + # model field when independent model validation confirms it is bad. + model_error = cp_model_helper.CpSatHelper.validate_model(self._model) + if model_error: + raise InvalidArgument( + "The model is invalid. Correct its field violations.", + "MODEL_INVALID", + violations={"model": model_error}, + ) + raise InvalidArgument( + "The solve input is invalid. Check the model and solver parameters.", + "INVALID_INPUT", + metadata={"validationError": result.solution_info}, + ) return result diff --git a/tests/test_service.py b/tests/test_service.py index ea2550d..9d3e050 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -3,6 +3,8 @@ import grpc import pytest +from google.rpc import error_details_pb2 +from grpc_status import rpc_status from ortools.sat import cp_model_pb2, sat_parameters_pb2 from fanscore.optimizer.v1 import optimizer_pb2 @@ -11,6 +13,27 @@ from optimizer.solver import Solver +def assert_error_details(error, reason, *, metadata=None, fields=()): + status = rpc_status.from_call(error) + assert status is not None + types = [detail.type_url for detail in status.details] + assert len(types) == len(set(types)), "error detail types must be unique" + info = error_details_pb2.ErrorInfo() + assert any(detail.Unpack(info) for detail in status.details) + assert info.domain == "optimizer.fanscore.ch" + assert info.reason == reason + if metadata is not None: + assert dict(info.metadata) == metadata + bad_request = error_details_pb2.BadRequest() + violations = [] + for detail in status.details: + if detail.Unpack(bad_request): + violations = list(bad_request.field_violations) + assert [violation.field for violation in violations] == list(fields) + assert all(violation.description for violation in violations) + return info + + def test_solves_knapsack_over_grpc(running_server, knapsack): with running_server() as (stub, address): response = stub.Solve(optimizer_pb2.SolveRequest(model=knapsack), timeout=2) @@ -32,15 +55,16 @@ def test_returns_infeasible_as_solver_result(running_server): assert not result.solution -def test_returns_invalid_model_as_solver_result(running_server): +def test_rejects_invalid_model_with_field_violation(running_server): model = cp_model_pb2.CpModelProto() model.variables.add(domain=[0, 1]) model.constraints.add().linear.CopyFrom( cp_model_pb2.LinearConstraintProto(vars=[99], coeffs=[1], domain=[0, 1]) ) - with running_server() as (stub, _): - result = stub.Solve(optimizer_pb2.SolveRequest(model=model), timeout=2).result - assert result.status == cp_model_pb2.MODEL_INVALID + with running_server() as (stub, _), pytest.raises(grpc.RpcError) as error: + stub.Solve(optimizer_pb2.SolveRequest(model=model), timeout=2) + assert error.value.code() == grpc.StatusCode.INVALID_ARGUMENT + assert_error_details(error.value, "MODEL_INVALID", fields=("model",)) def test_empty_present_model_is_valid(running_server): @@ -51,11 +75,27 @@ def test_empty_present_model_is_valid(running_server): assert result.status == cp_model_pb2.OPTIMAL +def test_native_parameter_error_does_not_blame_model(running_server, knapsack): + with running_server(max_concurrent_solves=1, max_workers=10001) as (stub, _): + with pytest.raises(grpc.RpcError) as error: + stub.Solve( + optimizer_pb2.SolveRequest( + model=knapsack, + parameters=sat_parameters_pb2.SatParameters(num_workers=10001), + ), + timeout=2, + ) + assert error.value.code() == grpc.StatusCode.INVALID_ARGUMENT + info = assert_error_details(error.value, "INVALID_INPUT") + assert "num_workers" in info.metadata["validationError"] + + def test_requires_model(running_server): with running_server() as (stub, _), pytest.raises(grpc.RpcError) as error: stub.Solve(optimizer_pb2.SolveRequest(), timeout=2) assert error.value.code() == grpc.StatusCode.INVALID_ARGUMENT assert error.value.details() == "model: value is required" + assert_error_details(error.value, "MODEL_REQUIRED", fields=("model",)) @pytest.mark.parametrize( @@ -68,6 +108,9 @@ def test_requires_model(running_server): sat_parameters_pb2.SatParameters(random_seed=-1), sat_parameters_pb2.SatParameters(relative_gap_limit=float("inf")), sat_parameters_pb2.SatParameters(debug_crash_if_presolve_breaks_hint=True), + sat_parameters_pb2.SatParameters( + num_search_workers=1, log_search_progress=True + ), ], ) def test_rejects_invalid_or_unsupported_parameters( @@ -80,6 +123,33 @@ def test_rejects_invalid_or_unsupported_parameters( timeout=5, ) assert error.value.code() == grpc.StatusCode.INVALID_ARGUMENT + names = sorted(field.name for field, _ in parameters.ListFields()) + if names[0] in ( + "num_search_workers", + "debug_crash_if_presolve_breaks_hint", + "log_search_progress", + ): + assert ( + error.value.details() == f"unsupported parameters: {', '.join(names)}" + ) + assert_error_details( + error.value, + "UNSUPPORTED_PARAMETERS", + metadata={"parameters": ", ".join(names)}, + fields=tuple(f"parameters.{name}" for name in names), + ) + else: + info = assert_error_details( + error.value, + "INVALID_PARAMETER", + fields=(f"parameters.{names[0]}",), + ) + assert info.metadata["parameter"] == names[0] + assert info.metadata["value"] == str(parameters.ListFields()[0][1]) + assert ( + error.value.details() + == f"{names[0]} must be {info.metadata['requirement']}" + ) result = stub.Solve( optimizer_pb2.SolveRequest(model=knapsack), timeout=5 ).result @@ -106,6 +176,12 @@ def solve(self, remaining_seconds): with pytest.raises(grpc.RpcError) as error: stub.Solve(optimizer_pb2.SolveRequest(model=knapsack), timeout=2) assert error.value.code() == grpc.StatusCode.RESOURCE_EXHAUSTED + assert error.value.details() == "solver capacity exhausted" + assert_error_details( + error.value, + "SOLVER_CAPACITY_EXHAUSTED", + metadata={"maxConcurrentSolves": "1"}, + ) assert check_health(address) finally: release.set() @@ -202,3 +278,31 @@ def test_message_size_is_bounded(running_server): ): stub.Solve(optimizer_pb2.SolveRequest(model=model), timeout=2) assert error.value.code() == grpc.StatusCode.RESOURCE_EXHAUSTED + # gRPC core rejects the payload before the application handler is invoked. + assert rpc_status.from_call(error.value) is None + + +def test_unexpected_failure_is_structured_and_releases_capacity( + running_server, knapsack, monkeypatch, caplog +): + def fail(*args, **kwargs): + raise RuntimeError("private diagnostic") + + with running_server(max_concurrent_solves=1) as (stub, _): + with monkeypatch.context() as patch: + patch.setattr(Solver, "solve", fail) + with pytest.raises(grpc.RpcError) as error: + stub.Solve(optimizer_pb2.SolveRequest(model=knapsack), timeout=2) + assert error.value.code() == grpc.StatusCode.INTERNAL + assert_error_details(error.value, "INTERNAL_ERROR") + assert ( + b"private diagnostic" + not in rpc_status.from_call(error.value).SerializeToString() + ) + assert "private diagnostic" in caplog.text + assert ( + stub.Solve( + optimizer_pb2.SolveRequest(model=knapsack), timeout=2 + ).result.status + == cp_model_pb2.OPTIMAL + ) diff --git a/uv.lock b/uv.lock index eb1a335..351a1f2 100644 --- a/uv.lock +++ b/uv.lock @@ -30,8 +30,10 @@ name = "fanscore-optimizer" version = "0.1.0" source = { editable = "." } dependencies = [ + { name = "googleapis-common-protos" }, { name = "grpcio" }, { name = "grpcio-health-checking" }, + { name = "grpcio-status" }, { name = "ortools" }, ] @@ -43,8 +45,10 @@ dev = [ [package.metadata] requires-dist = [ + { name = "googleapis-common-protos", specifier = ">=1.72,<2" }, { name = "grpcio", specifier = ">=1.81.1,<1.85" }, { name = "grpcio-health-checking", specifier = ">=1.81.1,<1.82" }, + { name = "grpcio-status", specifier = ">=1.81.1,<1.82" }, { name = "ortools", specifier = "==9.15.6755" }, ] @@ -54,6 +58,18 @@ dev = [ { name = "ruff", specifier = ">=0.16,<0.17" }, ] +[[package]] +name = "googleapis-common-protos" +version = "1.75.5" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "protobuf" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/8d/2b/6ce81972d5c8cab9705fddce3153be63222d9e12fd96f8baba5038a744dd/googleapis_common_protos-1.75.5.tar.gz", hash = "sha256:c7a866fc34ed29a3b10af627a4b9b1dc2433313ca6e959f0ae4feb132047ed72", size = 156513, upload-time = "2026-09-29T19:26:14.863Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/65/b9/6b29500a1c581ff4d77fd83c6568d068bee06f1b139fb6eb0a4f2d4bce8a/googleapis_common_protos-1.75.5-py3-none-any.whl", hash = "sha256:d7285525c23039db98f2463e6d5a4f9b958b94d497f03a844ece3259c4e72d5d", size = 307737, upload-time = "2026-09-29T19:25:48.735Z" }, +] + [[package]] name = "grpcio" version = "1.84.0" @@ -88,6 +104,20 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/6e/87/174bbfb5613794862c45b5cdf567fcfb478ad5a3a7b594e11d998656b2f9/grpcio_health_checking-1.81.1-py3-none-any.whl", hash = "sha256:cbc6a4171825ec64389de2f062d296ba129a5c27eefd0dd55fa837909184bdf9", size = 19120, upload-time = "2026-06-11T12:58:38.008Z" }, ] +[[package]] +name = "grpcio-status" +version = "1.81.1" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "googleapis-common-protos" }, + { name = "grpcio" }, + { name = "protobuf" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/32/26/0aa9168c87882381fd810d140c279a2490ed6aee655f0515d6f56c5ca404/grpcio_status-1.81.1.tar.gz", hash = "sha256:9389a03e746017b10f0630c064289201458f3ce01f5d7ef4b0bebc1ef6cf82ad", size = 13923, upload-time = "2026-06-11T12:58:48.636Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/e5/5e/5abfec5f7e89d3b7993d57cfb025ca5f968a2c18656d7fcda2b6919440b9/grpcio_status-1.81.1-py3-none-any.whl", hash = "sha256:08072fa9995f4a95c647fc6f4f85e2411573d00087bcabdf30f260114338f232", size = 14638, upload-time = "2026-06-11T12:58:31.982Z" }, +] + [[package]] name = "immutabledict" version = "4.3.1"