Deprecate `TrainerDataLoadingMixin` and move logic to `DataConnector` #11282

daniellepintz · 2021-12-31T02:17:00Z

What does this PR do?

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 🙃

…lightning into TrainerDataLoadingMixin

pytorch_lightning/trainer/data_loading.py

pytorch_lightning/trainer/trainer.py

pl_examples/loop_examples/kfold.py

pytorch_lightning/trainer/connectors/data_connector.py

…lightning into TrainerDataLoadingMixin

Co-authored-by: Rohit Gupta <[email protected]> Co-authored-by: Aki Nitta <[email protected]>

…torch-lightning into TrainerDataLoadingMixin

for more information, see https://pre-commit.ci

…torch-lightning into TrainerDataLoadingMixin

for more information, see https://pre-commit.ci

rohitgr7 · 2022-01-04T09:53:23Z

pytorch_lightning/trainer/trainer.py

@@ -2364,6 +2369,151 @@ def terminate_on_nan(self, val: bool) -> None:
        )
        self._terminate_on_nan = val  # : 212

+    def reset_train_dataloader(self, model: Optional["pl.LightningModule"] = None) -> None:


how about moving the reload logic to data_connector and trainer.reset_train_dataloader just calls trainer._data_connector._reset_train_dataloader? not sure if it's a good idea, but just trying to avoid all the logic being kept inside the single Trainer module.

I don't feel strongly either way. I probably have a slight preference for keeping it as it is now, since I feel like having a function that just calls the private function adds a bit of unnecessary abstraction/complexity. If others feel strongly we can change it

I'd say it's fine as it is. I'd only do that if it aids in testing, but we don't have unit tests for each of these separate methods.

It is fine because the trainer owns the dataloaders, so resetting is her responsibility. But this won't be the case in the future, pretty sure. Which is why I also suggested in my previous comment to map the public property to the protected one in the connector.

In any case, let's just please not mix properties and methods like this.

n any case, let's just please not mix properties and methods like this.

@daniellepintz I believe my comment was missed here. Could you send a follow up PR? I believe reviewers did not catch this properly

@awaelchli sorry I am confused by that line. Could you clarify which property you are talking about?

I mean the disorganization that this has caused. Previously, methods and properties were organized, but now new methods have been added and now it's mixed up:

method
method
property
property
new method
new method

It should be

method
method
new method
new method
property
property

given that Trainer wants to have all properties at the bottom.

Ah I see good point. Will send a follow up PR

CHANGELOG.md

pytorch_lightning/strategies/ipu.py

pytorch_lightning/trainer/trainer.py

tests/deprecated_api/test_remove_1-8.py

Co-authored-by: Rohit Gupta <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]>

…torch-lightning into TrainerDataLoadingMixin

pytorch_lightning/trainer/connectors/data_connector.py

carmocca · 2022-01-05T00:24:45Z

pytorch_lightning/trainer/trainer.py

@@ -2364,6 +2369,151 @@ def terminate_on_nan(self, val: bool) -> None:
        )
        self._terminate_on_nan = val  # : 212

+    def reset_train_dataloader(self, model: Optional["pl.LightningModule"] = None) -> None:


I'd say it's fine as it is. I'd only do that if it aids in testing, but we don't have unit tests for each of these separate methods.

rohitgr7

nit:

pytorch_lightning/trainer/trainer.py

Co-authored-by: Rohit Gupta <[email protected]>

daniellepintz · 2022-01-05T20:13:02Z

Ready to be merged!

daniellepintz added 4 commits December 30, 2021 11:44

Deprecate TrainerDataLoadingMixin

313d8a7

Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…

33e81e2

…lightning into TrainerDataLoadingMixin

fix tests

2950166

fix ipu test

9c1aaf4

daniellepintz marked this pull request as ready for review December 31, 2021 04:40

daniellepintz requested review from awaelchli, Borda, carmocca, justusschock, kaushikb11, rohitgr7, SeanNaren, SkafteNicki, tchaton and williamFalcon as code owners December 31, 2021 04:40

kaushikb11 approved these changes Jan 3, 2022

View reviewed changes

awaelchli reviewed Jan 3, 2022

View reviewed changes

pytorch_lightning/trainer/data_loading.py Outdated Show resolved Hide resolved

akihironitta reviewed Jan 3, 2022

View reviewed changes

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

rohitgr7 reviewed Jan 3, 2022

View reviewed changes

mergify bot added the has conflicts label Jan 3, 2022

daniellepintz and others added 3 commits January 3, 2022 10:52

Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…

bcb30c1

…lightning into TrainerDataLoadingMixin

Apply suggestions from code review

36ec027

Co-authored-by: Rohit Gupta <[email protected]> Co-authored-by: Aki Nitta <[email protected]>

Merge branch 'TrainerDataLoadingMixin' of github.com:daniellepintz/py…

bffd43a

…torch-lightning into TrainerDataLoadingMixin

mergify bot removed the has conflicts label Jan 3, 2022

[pre-commit.ci] auto fixes from pre-commit.com hooks

a6e9253

for more information, see https://pre-commit.ci

daniellepintz added the data handling Generic data-related topic label Jan 3, 2022

daniellepintz and others added 4 commits January 3, 2022 15:41

move reset dataloader methods to Trainer

afb6474

[pre-commit.ci] auto fixes from pre-commit.com hooks

838fa8f

for more information, see https://pre-commit.ci

fix mypy

18bf9d2

Merge branch 'TrainerDataLoadingMixin' of github.com:daniellepintz/py…

90226cd

…torch-lightning into TrainerDataLoadingMixin

[pre-commit.ci] auto fixes from pre-commit.com hooks

518d82d

for more information, see https://pre-commit.ci

awaelchli added this to the 1.6 milestone Jan 4, 2022

awaelchli added the deprecation Includes a deprecation label Jan 4, 2022

rohitgr7 reviewed Jan 4, 2022

View reviewed changes

carmocca reviewed Jan 4, 2022

View reviewed changes

pytorch_lightning/strategies/ipu.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

tests/deprecated_api/test_remove_1-8.py Show resolved Hide resolved

daniellepintz and others added 3 commits January 4, 2022 11:30

Apply suggestions from code review

b5d816e

Co-authored-by: Rohit Gupta <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]>

fix mypy

cc904e6

Merge branch 'TrainerDataLoadingMixin' of github.com:daniellepintz/py…

0154632

…torch-lightning into TrainerDataLoadingMixin

carmocca approved these changes Jan 5, 2022

View reviewed changes

mergify bot added the ready PRs ready to be merged label Jan 5, 2022

move properties

d3e5810

rohitgr7 approved these changes Jan 5, 2022

View reviewed changes

awaelchli approved these changes Jan 5, 2022

View reviewed changes

Apply suggestions from code review

decb23a

Co-authored-by: Rohit Gupta <[email protected]>

carmocca merged commit 5b59c95 into Lightning-AI:master Jan 5, 2022

daniellepintz deleted the TrainerDataLoadingMixin branch January 5, 2022 20:25

daniellepintz mentioned this pull request Jan 6, 2022

Move newly added Trainer methods to be with other methods #11335

Merged

12 tasks

Borda mentioned this pull request Mar 9, 2023

Removed the deprecated trainer_data_loading_mixin #14888

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `TrainerDataLoadingMixin` and move logic to `DataConnector` #11282

Deprecate `TrainerDataLoadingMixin` and move logic to `DataConnector` #11282

daniellepintz commented Dec 31, 2021 •

edited

Loading

rohitgr7 Jan 4, 2022

daniellepintz Jan 4, 2022

carmocca Jan 5, 2022

awaelchli Jan 5, 2022 •

edited

Loading

awaelchli Jan 5, 2022

daniellepintz Jan 5, 2022

awaelchli Jan 6, 2022

daniellepintz Jan 6, 2022

carmocca Jan 5, 2022

rohitgr7 left a comment

daniellepintz commented Jan 5, 2022

Deprecate TrainerDataLoadingMixin and move logic to DataConnector #11282

Deprecate TrainerDataLoadingMixin and move logic to DataConnector #11282

Conversation

daniellepintz commented Dec 31, 2021 • edited Loading

What does this PR do?

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

Before submitting

PR review

Did you have fun?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaelchli Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitgr7 left a comment

Choose a reason for hiding this comment

daniellepintz commented Jan 5, 2022

Deprecate `TrainerDataLoadingMixin` and move logic to `DataConnector` #11282

Deprecate `TrainerDataLoadingMixin` and move logic to `DataConnector` #11282

daniellepintz commented Dec 31, 2021 •

edited

Loading

awaelchli Jan 5, 2022 •

edited

Loading