From 0fe8962c1fc29444b0ab7a292b8a62d53fe6adf7 Mon Sep 17 00:00:00 2001 From: Mohit Kalra Date: Tue, 21 Jul 2026 21:20:38 -0700 Subject: [PATCH] Raise ValueError/NotImplementedError instead of generic RuntimeError Every failure path in the algorithm bindings (build, result, partial_result x3, merge) collapsed absl::Status errors from the C++ DP library into a bare std::runtime_error, no matter what actually went wrong. A bad epsilon and an internal library bug looked identical from Python, so callers had no way to catch "you gave me a bad argument" separately from "something broke internally" without string-matching the exception message. Traced through the vendored differential-privacy source (pinned submodule commit) to see which status codes these call sites can actually return: kInvalidArgument (bad bounds, calling result() twice, etc.), kInternal, and kUnimplemented, with kInvalidArgument by far the most common and the one users most need to distinguish. Added a small ThrowFromStatus() helper that maps kInvalidArgument to ValueError (via pybind11's existing py::value_error, already used elsewhere in this codebase) and kUnimplemented to NotImplementedError, while leaving everything else raising RuntimeError as before so this doesn't silently reclassify errors we haven't verified. One existing test (test_bounded_mean.py::test_result_crash) asserted RuntimeError for calling result() twice, which is actually the InvalidArgument case per algorithm.h's PartialResult() budget check - updated it to expect ValueError, and added a dedicated test file that exercises both the InvalidArgument -> ValueError path and confirms Internal errors (e.g. merging an empty Summary) still raise RuntimeError unchanged. This repo's C++ extension isn't buildable in a lightweight way locally (CI itself budgets 20 minutes just for the Google DP library build), so I couldn't compile and run these against the real bindings here - verified the status codes by reading the pinned upstream source instead and I'm leaning on CI to confirm the build and test run. Fixes #413 Co-Authored-By: Claude Sonnet 5 --- .../PyDP/pydp_lib/algorithm_builder.hpp | 13 ++--- src/bindings/PyDP/pydp_lib/status_errors.hpp | 44 ++++++++++++++++ tests/algorithms/test_bounded_mean.py | 6 ++- tests/algorithms/test_error_types.py | 50 +++++++++++++++++++ 4 files changed, 106 insertions(+), 7 deletions(-) create mode 100644 src/bindings/PyDP/pydp_lib/status_errors.hpp create mode 100644 tests/algorithms/test_error_types.py diff --git a/src/bindings/PyDP/pydp_lib/algorithm_builder.hpp b/src/bindings/PyDP/pydp_lib/algorithm_builder.hpp index 16bfd42c..37332f31 100644 --- a/src/bindings/PyDP/pydp_lib/algorithm_builder.hpp +++ b/src/bindings/PyDP/pydp_lib/algorithm_builder.hpp @@ -11,6 +11,7 @@ #include "algorithms/numerical-mechanisms.h" #include "algorithms/order-statistics.h" #include "proto/summary.pb.h" +#include "status_errors.hpp" namespace dp = differential_privacy; namespace py = pybind11; @@ -87,7 +88,7 @@ class AlgorithmBuilder { absl::StatusOr> obj = builder.Build(); if (!obj.ok()) { - throw std::runtime_error(obj.status().ToString()); + ThrowFromStatus(obj.status()); } return std::move(obj.value()); @@ -176,7 +177,7 @@ class AlgorithmBuilder { auto result = pythis.Result(v.begin(), v.end()); if (!result.ok()) { - throw std::runtime_error(result.status().ToString()); + ThrowFromStatus(result.status()); } if constexpr ((should_return_T())) return dp::GetValue(result.value()); @@ -190,7 +191,7 @@ class AlgorithmBuilder { auto result = pythis.PartialResult(); if (!result.ok()) { - throw std::runtime_error(result.status().ToString()); + ThrowFromStatus(result.status()); } if constexpr ((should_return_T())) @@ -205,7 +206,7 @@ class AlgorithmBuilder { auto result = pythis.PartialResult(privacy_budget); if (!result.ok()) { - throw std::runtime_error(result.status().ToString()); + ThrowFromStatus(result.status()); } if constexpr ((should_return_T())) @@ -220,7 +221,7 @@ class AlgorithmBuilder { auto result = pythis.PartialResult(noise_interval_level); if (!result.ok()) { - throw std::runtime_error(result.status().ToString()); + ThrowFromStatus(result.status()); } if constexpr ((should_return_T())) return dp::GetValue(result.value()); @@ -238,7 +239,7 @@ class AlgorithmBuilder { pyself.def("merge", [](Algorithm& pythis, const dp::Summary& summary) { auto status = pythis.Merge(summary); if (!status.ok()) { - throw std::runtime_error(status.ToString()); + ThrowFromStatus(status); } }); diff --git a/src/bindings/PyDP/pydp_lib/status_errors.hpp b/src/bindings/PyDP/pydp_lib/status_errors.hpp new file mode 100644 index 00000000..1720aa7d --- /dev/null +++ b/src/bindings/PyDP/pydp_lib/status_errors.hpp @@ -0,0 +1,44 @@ +#ifndef PYDP_LIB_STATUS_ERRORS_H_ +#define PYDP_LIB_STATUS_ERRORS_H_ + +#include + +#include + +#include "absl/status/status.h" + +namespace py = pybind11; + +namespace differential_privacy { +namespace python { + +// Raises a Python exception for a non-OK absl::Status coming back from the DP +// library, picking the exception type based on the status code instead of +// collapsing every failure into a generic RuntimeError. +// +// This lets callers distinguish, for example, a bad argument they passed in +// (ValueError) from a method that simply isn't implemented for a given +// algorithm (NotImplementedError), the same way they would for any other +// Python API. Status codes that don't have an obviously better fit (Internal, +// FailedPrecondition, etc.) keep raising RuntimeError, which was already the +// behavior for every code before this. +// +// `status` is expected to be non-OK; callers should check status.ok() (or +// statusor.ok()) first. +inline void ThrowFromStatus(const absl::Status& status) { + const std::string message = status.ToString(); + switch (status.code()) { + case absl::StatusCode::kInvalidArgument: + throw py::value_error(message); + case absl::StatusCode::kUnimplemented: + PyErr_SetString(PyExc_NotImplementedError, message.c_str()); + throw py::error_already_set(); + default: + throw std::runtime_error(message); + } +} + +} // namespace python +} // namespace differential_privacy + +#endif // PYDP_LIB_STATUS_ERRORS_H_ diff --git a/tests/algorithms/test_bounded_mean.py b/tests/algorithms/test_bounded_mean.py index 7ad4259c..c588f09c 100644 --- a/tests/algorithms/test_bounded_mean.py +++ b/tests/algorithms/test_bounded_mean.py @@ -86,10 +86,14 @@ def test_serialize_merge(): def test_result_crash(): + # Calling result()/partial_result() a second time trips the DP library's + # "InvalidArgument" check (an algorithm can only produce results once for + # a given epsilon/delta budget), which the bindings now surface as a + # ValueError instead of a generic RuntimeError. See #413. bm1 = BoundedMean(1, 0, 1, 10) bm1.add_entries([1 for i in range(100)]) bm1.result() - with pytest.raises(RuntimeError): + with pytest.raises(ValueError): bm1.result() diff --git a/tests/algorithms/test_error_types.py b/tests/algorithms/test_error_types.py new file mode 100644 index 00000000..89a03d59 --- /dev/null +++ b/tests/algorithms/test_error_types.py @@ -0,0 +1,50 @@ +# third party +import pytest + +# pydp absolute +import pydp._pydp as dp +import pydp.algorithms.laplacian as py_algos + +# Regression tests for #413: the bindings used to collapse every non-OK +# absl::Status coming back from the DP library into a plain RuntimeError, +# no matter what actually went wrong. That made it impossible to tell "you +# passed a bad argument" apart from "something failed internally" without +# parsing the exception message. These tests pin down the exception types +# for the status codes that are easy to trigger deterministically from +# Python: InvalidArgument (now ValueError) and Internal (still RuntimeError, +# since there's no better built-in fit). + + +def test_double_result_raises_value_error_not_runtime_error(): + # The DP library refuses to hand out a result twice for the same + # epsilon/delta budget and returns an InvalidArgument status for the + # second call. That should surface as a ValueError. + count = py_algos.Count(epsilon=1.0) + count.add_entries([1] * 10) + + count.result() + with pytest.raises(ValueError): + count.result() + + +def test_double_result_error_is_not_a_bare_runtime_error(): + # Guard against a ValueError subclassing RuntimeError one day and this + # test passing for the wrong reason. + count = py_algos.Count(epsilon=1.0) + count.add_entries([1] * 10) + count.result() + + with pytest.raises(ValueError) as exc_info: + count.result() + assert not isinstance(exc_info.value, RuntimeError) + + +def test_merge_empty_summary_still_raises_runtime_error(): + # Merging an empty Summary trips an Internal status in the DP library + # (there's no count data to merge). Internal errors don't have an + # obviously-better built-in Python exception, so they should keep + # raising RuntimeError, same as before this change. + count = py_algos.Count(epsilon=1.0) + + with pytest.raises(RuntimeError): + count.merge(dp.Summary())