Fixes for lightning 2.0 upgrade #7176

athitten · 2023-08-07T22:00:26Z

What does this PR do ?

Fix the following issues with the previous lightning 2.0 upgrade PR:

Decrease val_check_interval to 1 while resuming from a ckpt in some CI tests where max_steps is same as that of training and lightning 2.0 errors: fit_loop.py#L259. Lightning 2.0 calls setup_data right at the beginning of fit_loop.run unlike 1.9 that did it only when self.skip for fit was False, hence the error din't show up with 1.9
Remove the usage of trainer._checkpoint_connector as it's redundant in lightning 2.0 and use trainer.ckpt_path directly. Also having trainer._checkpoint_connector = _CheckpointConnector(trainer) overrode ckpt_path with None leading to starting training from scratch during resuming from a ckpt.
Remove optimizer_idx arg in optimizer_step of MegatronHalfPrecisionPlugin class as the parent function in lightning does not have it anymore.

Collection: [Note which collection this PR will affect]

Changelog

Add specific line by line info of high level changes in this PR.

Usage

You can potentially add a usage example below

# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

Make sure you read and followed Contributor guidelines
Did you write any new necessary tests?
Did you add or update any necessary documentation?
Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
- Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

New Feature
Bugfix
Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

Related to # (issue)

titu1994 · 2023-08-09T04:30:15Z

Jenkinsfile

@@ -3603,6 +3603,7 @@ assert_frame_equal(training_curve, gt_curve, rtol=1e-3, atol=1e-3)"'''
        trainer.precision=16 \
        trainer.gradient_clip_val=1.0 \
        exp_manager.exp_dir=examples/nlp/language_modeling/gpt_pretrain_results \
+        exp_manager.resume_if_exits=False \


resume_if_exists*

Thanks @titu1994, corrected the typo. Also the reason behind setting this to False is that this test aka Megatron GPT Pretraining and Resume Training PP=2 is picking up the last checkpoint from the previous test aka Megatron GPT with KERPLE Pretraining and Resume Training TP=2 and it errors out as it tries to find that checkpoint in the current test's exp_dir, but can't find it.

This was happening bcoz I had commented these lines out by mistake in the lightning 2.0 upgrade PR: https://github.com/NVIDIA/NeMo/blob/main/Jenkinsfile#L3583-L3584. Fixing it ASAP in a separate PR.

Signed-off-by: Abhishree <[email protected]>

…for NMT Signed-off-by: Abhishree <[email protected]>

1) Add resume_if_exists=False for Megatron GPT Pretraining and Resume Training PP=2 as it can resume from the checkpoint of the previous model test in CI leading to Error Signed-off-by: Abhishree <[email protected]>

1) Remove arg optimizer_idx in optimizer_step func as the arg is not used my parent func of lightning Signed-off-by: Abhishree <[email protected]>

Signed-off-by: Abhishree <[email protected]>

…en(dataloader) Signed-off-by: Abhishree <[email protected]>

Signed-off-by: Abhishree <[email protected]>

for more information, see https://pre-commit.ci

ericharper

LGTM. Thanks!

* Remove trainer._checkpoint_connector = _CheckpointConnector(trainer) Signed-off-by: Abhishree <[email protected]> * Remove trainer._checkpoint_connector = _CheckpointConnector(trainer) for NMT Signed-off-by: Abhishree <[email protected]> * Add resume_if_exists=False in JenkinsFile 1) Add resume_if_exists=False for Megatron GPT Pretraining and Resume Training PP=2 as it can resume from the checkpoint of the previous model test in CI leading to Error Signed-off-by: Abhishree <[email protected]> * Remove optimizer_idx in optimizer_step and fix typo 1) Remove arg optimizer_idx in optimizer_step func as the arg is not used my parent func of lightning Signed-off-by: Abhishree <[email protected]> * Remove resume_if_exists=False in JenkinsFile Signed-off-by: Abhishree <[email protected]> * Make trainer.val_check_interval=1 for few tests in JenkinsFile Signed-off-by: Abhishree <[email protected]> * Change val_check_interval in JenkinsFile during resume to less than len(dataloader) Signed-off-by: Abhishree <[email protected]> * Change val_check_interval to 1 for Megatron T5 with ALiBi resume Signed-off-by: Abhishree <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Abhishree <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Remove trainer._checkpoint_connector = _CheckpointConnector(trainer) Signed-off-by: Abhishree <[email protected]> * Remove trainer._checkpoint_connector = _CheckpointConnector(trainer) for NMT Signed-off-by: Abhishree <[email protected]> * Add resume_if_exists=False in JenkinsFile 1) Add resume_if_exists=False for Megatron GPT Pretraining and Resume Training PP=2 as it can resume from the checkpoint of the previous model test in CI leading to Error Signed-off-by: Abhishree <[email protected]> * Remove optimizer_idx in optimizer_step and fix typo 1) Remove arg optimizer_idx in optimizer_step func as the arg is not used my parent func of lightning Signed-off-by: Abhishree <[email protected]> * Remove resume_if_exists=False in JenkinsFile Signed-off-by: Abhishree <[email protected]> * Make trainer.val_check_interval=1 for few tests in JenkinsFile Signed-off-by: Abhishree <[email protected]> * Change val_check_interval in JenkinsFile during resume to less than len(dataloader) Signed-off-by: Abhishree <[email protected]> * Change val_check_interval to 1 for Megatron T5 with ALiBi resume Signed-off-by: Abhishree <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Abhishree <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Signed-off-by: dorotat <[email protected]>

* Remove trainer._checkpoint_connector = _CheckpointConnector(trainer) Signed-off-by: Abhishree <[email protected]> * Remove trainer._checkpoint_connector = _CheckpointConnector(trainer) for NMT Signed-off-by: Abhishree <[email protected]> * Add resume_if_exists=False in JenkinsFile 1) Add resume_if_exists=False for Megatron GPT Pretraining and Resume Training PP=2 as it can resume from the checkpoint of the previous model test in CI leading to Error Signed-off-by: Abhishree <[email protected]> * Remove optimizer_idx in optimizer_step and fix typo 1) Remove arg optimizer_idx in optimizer_step func as the arg is not used my parent func of lightning Signed-off-by: Abhishree <[email protected]> * Remove resume_if_exists=False in JenkinsFile Signed-off-by: Abhishree <[email protected]> * Make trainer.val_check_interval=1 for few tests in JenkinsFile Signed-off-by: Abhishree <[email protected]> * Change val_check_interval in JenkinsFile during resume to less than len(dataloader) Signed-off-by: Abhishree <[email protected]> * Change val_check_interval to 1 for Megatron T5 with ALiBi resume Signed-off-by: Abhishree <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Abhishree <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

github-actions bot added the NLP label Aug 7, 2023

athitten force-pushed the athitten/ptl_2.0_fixes branch from 044469b to d8c23ac Compare August 9, 2023 00:20

github-actions bot added the CI label Aug 9, 2023

titu1994 reviewed Aug 9, 2023

View reviewed changes

athitten force-pushed the athitten/ptl_2.0_fixes branch 2 times, most recently from 2cdeb16 to d0140e2 Compare August 10, 2023 00:14

github-actions bot added the core Changes to NeMo Core label Aug 10, 2023

athitten force-pushed the athitten/ptl_2.0_fixes branch 2 times, most recently from 09c2033 to 3a747c8 Compare August 10, 2023 05:17

github-actions bot removed core Changes to NeMo Core CI labels Aug 10, 2023

athitten force-pushed the athitten/ptl_2.0_fixes branch from 636d022 to 552b3b7 Compare August 11, 2023 01:32

github-actions bot added the CI label Aug 11, 2023

athitten force-pushed the athitten/ptl_2.0_fixes branch from 5d20bc2 to ecb0642 Compare August 11, 2023 17:16

athitten added 8 commits August 11, 2023 13:13

Remove trainer._checkpoint_connector = _CheckpointConnector(trainer)

9fa29d0

Signed-off-by: Abhishree <[email protected]>

Remove trainer._checkpoint_connector = _CheckpointConnector(trainer) …

2e98abf

…for NMT Signed-off-by: Abhishree <[email protected]>

Add resume_if_exists=False in JenkinsFile

f214ea4

1) Add resume_if_exists=False for Megatron GPT Pretraining and Resume Training PP=2 as it can resume from the checkpoint of the previous model test in CI leading to Error Signed-off-by: Abhishree <[email protected]>

Remove optimizer_idx in optimizer_step and fix typo

5cf206a

1) Remove arg optimizer_idx in optimizer_step func as the arg is not used my parent func of lightning Signed-off-by: Abhishree <[email protected]>

Remove resume_if_exists=False in JenkinsFile

0bdee44

Signed-off-by: Abhishree <[email protected]>

Make trainer.val_check_interval=1 for few tests in JenkinsFile

d0887f4

Signed-off-by: Abhishree <[email protected]>

Change val_check_interval in JenkinsFile during resume to less than l…

cdd6e8b

…en(dataloader) Signed-off-by: Abhishree <[email protected]>

Change val_check_interval to 1 for Megatron T5 with ALiBi resume

20a0275

Signed-off-by: Abhishree <[email protected]>

athitten force-pushed the athitten/ptl_2.0_fixes branch from 86c60d5 to 20a0275 Compare August 11, 2023 20:13

[pre-commit.ci] auto fixes from pre-commit.com hooks

287c097

for more information, see https://pre-commit.ci

ericharper approved these changes Aug 11, 2023

View reviewed changes

Merge branch 'main' into athitten/ptl_2.0_fixes

2f55903

ericharper marked this pull request as ready for review August 12, 2023 01:39

ericharper merged commit ab749e4 into main Aug 12, 2023
15 checks passed

ericharper deleted the athitten/ptl_2.0_fixes branch August 12, 2023 04:51

ericharper mentioned this pull request Aug 12, 2023

Fix val_check_interval on Resume tests #7215

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for lightning 2.0 upgrade #7176

Fixes for lightning 2.0 upgrade #7176

athitten commented Aug 7, 2023 •

edited

Loading

titu1994 Aug 9, 2023

athitten Aug 9, 2023

athitten Aug 9, 2023

ericharper left a comment

Fixes for lightning 2.0 upgrade #7176

Fixes for lightning 2.0 upgrade #7176

Conversation

athitten commented Aug 7, 2023 • edited Loading

What does this PR do ?

Changelog

Usage

Before your PR is "Ready for review"

Who can review?

Additional Information

titu1994 Aug 9, 2023

Choose a reason for hiding this comment

athitten Aug 9, 2023

Choose a reason for hiding this comment

athitten Aug 9, 2023

Choose a reason for hiding this comment

ericharper left a comment

Choose a reason for hiding this comment

athitten commented Aug 7, 2023 •

edited

Loading