-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Automatically check DataModule.has_{setup,teardown,prepare_data}
[2/2]
#7238
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7238 +/- ##
======================================
- Coverage 92% 92% -0%
======================================
Files 199 199
Lines 13030 13034 +4
======================================
- Hits 11967 11955 -12
- Misses 1063 1079 +16 |
DataModule.has_{setup,teardown,prepare_data}
DataModule.has_{setup,teardown,prepare_data}
DataModule.has_{setup,teardown,prepare_data}
DataModule.has_{setup,teardown,prepare_data}
DataModule.has_{setup,teardown,prepare_data}
DataModule.has_{setup,teardown,prepare_data}
DataModule.has_{setup,teardown,prepare_data}
DataModule.has_{setup,teardown,prepare_data}
[2/2]
DataModule.has_{setup,teardown,prepare_data}
[2/2]DataModule.has_{setup,teardown,prepare_data}
[2/2]
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.
Agreed with the fix for now but I'm in favor of simplifying/deprecating these properties in v1.4
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.
LGTM ! @ananthsub I think this is an important feature as preparing a dataset can be costly and shouldn't be done several times.
How do you propose to solve this problem if we remove those properties ?
…/2] (#7238) * Automatically check `DataModule.has_{setup,teardown,prepare_data}` * Use variable * Spacing * Docs * Update CHANGELOG * Remove `_DataModuleWrapper` * Add test * Update docs/source/extensions/datamodules.rst * Bad merge * add test for invalid name * Remove ValueError Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
…/2] (#7238) * Automatically check `DataModule.has_{setup,teardown,prepare_data}` * Use variable * Spacing * Docs * Update CHANGELOG * Remove `_DataModuleWrapper` * Add test * Update docs/source/extensions/datamodules.rst * Bad merge * add test for invalid name * Remove ValueError Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
What does this PR do?
This PR automates checking whether the appropriate setup/teardown/prepare_data has been called. The check was only performed by the
Trainer
. Now it is done by theDataModule
itself.This has implications for the users if they were manually calling these functions themselves, as they will now be a no-op if they had been called already.
Fixes:
Fixes the check when the stage is
None
. Before it wasn't checking each attribute (has_setup_{fit,validate,test}
) buthas_setup_None
which is not defined.This PR does NOT fix_ #7286 because to properly resolve it, we need to avoid
stage
becomingNone
in the first place.Note
The changes in this PR are the opposite of those discussed in #7301.
This PR attempts to fix the current design implementation. Not saying we should not change the design itself in the future.
Before submitting
PR review