-
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
Revert part of #10279 #10376
Revert part of #10279 #10376
Conversation
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 !
@@ -164,57 +164,6 @@ def test_setup_dataloaders_return_type(): | |||
assert lite_dataloader1.dataset is dataset1 | |||
|
|||
|
|||
def test_setup_custom_dataloaders(): |
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.
We could have a custom dataloader using only a dataset as a test.
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.
We already test that in tests/trainer/test_data_loading.py
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 meant, we should unittest the context manager to make sure the params are
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.
@tchaton isn't that covered by the test that @awaelchli added in #10334?
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 ! @carmocca I added a test to ensure the new context manager is being unit-tested.
@@ -164,57 +164,6 @@ def test_setup_dataloaders_return_type(): | |||
assert lite_dataloader1.dataset is dataset1 | |||
|
|||
|
|||
def test_setup_custom_dataloaders(): |
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 meant, we should unittest the context manager to make sure the params are
What does this PR do?
We shouldn't support this, as this is a bug in the DataLoader implementation.
Adding arbitrary
**kwargs
to a custom DataLoader allows for duplicate arguments.This is something that should be checked by the user.
The fix that this PR removes only works for the first argument and if the name is "dataset", but this would still break for any of the other duplications.
Instead, the author of the DataLoader should write something like:
Partly reverts #10279
Part of #10329
A follow-up PR will add a
MisconfigurationException
to help the user in these cases.Does your PR introduce any breaking changes? If yes, please list them.
This will no longer work for anybody already relying on this. Lite is experimental
Before submitting
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: