Add validation for count in PrivacyAccountant.composeValidate count in privacy accountant#368
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| def compose(self, event: dp_event.DpEvent, count: int = 1) -> Any: | ||
| """Updates internal state to account for application of a `DpEvent`. | ||
|
|
||
| Calls to `get_epsilon` or `get_delta` after calling `compose` will return |
There was a problem hiding this comment.
It looks like most of the docstring has been removed?
| raise UnsupportedEventError(f'Unsupported event: {event}.') | ||
| self._ledger.compose(event, count) | ||
| self._maybe_compose(event, count, True) | ||
| self._compose(event, count) |
There was a problem hiding this comment.
It is critical that _maybe_compose is called here. In fact, it looks like this code would cause an infinite loop.
| # Implementing `get_delta` is optional. | ||
| pass | ||
|
|
||
| # ----- New tests added for validating `count` in compose() ----- |
There was a problem hiding this comment.
Comments should be to clarify the code, not documenting when changes are made.
| return 0.0 | ||
|
|
||
|
|
||
| class PrivacyAccountantComposeCountValidationTest(absltest.TestCase): |
There was a problem hiding this comment.
Could you expand this to check all arg types of both concrete methods of PrivacyAccountant: supports and compose. Call the test PrivacyAccountantArgValidationTest and also add tests for event in supports and compose.
| return None | ||
|
|
||
| def get_epsilon(self, target_delta: float) -> float: | ||
| # Dummy implementation for testing. |
There was a problem hiding this comment.
nit: this is obvious from context, no need for this comment
| # ----- New tests added for validating `count` in compose() ----- | ||
|
|
||
| class _DummyPrivacyAccountant(privacy_accountant.PrivacyAccountant): | ||
| """Minimal concrete PrivacyAccountant for testing compose() validation.""" |
There was a problem hiding this comment.
Test all validation (see below)
2 participants
Motivation
PrivacyAccountant.composevalidateseventbut does not validate thecountargument. Invalidcountvalues (e.g. floats, zero, negative) causeerrors to surface deeper in the accounting stack, making them harder to
debug.
This PR adds early input validation for
countwith clear error messages,while preserving the existing chainable return
self.Changes
countis an integer (TypeErrorotherwise)countis positive (ValueErrorotherwise)privacy_accountant_test.pyusing a minimaldummy accountant
This is a backward-compatible improvement to input safety.