-
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
[WIP] Stricter argument hygiene for passing datamodules to trainer.fit or t… #7329
Conversation
Hello @ananthsub! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-05-04 06:44:45 UTC |
Codecov Report
@@ Coverage Diff @@
## master #7329 +/- ##
======================================
Coverage 87% 87%
======================================
Files 200 200
Lines 12896 12899 +3
======================================
+ Hits 11204 11231 +27
+ Misses 1692 1668 -24 |
I actually have a WIP (waiting for 1.4) branch to do exactly the opposite - adding the positional datamodule for the other functions. This is a very widely used pattern:
I'm not sure stronger typing is enough reason to remove it. This was also asked by @awaelchli here: #7258 (comment) |
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.
nice :]
@ananthsub @carmocca just thinking if we shall define this kind of collection of messages which are repetitive in multiple places on the codebase and use them as constant? |
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 ! Thanks for 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.
WIP or ready to go? 😅
Either way, LGTM!
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.
Waiting on Carlos resolution. I agree it is a common use-case and will frustrate users.
Not sure about this change. I understand the motivation but I don't think it is necessary. When the user will upgrade from 1.2 to 1.3, they will already see plenty of deprecation messages and this one here seems unnecessary. I vote for holding back on this. |
Do we go with #7431 or vote for one of them? |
IMO this is a no-go considering it's too ingrained into user's brains |
What's the verdict on this one? 😅 we have accepts and we have some arguments against. A vote with core? |
Yes, validate Carlos call there. This is too much of a common usage to be dropped. |
What does this PR do?
train_dataloader
in these APIsBefore submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃