Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints in resumed training #8480

stevehuang52 · 2024-02-21T22:51:14Z

What does this PR do ?

Fix PTL2.2 saving multiple *-last.ckpt checkpoints in resumed training.

In current PTL2.2 model_checkpoint.py, it doesn't delete the previous *-last.ckpt checkpoint that the model was resumed on when saving a new *-last.ckpt checkpoint, which results in multiple checkpoints. This will cause errors of "multiple *-last.ckpt checkpoints" when trying to resume from a previous job.

The fix is to check whether the checkpoint to be saved is also a -last.ckpt checkpoint. If it is, delete the previous -last.ckpt if they are under the same directory.

PR Type:

New Feature
Bugfix
Documentation

…om previous run Signed-off-by: He Huang (Steve) <[email protected]>

github-actions · 2024-03-07T01:38:32Z

This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days.

athitten · 2024-03-07T17:15:23Z

@stevehuang52 this problem basically happens when we try to resume 2nd time right ?

stevehuang52 · 2024-03-07T17:19:30Z

@stevehuang52 this problem basically happens when we try to resume 2nd time right ?

@athitten Yes, it happens when we resume for the 2nd time, the first resume is fine

athitten · 2024-03-07T22:47:12Z

nemo/utils/callbacks/nemo_model_checkpoint.py

+                pass
+            else:
+                return False
+        if self.dirpath is None:


@stevehuang52 why have we added if self.dirpath is None: instead of assert self.dirpath is not None which is the case in PTL code here

@athitten I remembered @titu1994 told me to avoid using assert in nemo, and to raise exceptions instead so that they can be caught by error handling code. However, it's up to you if you would like to keep it identical to PTL

Agreed, assert hides actual error, we should raise it properly with useful error message (like here)

athitten · 2024-03-08T19:28:36Z

nemo/utils/callbacks/nemo_model_checkpoint.py

@@ -454,3 +454,29 @@ def _remove_unfinished_checkpoints(checkpoint_dir: Union[Path, str]) -> None:
        # delete markers
        for marker_path in existing_marker_filepaths:
            os.remove(marker_path)
+
+    def _should_remove_checkpoint(self, trainer: "pl.Trainer", previous: str, current: str) -> bool:


@stevehuang52 we want to avoid overriding PTL's protected methods in NeMo as much as possible so that NeMo doesn't break when PTL changes something in the protected methods.

Maybe we can create this PR in PTL itself ? Since its not a lot of modification and just addition of couple of lines in _should_remove_checkpoint function.

@athitten thanks for the advice, I've created a PR to PTL Lightning-AI/pytorch-lightning#19613, let's see if they'll approve

Great, thank you very much @stevehuang52.

Seems they closed the PR above. Is the resolution implemented here?

@titu1994 Yeah they don't want to do the fix in PTL, we will do the fix in NeMo (which is this PR).

Signed-off-by: He Huang (Steve) <[email protected]>

stevehuang52 · 2024-03-18T15:15:33Z

jenkins

athitten · 2024-03-19T16:42:41Z

jenkins

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

athitten · 2024-03-19T20:05:13Z

jenkins

stevehuang52 · 2024-03-21T20:03:34Z

jenkins

stevehuang52 · 2024-03-22T14:16:10Z

@athitten seems that Jenkins passed without issues. Could you please review the PR again and see if there's other changes or tests needed? Thanks~

athitten

LGTM, thanks very much @stevehuang52

…ing (#8480) * Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints when resuming from previous run Signed-off-by: He Huang (Steve) <[email protected]> * Fix missing import Signed-off-by: He Huang (Steve) <[email protected]> * fix broken test Signed-off-by: stevehuang52 <[email protected]> --------- Signed-off-by: He Huang (Steve) <[email protected]> Signed-off-by: stevehuang52 <[email protected]> Co-authored-by: Abhishree Thittenamane <[email protected]> Signed-off-by: Michal Futrega <[email protected]>

…ed training (#8480)" This reverts commit 11b7a73.

* add init griffin * remove unnecessary imports * add sft * add sft model init * add text gen starategy for Griffin no cache * test SFT * minor fix to config * fix logprob output issue * sft WS fixed * replace trainer in conversion script * Revert "Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints in resumed training (#8480)" This reverts commit 11b7a73. * Revert "FSDP update to PTL 2.2 (#8658)" This reverts commit 355e36c. * init dist opt * add peft * fix generate script * convert to HF format * further cleanups * minor fix * minor fix * more refactoring * remove local path from config * undo unnecessary changes * remove pretraining * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix val param sync * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * minor fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Addresing MR comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * code ql fixed * more code ql * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * address comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add jenkins * remove jenkins for momentarily * add reqs for griffin * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add req test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add reqs to nlp * add reqs to nlp * replace torch scan * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * jit fusion for embedding decoder * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * jit fusion for embedding decoder * add fix to rglru * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Ali Taghibakhshi <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Harper <[email protected]>

…ing (NVIDIA#8480) * Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints when resuming from previous run Signed-off-by: He Huang (Steve) <[email protected]> * Fix missing import Signed-off-by: He Huang (Steve) <[email protected]> * fix broken test Signed-off-by: stevehuang52 <[email protected]> --------- Signed-off-by: He Huang (Steve) <[email protected]> Signed-off-by: stevehuang52 <[email protected]> Co-authored-by: Abhishree Thittenamane <[email protected]>

* add init griffin * remove unnecessary imports * add sft * add sft model init * add text gen starategy for Griffin no cache * test SFT * minor fix to config * fix logprob output issue * sft WS fixed * replace trainer in conversion script * Revert "Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints in resumed training (NVIDIA#8480)" This reverts commit 445fb5c. * Revert "FSDP update to PTL 2.2 (NVIDIA#8658)" This reverts commit 60364e9. * init dist opt * add peft * fix generate script * convert to HF format * further cleanups * minor fix * minor fix * more refactoring * remove local path from config * undo unnecessary changes * remove pretraining * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix val param sync * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * minor fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Addresing MR comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * code ql fixed * more code ql * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * address comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add jenkins * remove jenkins for momentarily * add reqs for griffin * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add req test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add reqs to nlp * add reqs to nlp * replace torch scan * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * jit fusion for embedding decoder * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * jit fusion for embedding decoder * add fix to rglru * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Ali Taghibakhshi <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Harper <[email protected]>

Fix PTL2.2 saving multiple *-last.ckpt checkpoints when resuming fr…

fe6ac3e

…om previous run Signed-off-by: He Huang (Steve) <[email protected]>

stevehuang52 requested a review from ericharper February 21, 2024 22:51

ericharper requested a review from athitten February 21, 2024 23:17

github-actions bot added the stale label Mar 7, 2024

Merge branch 'main' into stevehuang52-patch-4

ecafda0

stevehuang52 removed the stale label Mar 7, 2024

athitten reviewed Mar 7, 2024

View reviewed changes

athitten reviewed Mar 8, 2024

View reviewed changes

stevehuang52 added 2 commits March 18, 2024 11:05

Fix missing import

29f8652

Signed-off-by: He Huang (Steve) <[email protected]>

Merge branch 'main' into stevehuang52-patch-4

1d01c30

Merge branch 'main' into stevehuang52-patch-4

e9d670c

fix broken test

88e5bf4

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

github-actions bot added the core Changes to NeMo Core label Mar 19, 2024

stevehuang52 requested a review from athitten March 19, 2024 19:39

Merge branch 'main' into stevehuang52-patch-4

90e6fc8

stevehuang52 requested a review from titu1994 March 20, 2024 18:58

stevehuang52 added 2 commits March 21, 2024 14:12

Merge branch 'main' into stevehuang52-patch-4

7d83793

Merge branch 'main' into stevehuang52-patch-4

4420a2b

athitten approved these changes Mar 23, 2024

View reviewed changes

athitten merged commit 11b7a73 into main Mar 23, 2024

athitten deleted the stevehuang52-patch-4 branch March 23, 2024 03:14

JRD971000 pushed a commit that referenced this pull request Apr 10, 2024

Revert "Fix PTL2.2 saving multiple *-last.ckpt checkpoints in resum…

57dd61d

…ed training (#8480)" This reverts commit 11b7a73.

JRD971000 added a commit that referenced this pull request Apr 11, 2024

Revert "Fix PTL2.2 saving multiple *-last.ckpt checkpoints in resum…

390af24

…ed training (#8480)" This reverts commit 11b7a73.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints in resumed training #8480

Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints in resumed training #8480

stevehuang52 commented Feb 21, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024

athitten commented Mar 7, 2024

stevehuang52 commented Mar 7, 2024 •

edited

Loading

athitten Mar 7, 2024

stevehuang52 Mar 8, 2024

titu1994 Mar 21, 2024

athitten Mar 8, 2024 •

edited

Loading

stevehuang52 Mar 11, 2024

athitten Mar 11, 2024

titu1994 Mar 21, 2024

stevehuang52 Mar 21, 2024

stevehuang52 commented Mar 18, 2024

athitten commented Mar 19, 2024

athitten commented Mar 19, 2024

stevehuang52 commented Mar 21, 2024

stevehuang52 commented Mar 22, 2024

athitten left a comment

Fix PTL2.2 saving multiple *-last.ckpt checkpoints in resumed training #8480

Fix PTL2.2 saving multiple *-last.ckpt checkpoints in resumed training #8480

Conversation

stevehuang52 commented Feb 21, 2024 • edited Loading

What does this PR do ?

github-actions bot commented Mar 7, 2024

athitten commented Mar 7, 2024

stevehuang52 commented Mar 7, 2024 • edited Loading

athitten Mar 7, 2024

Choose a reason for hiding this comment

stevehuang52 Mar 8, 2024

Choose a reason for hiding this comment

titu1994 Mar 21, 2024

Choose a reason for hiding this comment

athitten Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

stevehuang52 Mar 11, 2024

Choose a reason for hiding this comment

athitten Mar 11, 2024

Choose a reason for hiding this comment

titu1994 Mar 21, 2024

Choose a reason for hiding this comment

stevehuang52 Mar 21, 2024

Choose a reason for hiding this comment

stevehuang52 commented Mar 18, 2024

athitten commented Mar 19, 2024

athitten commented Mar 19, 2024

stevehuang52 commented Mar 21, 2024

stevehuang52 commented Mar 22, 2024

athitten left a comment

Choose a reason for hiding this comment

Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints in resumed training #8480

Fix PTL2.2 saving multiple `*-last.ckpt` checkpoints in resumed training #8480

stevehuang52 commented Feb 21, 2024 •

edited

Loading

stevehuang52 commented Mar 7, 2024 •

edited

Loading

athitten Mar 8, 2024 •

edited

Loading