-
Notifications
You must be signed in to change notification settings - Fork 360
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
Fix per-layer clipping in distributed #347
Fix per-layer clipping in distributed #347
Conversation
This pull request was exported from Phabricator. Differential Revision: D34002861 |
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.
Added some comments.
Also, @alexandresablayrolles may I ask you to describe in a few words the problem and the proposed solution? I didn't work on DDP and it's hard for me to validate the logic.
opacus/privacy_engine.py
Outdated
@@ -327,7 +328,7 @@ def make_private( | |||
if noise_generator and self.secure_mode: | |||
raise ValueError("Passing seed is prohibited in secure mode") | |||
|
|||
distributed = type(module) is DPDDP | |||
distributed = type(module) is DPDDP or type(module) is DDP |
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.
Actually, we should avoid type comparisons like this. Do this instead:
isinstance(module, (DPDDP, DDP))
opacus/tests/multigpu_gradcheck.py
Outdated
print( | ||
f"Running {clipping}-clipping DDP {withdp} differential privacy example on rank {rank}." | ||
) |
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.
We shouldn't use prints anywhere in the library, in tests too. It may have unexpected side effects.
If we really needs to do this output here, we should use logging:
import logging
logging.info(f"Running {clipping}-clipping DDP {withdp} differential privacy example on rank {rank}.")
@@ -77,7 +80,7 @@ def demo_basic(rank, weight, world_size, dp, clipping): | |||
if dp: | |||
max_grad_norm = 1e8 |
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.
Let's follow pep-8 and name this constant with capital letters: MAX_GRAD_NORM
. This will emphasize that this thing is pre-defined and hard coded.
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.
It's not a constant: if clipping is per-layer, it becomes a list, see line 80.
This pull request was exported from Phabricator. Differential Revision: D34002861 |
Summary: Pull Request resolved: pytorch#347 PrivacyEngine now recognizes DDP as well (in the case of per layer clipping) DDPPerLayerOptimizer is updated with pre_step Reviewed By: pierrestock Differential Revision: D34002861 fbshipit-source-id: e968c7c2890677e41a622373e956ae49f787a75d
54de3f6
to
ffa9e9c
Compare
Summary: Pull Request resolved: pytorch#347 PrivacyEngine now recognizes DDP as well (in the case of per layer clipping) DDPPerLayerOptimizer is updated with pre_step Reviewed By: pierrestock Differential Revision: D34002861 fbshipit-source-id: 467e9151b2018d68f35dcaf801dffc48d2c88297
ffa9e9c
to
5c30265
Compare
This pull request was exported from Phabricator. Differential Revision: D34002861 |
Problem
DDPPerLayerOptimizer
was buggy and our test did not catch that.Changes
Per-layer clipping uses the regular pytorch DDP (Distributed Data Parallel) instead of our custom DPDDP (Differentially Private DDP). Our PrivacyEngine only checked for DPDDP and not for DDP.
I fixed PrivacyEngine to have distributed=True if wrapper is DDP instead of DPDDP.
This made the test non-buggy and I modified DPDDP to pass the test (adding
_check_skip_next_step
on step, addingif self._step_skip_queue and self._step_skip_queue[0]
when doing per-parameter noise)