-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Deprecate TrainerDataLoadingMixin
and move logic to DataConnector
#11282
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
Merged
carmocca
merged 18 commits into
Lightning-AI:master
from
daniellepintz:TrainerDataLoadingMixin
Jan 5, 2022
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
313d8a7
Deprecate TrainerDataLoadingMixin
daniellepintz 33e81e2
Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…
daniellepintz 2950166
fix tests
daniellepintz 9c1aaf4
fix ipu test
daniellepintz bcb30c1
Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…
daniellepintz 36ec027
Apply suggestions from code review
daniellepintz bffd43a
Merge branch 'TrainerDataLoadingMixin' of github.com:daniellepintz/py…
daniellepintz a6e9253
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] afb6474
move reset dataloader methods to Trainer
daniellepintz 838fa8f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 18bf9d2
fix mypy
daniellepintz 90226cd
Merge branch 'TrainerDataLoadingMixin' of github.com:daniellepintz/py…
daniellepintz 518d82d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] b5d816e
Apply suggestions from code review
daniellepintz cc904e6
fix mypy
daniellepintz 0154632
Merge branch 'TrainerDataLoadingMixin' of github.com:daniellepintz/py…
daniellepintz d3e5810
move properties
daniellepintz decb23a
Apply suggestions from code review
daniellepintz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about moving the reload logic to data_connector and
trainer.reset_train_dataloader
just callstrainer._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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniellepintz I believe my comment was missed here. Could you send a follow up PR? I believe reviewers did not catch this properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awaelchli sorry I am confused by that line. Could you clarify which property you are talking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see good point. Will send a follow up PR