-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DP Padding #1225
DP Padding #1225
Conversation
With draft:
However, the added rows by default seem to be majorly slowing down the CI tests, so will need to scale that back so they don't take so long. Also created a table of parameters (in the test code) and printed the results here for expected number of dummies for different parameter sets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to be aside from the tests failing.
Working on updating the approach for DP padding to add separate batches of dummies for OPRF and then right before Breakdown Reveal Agg. To do so working on introducing a Paddable trait so we can reuse as much of the code in either case. Have this doc to summarize the approach, but having issues with trait bounds. @benjaminsavage can you take a look at this new direction? Latest code on the PR but this doc summarizes. https://docs.google.com/document/d/1heIeBE1lMSnmRGAUoopmDuOnKLkR1-_CA9EpqcbPdug/edit#heading=h.f83tb89xflfw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe OPRFPaddingDp
and Error
should move out of this file, if they're used for the secure mode of operation? (It may make sense to do that as a follow-up, though.)
Re @andyleiserson got email of comment but can't find to reply inline.
Yes, I agree it may make sense to move some code around and do some renaming. Would probably put moving into a new PR to make it easier to review and would do a few things
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1225 +/- ##
==========================================
- Coverage 92.74% 92.62% -0.12%
==========================================
Files 198 200 +2
Lines 30780 31409 +629
==========================================
+ Hits 28546 29093 +547
- Misses 2234 2316 +82 ☔ View full report in Codecov by Sentry. |
Sorry about that. I accidentally clicked the button to post a single comment rather than start a review. Then I deleted the comment and re-posted it as part of the review. The code rearrangements you list make sense to me, and I agree it would be better to do in a separate PR. |
Ran draft runs with the
No really impact on draft run performance.
Even with default parameters there are some tests in the CI that run very slowly, so I am also adjusting back to |
@andyleiserson or @benjaminsavage I think this PR is ready to merge now. Ran some draft runs above and have set the parameters to make CI tests run fast enough. Will follow up with a PR that lets us pass in a choice of more conservative parameters. If one of you can merge, that'd be great as I don't have merge access. If you feel this is too many commits, Alex sometimes asks to use the merge option which squashes into one commit on main. Thanks |
PR to add DP padding for both OPRF and the new aggregation.
Right now the code sets parameters internally to
PaddingParameters::default()
which are moderate DP parameters. If we would like to set the parameters at run time we can add to the query config in a future PR.Additional parameter sets are used for some tests:
PaddingParameters::relaxed()
which is needed to make sure the circuit stays very small so concurrency tests and explore many execution paths.PaddingParameters::no_padding()
which is used in some tests where no DP for outputs is being applied.