[WIP] Trainer supports all Datasets as train_dataset, with/without __len__ #5990
Author: j-rossi-nl
Pull request: yes
Labels: name: wontfix

A first PR.

Passed:
- `make test`
- `make style`
- `make quality`

Modifies:
- trainer.py: fixes issue
- test_trainer.py: calls `Trainer.train()`

It fixes only the case where the TRAINING dataset has not the `__len__` method.

The distinction is not between `Dataset` and `IterableDataset`, but between objects that are instances of class where `__len__` is implemented or not. This is pointed in [pytorch source](https://pytorch.org/docs/stable/_modules/torch/utils/data/dataset.html#Dataset), implementation of `__len__` is up to the user.
The test is therefore: `isinstance(dataset, collections.Sized)`

NB: fixing for EVAL and TEST dataset will require more code refactor: all get funneled to `Trainer._prediction_loop()` without keeping track of whether it is EVAL or TEST, which makes the usage of `TrainingArguments.eval_steps` impossible to assume. (not to mention, there is no `test_steps` field in `TrainingArguments`)

Not all datasets have an implementation of `__len__` method, therefore the trainer should not assume it is available.

The use case is:
- Dataset with `__len__`: use `num_train_epochs` or `max_steps` to specify how long should training run
- Dataset without `__len__`: use only `max_steps`

The limitation is still valid on the EVAL / TEST dataset who still has to implement `__len__`

Comments:
> Note that there is some moving around of the code in Trainer coming in #5982 (I'll probably merge it today) so you may need to adapt a bit the code.

I'll wait for the merge of #5982 and introduce the fix for #5990 after.

> Note that there is no test_steps field since the evaluation is supposed to be complete on the test set (users can always pass along a shorter dataset if they want).

I see. 
At the moment, the functionality is that it will refuse a dataset that does not implement `__len__`, whether it is the EVAL dataset or the TEST dataset.



# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/5995?src=pr&el=h1) Report
> Merging [#5995](https://codecov.io/gh/huggingface/transformers/pull/5995?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/c69ea5efc4eac65b183e8d07b1bf91d20bbe0c8c&el=desc) will **increase** coverage by `0.40%`.
> The diff coverage is `78.57%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/5995/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/5995?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #5995      +/-   ##
==========================================
+ Coverage   78.50%   78.90%   +0.40%     
==========================================
  Files         146      146              
  Lines       26249    26264      +15     
==========================================
+ Hits        20606    20723     +117     
+ Misses       5643     5541     -102     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/5995?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/trainer.py](https://codecov.io/gh/huggingface/transformers/pull/5995/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90cmFpbmVyLnB5) | `60.63% <78.57%> (+19.75%)` | :arrow_up: |
| [src/transformers/generation\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/5995/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9nZW5lcmF0aW9uX3RmX3V0aWxzLnB5) | `86.21% <0.00%> (+0.75%)` | :arrow_up: |
| [src/transformers/tokenization\_bert.py](https://codecov.io/gh/huggingface/transformers/pull/5995/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fYmVydC5weQ==) | `92.23% <0.00%> (+0.91%)` | :arrow_up: |
| [src/transformers/optimization.py](https://codecov.io/gh/huggingface/transformers/pull/5995/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9vcHRpbWl6YXRpb24ucHk=) | `97.36% <0.00%> (+1.31%)` | :arrow_up: |
| [src/transformers/modeling\_auto.py](https://codecov.io/gh/huggingface/transformers/pull/5995/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19hdXRvLnB5) | `77.90% <0.00%> (+3.48%)` | :arrow_up: |
| [src/transformers/training\_args.py](https://codecov.io/gh/huggingface/transformers/pull/5995/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90cmFpbmluZ19hcmdzLnB5) | `86.73% <0.00%> (+6.12%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/5995?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/5995?src=pr&el=footer). Last update [c69ea5e...4066671](https://codecov.io/gh/huggingface/transformers/pull/5995?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Hi,

- Merged to include all changes from #5982
- Now accepts `IterableDataset` for any dataset (TRAIN, EVAL, TEST)
- Displays information (how many steps, etc...)
- `test_trainer.py` includes an end-to-end test of `train()` and `predict()`

It fixes the issue #5990 with my code.

Checklist:
- `make test` is positive (all passed, no failed)
- `make style` and `make quality`

Looking forward to review.

I'm still unsure about how to test what the dataset is:
- the  type hinting says `train_dataset: Dataset`
- `pytorch` indicates it is good practice to implement `__len__` on Map-Style dataset, but in the code there is no way this could be enforced
- `pytorch` relies only on the logic: user will inherit `Dataset` and implement `__len__` or user will inherit `IterableDataset` and not implement `__len__`. In `Dataloader` every time there is a doubt it is checking if the object is an instance of `Dataset` or `IterableDataset`
- in my code, I followed my first rule: either the object has `__len__` or it does not. 

Still wondering if I should code it `pytorch`-style and trust the given answer blindly, or keep it the way it is done, which is a bit more paranoid ?
Any comment appreciated.

- the test is whether a `Dataset` object has `__len__` or not
- iterable dataset is OK for training,  __only__ if `max_steps` has a strictly positive value
- iterable dataset is not acceptable for evaluation or prediction

The confusion about `eval_steps` has been purged.

The test `test_trainer_iterable_dataset` in `test_trainer.py` has been extended to check for corner cases and associated exceptions. Only the exception type is checked, not exception message.

Checklist:
- `make test` passed (no fail)
- `make style`
- `make quality`

Looking forward to review.

Nitpicking is fine in my book.

- removed redundant test on dataset (eval / test)

Checklist:
- `make test` passed (no fail)
- `make style`
- `make quality`

Up for review again.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.


Hi, is this PR still being reviewed? I would like to use `Trainer` with an `IterableDataset` and this looks like exactly what's needed to make that happen. If you have time, I would greatly appreciate this PR to get into the next version :) thank you!

I realize a part has been merged, but not everything.

And I can't seem to find a way to re-open this PR. So I guess, I should open a new one, and link to this one...

@carson-sestili The PR #7858 has been merged to master and fixes the bug.
You can already use it by installing from source.


@j-rossi-nl Thank you very much!