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.

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

Avoid patching common DataHooks to the LightningModule #10603

Merged
merged 20 commits into from
Feb 25, 2022

Conversation

rohitgr7
Copy link
Contributor

@rohitgr7 rohitgr7 commented Nov 18, 2021

What does this PR do?

Fixes #10498

If any of these hooks are overridden in both model and datamodule, then we raise a warning and prioritize datamodule.

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

cc @Borda @justusschock @awaelchli @rohitgr7 @carmocca @ananthsub @ninginthecloud @jjenniferdai @akihironitta

@rohitgr7 rohitgr7 added refactor lightningdatamodule pl.LightningDataModule labels Nov 18, 2021
@rohitgr7 rohitgr7 added this to the 1.6 milestone Nov 18, 2021
@rohitgr7 rohitgr7 self-assigned this Nov 18, 2021
@rohitgr7 rohitgr7 force-pushed the ref/unpatch_datahooks branch from 0a67469 to 3add38b Compare November 18, 2021 09:20
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

@rohitgr7 rohitgr7 requested a review from ananthsub November 19, 2021 13:51
@rohitgr7 rohitgr7 marked this pull request as draft November 22, 2021 19:30
@rohitgr7 rohitgr7 force-pushed the ref/unpatch_datahooks branch from dae8d5f to 3adc0c9 Compare November 23, 2021 15:57
@rohitgr7 rohitgr7 marked this pull request as ready for review November 23, 2021 17:13
@mergify mergify bot removed the has conflicts label Nov 23, 2021
Copy link
Contributor

@ninginthecloud ninginthecloud left a comment

Choose a reason for hiding this comment

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

Hi, @rohitgr7 I'm ok with this update, thanks a lot for all the change! Would you add the warnings into the codebase as you proposed in the comment? Additionally, we currently have both _DataHookSource and _DataLoaderSource, could we simplify as just one? Afterall, *_dataloader's are also data hooks, the purpose of source is providing a source of truth handling different overridden. Let me know what you think.

@rohitgr7
Copy link
Contributor Author

rohitgr7 commented Jan 19, 2022

Hi, @rohitgr7 I'm ok with this update, thanks a lot for all the change! Would you add the warnings into the codebase as you proposed in the comment? Additionally, we currently have both _DataHookSource and _DataLoaderSource, could we simplify as just one? Afterall, *_dataloader's are also data hooks, the purpose of source is providing a source of truth handling different overridden. Let me know what you think.

yes @ninginthecloud, I was going to propose this in a follow-up issue. Also, we need to ensure to have strict rules there:

  1. strictly use LDM for dataloader hooks just like we do now. No warnings in such a case.
  2. Strictly follow LDM, if passed, for both common hooks and dataloader hooks. For other hooks, IMO we could start raising the warning for the current behavior i.e if LDM is passed, use hooks for LM if there are not overridden in LDM but overridden in LightningModule. Making LDM as a source of truth always.

else I think it would be really ambiguous to raise a warning for one hook if it's overridden in LM but not in LDM and not raise for another if it's overridden in LDM.

@mergify mergify bot removed the has conflicts label Jan 19, 2022
Copy link
Contributor

@ninginthecloud ninginthecloud left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@mergify mergify bot removed the has conflicts label Feb 7, 2022
pytorch_lightning/trainer/connectors/data_connector.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/connectors/data_connector.py Outdated Show resolved Hide resolved
pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
tests/trainer/connectors/test_data_connector.py Outdated Show resolved Hide resolved
@rohitgr7 rohitgr7 requested a review from carmocca February 8, 2022 11:32
@mergify mergify bot added the has conflicts label Feb 9, 2022
@mergify mergify bot removed the has conflicts label Feb 21, 2022
pytorch_lightning/trainer/connectors/data_connector.py Outdated Show resolved Hide resolved
tests/trainer/connectors/test_data_connector.py Outdated Show resolved Hide resolved
tests/trainer/connectors/test_data_connector.py Outdated Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label Feb 22, 2022
@rohitgr7 rohitgr7 enabled auto-merge (squash) February 22, 2022 18:00
@rohitgr7 rohitgr7 requested a review from awaelchli February 24, 2022 09:44
@rohitgr7 rohitgr7 merged commit 5d2d9b0 into master Feb 25, 2022
@rohitgr7 rohitgr7 deleted the ref/unpatch_datahooks branch February 25, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lightningdatamodule pl.LightningDataModule ready PRs ready to be merged refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid patching DataHooks
6 participants