Better validator for matching parameters #489
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
August 31, 2022 15:12
ffuuugor
requested review from
romovpa,
karthikprasad,
alexanderguzhva and
ashkan-software
facebook-github-bot
added
the
CLA Signed
|
@ffuuugor has updated the pull request. You must reimport the pull request before landing. |
|
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
romovpa
approved these changes
Aug 31, 2022
opacus/tests/privacy_engine_test.py
Outdated
| ): | ||
| model = self._init_model() | ||
| model = PrivacyEngine.get_compatible_module(model) | ||
| optimizer = torch.optim.SGD(model.parameters(), lr=self.LR, momentum=0) | ||
| optimizer = torch.optim.SGD( | ||
| [ |
There was a problem hiding this comment.
I would suggest something like this for readability
model.parameters()
if not opt_exclude_frozen
else [p for p in model.parameters() if p.requires_grad]Same in the _init_vanilla_training
karthikprasad
approved these changes
Aug 31, 2022
|
@ffuuugor has updated the pull request. You must reimport the pull request before landing. |
|
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@ffuuugor has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR resolves #477 and addresses various issues regarding parameter validation
Background
As identified in #432, we want to warn the user if optimizer parameters are different from model parameters (e.g. because the module has been replaced by the ModuleValidator). Optimizer expects the weights to have
.grad_sampleattribute populated by theGradSampleModule, so there's no sense in having optimizer with different set of params to the input modelIssue 1: Comparing values
One problem with how we do it currently is that we're looking for the wrong thing. We use
torch.eq, which performs elementwise comparison and will output True for identical tensors. The problem is - we don't need identical, we need the exact same object - so that optimizer can read per-sample gradients computed by the GradSampleModule.Issue 2: Frozen layers
When freezing some model layers, people are free to choose how to initialize their optimizer: with all
model.parameters()or only those which are not frozen - it doesn't have any material difference on the training process.However, if they do the latter, opacus will complain that model and optimizer weights are mismatched.
This PR
In this PR we address both issues by:
inoperator, which checks object and not its value