Skip to content
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

Jump to bottom

Fix issues with PL 1.8 #5353

Merged
merged 29 commits into from Nov 28, 2022
Merged

Fix issues with PL 1.8 #5353

merged 29 commits into from Nov 28, 2022

Conversation

SeanNaren
Copy link
Collaborator

What does this PR do ?

Attempts to fix all issues after many deprecations/lightning lite becoming the backbone for a lot of the underlying pytorch lightning code.

cc @titu1994

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

@SeanNaren
Fix issues with PL 1.8
477baaa 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren SeanNaren mentioned this pull request Nov 8, 2022
Merged
8 tasks
@SeanNaren
Set scripting variable
c6eb78c 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Fix missing arg
4cc6301 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Cleanup list
25e0f77 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Fix reference
60db72b 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Merge branch 'main' into feat/lightning_1.8_support
0ed8d1e
@SeanNaren
Try to fix hanging EMA test
6ef98e7 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Missing \
e25704c 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Add strategy
4da05eb 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
See if setting the chdir fixes the hanging DDP test
4e6913c 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
titu1994
titu1994 reviewed Nov 9, 2022
View reviewed changes
nemo/collections/tts/models/fastpitch.py Show resolved Hide resolved
nemo/core/config/hydra_runner.py Outdated Show resolved Hide resolved
nemo/utils/exp_manager.py Show resolved Hide resolved
SeanNaren and others added 7 commits November 10, 2022 11:49
@SeanNaren
See if removing the subdir setter fixes the issue
07d4292 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Merge branch 'main' into feat/lightning_1.8_support
9c6307a 
# Conflicts:
#	nemo/utils/exp_manager.py
@SeanNaren
Remove checks
15087e5 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Try [0,1] for devices
4a01cec 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@okuchaiev
Merge branch 'main' into feat/lightning_1.8_support
6c3e417
@okuchaiev
Merge branch 'main' into feat/lightning_1.8_support
1aad730
@SeanNaren
Add code back
b1add40 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren SeanNaren mentioned this pull request Nov 15, 2022
Closed
@SeanNaren
Remove space
8c7d7e0 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Copy link
Collaborator Author

This PR should hopefully unblock the move to 1.8: Lightning-AI/pytorch-lightning#15737 (review)

We'll just wait till it gets into a minor release version (which hopefully would be tomorrow).

@SeanNaren
Merge branch 'main' into feat/lightning_1.8_support
dce4918 
# Conflicts:
#	nemo/core/classes/exportable.py
@SeanNaren
Update requirements
9e8ab6b 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Swap import path
b33501d 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Update references
580654c 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Fix deprecated variables
ed8df6f 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Fix missing var
12d7ea2 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Fix var
0e8e59a 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
SeanNaren
SeanNaren commented Nov 23, 2022
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
@SeanNaren
Revert changes
d672304 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren SeanNaren marked this pull request as ready for review November 23, 2022 21:00
titu1994
titu1994 reviewed Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

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

Overall, incredible work @SeanNaren ! Minor comments and needs one other reviewer since it's a core change

nemo/core/classes/exportable.py
@@ -128,7 +129,7 @@ def _export(
# Set module mode
with torch.onnx.select_model_mode_for_export(
self, training
), torch.inference_mode(), torch.no_grad(), torch.jit.optimized_execution(True):
), torch.inference_mode(), torch.no_grad(), torch.jit.optimized_execution(True), _jit_is_scripting():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm we don't use lightning during export, is this method safe ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would the tests have confirmed if this is fine? The underlying code is simple to what is happening:

In Lightning:

@contextmanager
def _jit_is_scripting() -> Generator:
    """Workaround for https://github.com/pytorch/pytorch/issues/67146."""
    LightningModule._jit_is_scripting = True
    try:
        yield
    finally:
        LightningModule._jit_is_scripting = False

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm i dont think it would be caught in tests, lets keep it for now

tests/hydra/test_hydra_runner.py Outdated Show resolved Hide resolved
@SeanNaren
Address review
d614378 
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 24, 2022

This pull request fixes 1 alert when merging d614378 into 2430d0f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@titu1994 titu1994 merged commit d349f4a into main Nov 28, 2022
@titu1994 titu1994 deleted the feat/lightning_1.8_support branch November 28, 2022 21:30
hainan-xv pushed a commit to hainan-xv/NeMo that referenced this pull request Nov 29, 2022
@SeanNaren @okuchaiev
Fix issues with PL 1.8 (NVIDIA#5353)
ad3dd9f 
* Fix issues with PL 1.8

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Set scripting variable

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing arg

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Cleanup list

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix reference

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try to fix hanging EMA test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Missing \

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add strategy

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if setting the chdir fixes the hanging DDP test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if removing the subdir setter fixes the issue

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove checks

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try [0,1] for devices

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add code back

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove space

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update requirements

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Swap import path

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update references

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix deprecated variables

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Revert changes

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Address review

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Co-authored-by: Oleksii Kuchaiev <okuchaiev@users.noreply.github.com>
Signed-off-by: Hainan Xu <hainanx@nvidia.com>
hainan-xv pushed a commit to hainan-xv/NeMo that referenced this pull request Nov 29, 2022
@SeanNaren @okuchaiev
Fix issues with PL 1.8 (NVIDIA#5353)
84a9d61 
* Fix issues with PL 1.8

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Set scripting variable

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing arg

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Cleanup list

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix reference

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try to fix hanging EMA test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Missing \

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add strategy

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if setting the chdir fixes the hanging DDP test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if removing the subdir setter fixes the issue

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove checks

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try [0,1] for devices

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add code back

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove space

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update requirements

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Swap import path

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update references

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix deprecated variables

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Revert changes

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Address review

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Co-authored-by: Oleksii Kuchaiev <okuchaiev@users.noreply.github.com>
Signed-off-by: Hainan Xu <hainanx@nvidia.com>
hainan-xv pushed a commit to hainan-xv/NeMo that referenced this pull request Dec 5, 2022
@SeanNaren @okuchaiev
Fix issues with PL 1.8 (NVIDIA#5353)
0d69e94 
* Fix issues with PL 1.8

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Set scripting variable

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing arg

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Cleanup list

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix reference

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try to fix hanging EMA test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Missing \

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add strategy

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if setting the chdir fixes the hanging DDP test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if removing the subdir setter fixes the issue

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove checks

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try [0,1] for devices

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add code back

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove space

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update requirements

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Swap import path

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update references

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix deprecated variables

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Revert changes

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Address review

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Co-authored-by: Oleksii Kuchaiev <okuchaiev@users.noreply.github.com>
Signed-off-by: Hainan Xu <hainanx@nvidia.com>
andrusenkoau pushed a commit to andrusenkoau/NeMo that referenced this pull request Jan 5, 2023
@SeanNaren @okuchaiev @andrusenkoau
Fix issues with PL 1.8 (NVIDIA#5353)
6e26d60 
* Fix issues with PL 1.8

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Set scripting variable

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing arg

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Cleanup list

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix reference

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try to fix hanging EMA test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Missing \

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add strategy

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if setting the chdir fixes the hanging DDP test

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* See if removing the subdir setter fixes the issue

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove checks

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Try [0,1] for devices

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Add code back

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Remove space

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update requirements

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Swap import path

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Update references

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix deprecated variables

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix missing var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Fix var

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Revert changes

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

* Address review

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>

Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Co-authored-by: Oleksii Kuchaiev <okuchaiev@users.noreply.github.com>
Signed-off-by: andrusenkoau <andrusenkoau@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@titu1994 titu1994 titu1994 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SeanNaren @titu1994 @okuchaiev