Skip to content

[fix] some param will be set to optimizer twice when using MLM transformer heads #965

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.

Sign up for GitHub

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

Open
butterluo wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[fix] some param will be set to optimizer twice when using MLM transformer heads #965

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3b9dc68
When using MMFTransformer with multi heads, an warning will occur whi…
butterluo Jun 10, 2021
a39ac78
Update mmf/models/transformers/base.py
butterluo Jun 13, 2021
423db55
Apply apsdehal's suggestion. Chang var name, and refactor if-else codes
butterluo Jun 13, 2021
15e15a7
Merge branch 'fix_dup_param' of https://github.com/butterluo/mmf into…
butterluo Jun 13, 2021
ae967b2
Merge branch 'master' into fix_dup_param
apsdehal Jun 21, 2021
de0111b
formated the code
butterluo Jun 22, 2021
2e575a6
Merge branch 'fix_dup_param' of https://github.com/butterluo/mmf into…
butterluo Jun 22, 2021
496e54b
formated by black
butterluo Jun 27, 2021
57a2aed
Merge branch 'master' into fix_dup_param
butterluo Jun 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions mmf/models/transformers/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,25 +103,13 @@ def build(self):

def get_optimizer_parameters(self, config):
lr = config.optimizer.params.lr

backbone_param_set = set()
Copy link
Copy Markdown
Contributor

@apsdehal apsdehal Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name this trunk_params_set.

Copy link
Copy Markdown
Author

@butterluo butterluo Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reviewing. I will change my code according to ur suggestion.

param_list = []
parameters = []
head_configs = self.config.get("heads", [])

for name, module in self.named_children():
# Heads can have different learning rates. This is handled here
if name == "heads":
# Parameters in the head which have a separate learning
# rate, are added as a separate param group
for head_config, head in zip(head_configs, self.heads):
parameters, param_list = self.set_lr_for_parameters(
config=head_config,
module_name="{} head".format(head_config.get("type", "MLP")),
base_lr=lr,
module=head,
parameters=parameters,
param_list=param_list,
)
elif name == "encoders":

if name == "encoders":
for key in module:
for modality in self.config.modalities:
if key == modality.key:
Expand All @@ -134,29 +122,48 @@ def get_optimizer_parameters(self, config):
parameters=parameters,
param_list=param_list,
)
else:
elif name != "heads":
Comment thread
butterluo marked this conversation as resolved.
Outdated
# For other modules in trunk, add to same param group
param_list += list(module.named_parameters())

backbone_param_set.update(list(module.parameters()))
head_configs = self.config.get("heads", [])
# Heads can have different learning rates. This is handled here
if len(head_configs) > 0:
# Parameters in the head which have a separate learning
# rate, are added as a separate param group
for head_config, head in zip(head_configs, self.heads):
parameters, param_list = self.set_lr_for_parameters(
config=head_config,
module_name="{} head".format(head_config.get("type", "MLP")),
base_lr=lr,
module=head,
parameters=parameters,
param_list=param_list,
backbone_param_set = backbone_param_set
)
parameters += get_bert_configured_parameters(param_list)

return parameters

def set_lr_for_parameters(
self, config, module_name, base_lr, module, parameters, param_list
self, config, module_name, base_lr, module, parameters, param_list, backbone_param_set = None
Comment thread
butterluo marked this conversation as resolved.
Outdated
):
lr_multiplier = config.get("lr_multiplier", 1.0)
if backbone_param_set is None:
Copy link
Copy Markdown
Contributor

@apsdehal apsdehal Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is None, make it an empty list, [].

Copy link
Copy Markdown
Author

@butterluo butterluo Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reviewing. I will change my code according to ur suggestion.

module_param = list(module.named_parameters())
else:
Copy link
Copy Markdown
Contributor

@apsdehal apsdehal Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, you can remove this else condition.

Copy link
Copy Markdown
Author

@butterluo butterluo Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reviewing. I will change my code according to ur suggestion.

module_param = [ tup for tup in module.named_parameters() if tup[1] not in backbone_param_set ]
if lr_multiplier != 1.0:
logger.info(
f"Setting learning rate of {module_name} to be {base_lr} * {lr_multiplier}."
) # noqa
parameters += get_bert_configured_parameters(
module, base_lr * lr_multiplier
module_param, base_lr * lr_multiplier
)
else:
# Parameters for the modules with same learning rate as
# trunk, add to same param group
param_list += list(module.named_parameters())
param_list += module_param
return parameters, param_list

def build_encoders(self):
Expand Down
⚡