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())