Refactor: multi-node save best model only when needed
State: closed | Pull request: yes
Labels: name: stale

name: Run CICD

# What does this PR do ?

Slight refactor of save best model, which may resolve or alleviate #7938 and #7756.

**Collection**: core

# Changelog 
- sets previous_best_path in init, to have it correct on resume training
- compare previous_best_path and best_model_path, reload and save only if necessary, also on tain end
- skip reloading checkpoint in on_save_checkpioint; with previous_best_path being set properly the change can come only from the current state

also

- fix check_slurm, trainer.accelerator_connector.is_slurm_managing_tasks was dropped in lightning 1.6.0, hence the check was always false

# Usage
* You can potentially add a usage example below

```python
# Add a code snippet demonstrating how to use this 
```

# Jenkins CI
To run Jenkins, a NeMo User with write access must comment `jenkins` on the PR.

# Before your PR is "Ready for review"
**Pre checks**:
- [x] Make sure you read and followed [Contributor guidelines](https://github.com/NVIDIA/NeMo/blob/main/CONTRIBUTING.md)
- [ ] 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
- [x] 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](https://github.com/NVIDIA/NeMo/blob/main/CONTRIBUTING.md) contains specific people who can review PRs to various areas.

@titu1994 please review, also check with distributed training

# Additional Information
* Related to #7938 #7756


@athitten @aklife97 anything else that needs to be addressed here? 

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.

@titu1994 anything else that needs to be resolved here?

jenkins

@athitten for final approval, it looks ok to me 

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 status?

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.

ping

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.

This PR was closed because it has been inactive for 7 days since being marked as stale.

@itzsimpl sorry this completely fell through the cracks. My apologies. Since the feature branch is in your personal forked version of NeMo,  can I rebase your feature branch to latest main? There's a minor conflict that needs to be addressed for merging this PR.

Fixed, let me know if you need anything else.

> Fixed, let me know if you need anything else.

Thank you, we need to wait for the CI to pass. I have started the CI

@titu1994 do you want to take another pass over the PR and approve it since its a nemo core PR  ? Sorry this was opened few months ago, but fell through the cracks from my end.

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 @titu1994 anything else that needs to be done here?

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.

This PR was closed because it has been inactive for 7 days since being marked as stale.