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 TBPTT example #20528

Merged
merged 5 commits into from
Jan 6, 2025
Merged

Fix TBPTT example #20528

merged 5 commits into from
Jan 6, 2025

Conversation

lantiga
Copy link
Collaborator

@lantiga lantiga commented Jan 6, 2025 •
edited by github-actions bot
Loading

What does this PR do?

Fixes #20517

Thanks to @simon-bachhuber for the fix

📚 Documentation preview 📚: https://pytorch-lightning--20528.org.readthedocs.build/en/20528/

@github-actions github-actions bot added docs Documentation related pl Generic label for PyTorch Lightning package labels Jan 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2025 •
edited
Loading

⚡ Required checks status: All passing 🟢

Groups summary

🟢 pytorch_lightning: Tests workflow
Check ID Status
pl-cpu (macOS-14, lightning, 3.9, 2.1, oldest) success ✅
pl-cpu (macOS-14, lightning, 3.10, 2.1) success ✅
pl-cpu (macOS-14, lightning, 3.11, 2.2.2) success ✅
pl-cpu (macOS-14, lightning, 3.11, 2.3) success ✅
pl-cpu (macOS-14, lightning, 3.12.7, 2.4.1) success ✅
pl-cpu (macOS-14, lightning, 3.12.7, 2.5.1) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.9, 2.1, oldest) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.10, 2.1) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.11, 2.2.2) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.11, 2.3) success ✅
pl-cpu (ubuntu-22.04, lightning, 3.12.7, 2.4.1) success ✅
pl-cpu (ubuntu-22.04, lightning, 3.12.7, 2.5.1) success ✅
pl-cpu (windows-2022, lightning, 3.9, 2.1, oldest) success ✅
pl-cpu (windows-2022, lightning, 3.10, 2.1) success ✅
pl-cpu (windows-2022, lightning, 3.11, 2.2.2) success ✅
pl-cpu (windows-2022, lightning, 3.11, 2.3) success ✅
pl-cpu (windows-2022, lightning, 3.12.7, 2.4.1) success ✅
pl-cpu (windows-2022, lightning, 3.12.7, 2.5.1) success ✅
pl-cpu (macOS-14, pytorch, 3.9, 2.1) success ✅
pl-cpu (ubuntu-20.04, pytorch, 3.9, 2.1) success ✅
pl-cpu (windows-2022, pytorch, 3.9, 2.1) success ✅
pl-cpu (macOS-14, pytorch, 3.12.7, 2.5.1) success ✅
pl-cpu (ubuntu-22.04, pytorch, 3.12.7, 2.5.1) success ✅
pl-cpu (windows-2022, pytorch, 3.12.7, 2.5.1) success ✅

These checks are required after the changes to tests/tests_pytorch/helpers/advanced_models.py, tests/tests_pytorch/helpers/test_models.py.

🟢 pytorch_lightning: Azure GPU
Check ID Status
pytorch-lightning (GPUs) (testing Lightning | latest) success ✅
pytorch-lightning (GPUs) (testing PyTorch | latest) success ✅

These checks are required after the changes to tests/tests_pytorch/helpers/advanced_models.py, tests/tests_pytorch/helpers/test_models.py.

🟢 pytorch_lightning: Docs
Check ID Status
docs-make (pytorch, doctest) success ✅
docs-make (pytorch, html) success ✅

These checks are required after the changes to docs/source-pytorch/common/tbptt.rst.

Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

@Borda
Copy link
Member

Borda commented Jan 6, 2025

could we also add a test for it?

@t-vi
Copy link
Contributor

t-vi commented Jan 6, 2025

@Borda given that it is in the example, wouldn't it be in the doc build (or in a bit of code that cannot be run on the CI due to being too large)?

@lantiga
Copy link
Collaborator Author

lantiga commented Jan 6, 2025 •
edited
Loading

Actually, I would like this to be a copy-pastable example, which this isn't. More work to be done.

@lantiga
Copy link
Collaborator Author

lantiga commented Jan 6, 2025

Alright, I made it correct and runnable.

@lantiga
Copy link
Collaborator Author

lantiga commented Jan 6, 2025

I added the corresponding test, it's very lightweight

@codecov Codecov
Copy link

codecov bot commented Jan 6, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79%. Comparing base (efe311c) to head (fb9e836).
Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (efe311c) and HEAD (fb9e836). Click for more details.

HEAD has 144 uploads less than BASE
Flag BASE (efe311c) HEAD (fb9e836)
cpu 70 35
lightning 50 28
pytest 33 0
python3.9 18 9
python3.11 21 11
python3.10 9 4
lightning_fabric 11 0
python3.12.7 22 11
pytorch2.1 14 13
pytest-full 37 35
pytorch2.2.2 9 6
pytorch_lightning 9 7
Additional details and impacted files 
@@            Coverage Diff            @@
##           master   #20528     +/-   ##
=========================================
- Coverage      87%      79%     -8%     
=========================================
  Files         267      264      -3     
  Lines       23380    23325     -55     
=========================================
- Hits        20235    18366   -1869     
- Misses       3145     4959   +1814

Borda
Borda approved these changes Jan 6, 2025
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

Looks good :)

@lantiga lantiga merged commit 76f0c54 into master Jan 6, 2025
45 checks passed
@lantiga lantiga deleted the luca/fix-tbptt-example branch January 6, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@ethanwharris ethanwharris Awaiting requested review from ethanwharris ethanwharris is a code owner

Assignees
No one assigned
Labels
docs Documentation related pl Generic label for PyTorch Lightning package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documentation Example of Truncated BPTT is not working; self.optimizer.step() makes no sense
3 participants
@lantiga @Borda @t-vi