Make dataloader_idx optional for batch start/end hooks #16753
Conversation
carmocca
added
refactor
hooks
carmocca
requested review from
tchaton,
lantiga,
awaelchli,
hhsecond,
ethanwharris,
williamFalcon and
justusschock
as code owners
⚡ Required checks status: All passing 🟢Groups summary🟢 pytorch_lightning: Tests workflow
These checks are required after the changes to 🟢 pytorch_lightning: Azure GPU
These checks are required after the changes to 🟢 pytorch_lightning: Azure HPU
These checks are required after the changes to 🟢 pytorch_lightning: Azure IPU
These checks are required after the changes to 🟢 pytorch_lightning: Docs
These checks are required after the changes to 🟢 lightning_app: Tests workflow
These checks are required after the changes to 🟢 lightning_app: Examples
These checks are required after the changes to 🟢 lightning_app: Azure
These checks are required after the changes to 🟢 lightning_app: Docs
These checks are required after the changes to 🟢 mypy
These checks are required after the changes to 🟢 installThese checks are required after the changes to 🟢 link-check
These checks are required after the changes to Thank you for your contribution! 💜
|
|
Does this close the age old #4206 as well? |
No it doesn't. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #16753 +/- ##
=========================================
- Coverage 81% 78% -3%
=========================================
Files 446 426 -20
Lines 32270 32057 -213
=========================================
- Hits 26247 25073 -1174
- Misses 6023 6984 +961 |
|
Why not, the error there was TypeError: validation_step() takes 3 positional arguments but 4 were given which you would get when missing the arg in the method override. But you are making that optional now. |
|
The linked issue references |
What does this PR do?
When you don't use multiple dataloaders,
training_stepdoes not take adataloader_idxargument.However, this is not the case for
on_{validation,test,predict}_batch_{start,end}.This PR makes it optional for them too.
This is a breaking change, as the users who define this argument and don't use multiple dataloaders now need to set
dataloader_idx=0in their signature, or remove it if they don't need it.cc @justusschock @awaelchli @Borda @carmocca