-
Notifications
You must be signed in to change notification settings - Fork 31.9k
Trainer with Iterable Dataset #7858
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
Trainer with Iterable Dataset #7858
Conversation
* fix only for torch * TF trainer untouched * trainer tests are skipped when no torch
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sgugger
left a 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.
Thanks for your PR. There are a few things I'd like to adjust, on the error messages especially. Or the comments in the tests.
In general, please limit your PR to the functionality you are adding. We can discuss unrelated changes to other parts of the code in separate PRs but it makes reviewing less efficient if you try to group them together.
sgugger
left a 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.
Thanks for doing most of the items in the review. I still disagree with your changes in the test file, which makes reusing the RegressionDataset for other frameworks impossible (and is unrelated to the feature you are trying to implement).
* unnecessary inheritance * RegressionDataset implements all needed methods __len__ and __getitem__
* was wrongly under is_torch_available()
LysandreJik
left a 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.
LGTM
|
Thanks for your contribution! |
What does this PR do?
Fixes #5990
Follows #5995 (closed because stale).
Merges all commits from master.
Pull Request section?
to the it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@sgugger