Optimized differential privacy implementation #997
Conversation
tharvik
left a comment
tharvik
left a comment
There was a problem hiding this comment.
thanks for the great work! I added a few comments on how to improve parts of it but the core feature is there 🎉
btw, I don't remember if we talked about adding a test for the feature. for an integration test, you can add one in server/tests/e2e/federated.spec.ts trying out the feature and if you have some hardening/units tests in mind, you can also some in discojs/src/privacy.spec.ts to ensure that it works in many conditions.
discojs/src/training/trainer.ts
Outdated
| ret = privacy.addNoise(ret, options.noiseScale); | ||
| // Adding Gaussian noise for DP | ||
| if (options.noiseScale !== undefined && options.delta !== undefined){ | ||
| const dpDefaultRadius = options.dpDefaultClippingRadius; // options.dpDefaultClippingRadius should be a number |
There was a problem hiding this comment.
there are two type of radius, one for BFT and one for DP, consider adding a specific object for BFT in the privacy's schema and rename dpDefaultClippingRadius to clippingRadius
you can also set defaults directly in the schema with the default method
discojs/src/training/trainer.ts
Outdated
| if (options.clippingRadius !== undefined){ | ||
| /** When clipping radius for BFT is smaller than the improved clipping radius */ | ||
| if (Array.isArray(dpClippingRadius)&&options.clippingRadius < Math.min(...dpClippingRadius)) | ||
| ret = current.add(await privacy.addNoise(weightsProgress, epsilon, delta, options.clippingRadius)); | ||
| else if(typeof dpClippingRadius == "number" && options.clippingRadius < dpClippingRadius) | ||
| ret = current.add(await privacy.addNoise(weightsProgress, epsilon, delta, options.clippingRadius)); | ||
| }else{ | ||
| ret = current.add(await privacy.addNoise(weightsProgress, epsilon, delta, dpClippingRadius)); | ||
| } |
There was a problem hiding this comment.
theses three code branches are quite similar, surely there is a way to commonize logic
| <div v-show="differentialPrivacy" class="flex flex-col"> | ||
| <FormLabel | ||
| label="Default clipping norm: Initial limit on weight update size for differential privacy" | ||
| type="required" |
There was a problem hiding this comment.
here is it required but not in the schema, is it wanted?
| type="required" | ||
| > | ||
| <FormField | ||
| name="trainingInformation.privacy.delta" |
There was a problem hiding this comment.
we have to readd (yeah, that's bothering but I wasn't able to find a way to map over zod schema to transform theses) in the nonLocalNetwork schema below. that let the form manager know what is a valid value and warn the user about it. it has to have the same shape as the base schema otherwise it will get rejected by the server.
Gaussian noise calibration implementation
Adaptive clipping mechanism
cli testing tool modification
Webapp interface updates