-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Upgrade to pytorch lightning 2.0 #6433
Changes from all commits
bca054e
ae35807
2d39e48
47c1c87
5e35bcc
5d6747d
c2e3e76
2a4274a
e1c71a6
c206bb0
1d15c94
57d8af7
2cac147
cfb9c70
9e0f6c5
5c6ae50
6974037
ed1af3d
ea2fd23
c334149
786e962
74147b7
792234e
a7a9c3f
776fd7d
41e5dda
3276dd5
ec4fb5d
b635c16
11a6e13
876dba9
7a0ed39
4a35223
d368515
c8d28ff
24e77f3
81e3868
ae04106
5765625
0edd455
a73c0f7
0754dc9
7d0396b
b800f93
6617008
03a8fd9
f99433a
52d43bd
be6875a
f519c8a
8988355
00e8150
e0dfba2
104c48a
828df4f
0f0712b
76a9a59
cc14249
c0963c4
16fe164
f8fe51f
6c6f875
0480d9a
697571d
0ab3c3a
3bdeb93
363fc7e
08c874d
109402b
70d6e3e
f5d4307
c5d42a6
4cf8704
df45646
a5ac49d
52f67c4
cbfd81f
42495b2
fd177b2
0e0f9f3
22b8cfd
39fa73a
ef78022
7926361
538b733
55353a1
cbec318
b5b2abf
5a60f11
c8dab2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,10 @@ | |
|
||
@hydra_runner(config_path="conf", config_name="umls_medical_entity_linking_config.yaml") | ||
def main(cfg: DictConfig) -> None: | ||
# PTL 2.0 has find_unused_parameters as False by default, so its required to set it to True | ||
# when there are unused parameters here | ||
if cfg.trainer.strategy == 'ddp': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ericharper @okuchaiev - This thing caused a accuracy / BLEU score regression a few years back. We should train a small model with just If it doesn't hurt memory or execution speed, maybe we can just make this value the default everywhere to not lose accuracy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @titu1994 quick FYI: in previous lightning versions like 1.9 find_unused_parameters was True by default. In lightning 2.0 it was made False by default to improve performance. Did we observe ACC regression with 1.9 versions ? If not, then setting "ddp_find_unused_parameters_true" should be okay right as it was the case by default in 1.9. More details in this PR: lightning PR 16611 |
||
cfg.trainer.strategy = "ddp_find_unused_parameters_true" | ||
logging.info(f"\nConfig Params:\n{OmegaConf.to_yaml(cfg)}") | ||
trainer = Trainer(**cfg.trainer) | ||
exp_manager(trainer, cfg.get("exp_manager", None)) | ||
|
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.
is there a reason for removing this line? removing this can significantly increase time for this CI test
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.
@aklife97 thanks, adding it back in. During the migration to 2.0 there was an issue using this and hence was removed temporarily. Missed to add it back.
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.
@aklife97 sorry my bad.Actually this trainer.limit_val_batches=1 needs to be removed or made as 2 (which is global bs/micro bs). Otherwise, there's an error with the datalaoder_iter.
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.
setting it to 2 sounds good, can we also change other tests that remove it?