Raise ValueError/NotImplementedError instead of a generic RuntimeError based on DP error code#498
Draft
kalra-mohit wants to merge 1 commit into
Draft
Conversation
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 OpenMined#413 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every result/build/merge call in
algorithm_builder.hppcatches a non-OKabsl::Statusfrom the C++ DP library and turns it intostd::runtime_error, which pybind11 maps to a plain PythonRuntimeError. That's the same exception type whether you passed in a bad epsilon, asked for a result you'd already consumed, or hit a genuine internal failure in the library. From Python there's no way toexceptyour way out of the cases that are actually your fault versus the ones that aren't, short of parsing the message string.What I did
I traced through the vendored
differential-privacysource at the pinned submodule commit to see what status codes these specific call sites (build,result,partial_resultx3,merge) can actually produce. The two I could pin down cleanly:kInvalidArgument- by far the most common. Covers things like bad bounds/overflow in the builder, and (somewhat non-obviously) callingresult()/partial_result()a second time on the same algorithm instance, sinceAlgorithm::PartialResult()inalgorithm.hexplicitly returnsInvalidArgumentErroronceresult_returned_is already true.kUnimplemented- returned by the baseNoiseConfidenceInterval()for algorithms that don't override it.kInternal- e.g.Count::Merge()on an emptySummary.flowchart LR A[absl::Status from DP library] --> B{status.code} B -- kInvalidArgument --> C[py::value_error → ValueError] B -- kUnimplemented --> D[PyErr_SetString NotImplementedError] B -- everything else --> E[std::runtime_error → RuntimeError]I added a small
ThrowFromStatus()helper in a newstatus_errors.hppand swapped it in at all six throw sites inalgorithm_builder.hpp.kInvalidArgumentmaps toValueErrorusing pybind11's built-inpy::value_error(already the pattern used for build errors inqunatile_tree.cpp, so this isn't introducing a new convention).kUnimplementedmaps toNotImplementedErrorviaPyErr_SetString, since pybind11 doesn't ship a wrapper for that one. Everything else -kInternal,kFailedPrecondition, anything I didn't verify - still raisesRuntimeError, same as before. I didn't want to guess at mappings for codes I hadn't actually traced back to a real code path.One existing test,
test_bounded_mean.py::test_result_crash, assertedRuntimeErrorfor the "call result() twice" case - that's actually theInvalidArgumentpath, so I updated it to expectValueErrorand left a comment explaining why. I also addedtests/algorithms/test_error_types.pywith a couple of focused regression tests: one confirms the double-result case now raisesValueError(and explicitly isn't just accidentally aRuntimeErrorsubclass), and another confirms merging an emptySummarystill raisesRuntimeError, so the change doesn't accidentally widen beyond what I verified.Testing
This one's a C++ binding change, and I wasn't able to build the extension locally to run these against the real bindings - the CI workflow itself budgets 20 minutes just for the Google DP library build step, which is more than makes sense for a local sanity-check loop in this environment. What I did do:
differential-privacysubmodule commit for every scenario the new tests exercise, rather than guessing.black --checkandclang-format --style=file(matching.clang-format, which is what this repo's CI uses) against every file I touched - all clean, no diffs.py_compileon the Python test files.I'm relying on CI here to actually build and run the test suite - opening this as a draft until that comes back green, at which point I'll flip it to ready for review.
Fixes #413