to_tf_dataset rewrite
Opened by Rocketknight1 (closed)

This PR rewrites almost all of `to_tf_dataset()`, which makes it kind of hard to list all the changes, but the most critical ones are:

- Much better stability and no more dropping unexpected column names (Sorry @NielsRogge)
- Doesn't clobber custom transforms on the data (Sorry @NielsRogge again)
- Much better handling of the situation when the `collate_fn` adds columns that aren't in the dataset.
- Better inference of shapes and data types
- Lots of hacky special-casing code removed
- Can return string columns (as `tf.String`)
- Most arguments have default values, calling the method should be much simpler
- ~~Can accept a `model` argument and only return columns that are valid inputs to that model~~
- Drops the `dummy_labels` argument - this was a workaround for Keras issues that have been resolved by changes in `transformers`. Also remove it from tests and the Overview notebook.

I still have a couple of TODOs remaining and some testing to do, so don't merge yet, but it should be mostly ready for review at this point!

Comments:
_The documentation is not available anymore as the PR was closed or merged._

[Magic is now banned](https://www.youtube.com/watch?v=WIn58XoY728#t=36s) by decree of @sgugger. This is honestly much cleaner, and the functionality will make much more sense in `transformers` anyway!

@gante I renamed the default collator to `minimal_tf_collate_fn`!

@lhoestq @sgugger @gante 

I think this should now be ready, it looks good in testing! I'll try a few more notebooks today and tomorrow to be sure before I merge. Key changes are:

- No column autodetection magic (will make a separate PR to add this as a `transformers` function)
- Drops non-numerical features automatically (this is more of a 'DataLoader' method, we'll have a separate method to expose 'raw' datasets to `tf.data`)
- Better autodetection of numerical features.
- Shouldn't randomly crash mid-function :skull: 

We definitely have some questions still to resolve about how to handle making a 'DataLoader' dataset versus a 'raw' dataset - see [the Notion doc](https://www.notion.so/huggingface2/Splitting-to_tf_dataset-c2e0773c4bec484384064b30ed634383) if you're interested. Still, since this PR is just fixes/improvements to an existing method which never supported non-numerical features anyway, we can merge it before we've resolved those issues, and then think about how to name and split things afterwards.

P.S. I'll take out the region comments at the end before I merge, I promise! They're just helpful while I'm editing it

+1 for the tests

> Drops non-numerical features automatically

Can you give more details on how this work and the rationale as well ? This is not explained in the docs

Also why are you adding `error_on_missing` and `auto_fix_label_names ` ? The rationale is not clear to me. In particular I think it is sensible enough to expect users to not ask columns that don't exist, and to rename a label column when required.

@lhoestq I rewrote those parts - they were causing some other issues too! `error_on_missing` and `auto_fix_label_names` have been removed. The new logic is to simply drop (before batch collation) all columns the user doesn't ask for, but not to raise errors if the user asked for columns not in the dataset, as they may be added by the collator. Hopefully this cleans it up and matches the documentation better!

@lhoestq New tests are now in!

Seeing some other random tests failing that don't look to be associated with this PR.

@lhoestq I can't figure out these test failures! They don't seem related to this PR at all, but I rebased to the latest version and they keep happening, even though they're not visible on master.

Thanks for the ping, will take a look tomorrow :)

Maybe the rebase didn't go well for the code recently merged about label alignment from https://github.com/huggingface/datasets/pull/4277 ?

It's very strange! The rebase looks fine to me. I might try to move my changes to a new branch from `master` and see if I can figure out which change causes this problem to appear.

@lhoestq Got it! It was caused by a name collision - I was importing `typing.Sequence`, but the code also needed `features.Sequence`. The tests from that PR were expecting the latter but got the former, and then crashed.

@lhoestq Thanks! Also, when you're ready, don't merge it immediately! I'd like to do a quick round of manual testing with the very final build once you're happy to make sure it still works in our notebooks and examples.

@lhoestq Tests look good to me, merging now!