Fix per-layer clipping in distributed #347
Conversation
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D34002861 |
There was a problem hiding this comment.
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.
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.
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.
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.
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
alexandresablayrolles
force-pushed
the
export-D34002861
branch
from
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
alexandresablayrolles
force-pushed
the
export-D34002861
branch
from
ffa9e9c to
5c30265
Compare
|
This pull request was exported from Phabricator. Differential Revision: D34002861 |
Problem
DDPPerLayerOptimizerwas 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_stepon step, addingif self._step_skip_queue and self._step_skip_queue[0]when doing per-parameter noise)