[core / SFTTrainer] Fix breaking change

[younesbelkada]

What does this PR do?

fixes a breaking change on a recent PR: #1188
Fixes: #1216

In fact, if one uses the default data collator + remove_unused_columns=False, the training script fails for some datasets such as imdb as the collator will try to encode all the columns, including "text". The fix should be to force-set remove_unused_columns=True, only in the case users do not pass a data_collator for non-packing datasets and explain to the users who they should proceed in case they decided to pass remove_unused_columns=False

cc @lvwerra

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[xkszltl]

Thanks for the action.
Not sure about details yet, just a general comment:

only in the case users do not pass a data_collator for non-packing datasets

Long special condition may not be a good design and may not be accurate either.

[younesbelkada]

I agree, the thing is that :
1- in previous releases it was possible to pass remove_unused_columns=False to trainingarguments + non-packed dataset without any problem - however we were silently ignoring the unusued columns - and #1188 breaks it but while preserving the "right" behaviour
2- passing remove_unused_columns=False + default data collator will most likely fail before the default datacollator will try to tokenize text columns by default, so it'll always fail in that scenario. IMO it should be bad intent and we should leave the support of that feature only for users that they know what they're doing, by subclassing the default collator and passing it to SFTTrainer
What do you think?

[xkszltl]

2. Wait, maybe I didn't got it correctly in the first place? My intention is to tokenize the text column so that it can be trained with, is that supposed to be the case?

[younesbelkada]

tokenizing the text column should be already handled properly by the SFTTrainer, when you pass dataset_text_field="text" it will tokenize the text field and remove the text column after tokenizing it

[xkszltl]

(I could be using it in the totally wrong way because it was just used as a mitigation of the other issue I mentioned.)

[lvwerra]

Thanks for fixing @younesbelkada! Left a comment regarding the warning logic.

[lvwerra]

I am not sure this is the right approach here, potentially overwriting a user choice (args.remove_unused_columns = False) or warning when no warning is needed (if there are no extra columns).

[lvwerra]

Maybe we can check if extra columns exist and suggesting setting removed unused columns to True if that's the case?

[lvwerra]

I still think we shouldn't overwrite if the user provided remove_unused_colums=False and not always yield a warning because there might not be an issue at all.