-
Notifications
You must be signed in to change notification settings - Fork 629
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update validation for NLP tasks #59
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.
I'm fine with enabling max_new_tokens by default instead of max_length however, I think that it should get its own validation class.
For conversation, we were missing the input validation for sure, but the parameters still do exist (but they are NOT the same as text anymore though, return_num_sequences is impossible for instance, at least in transformers.)
I know it's cumbersome to write many repetitive tests, but overall I think it works better than parametrizing, as parametrization leaves more room for silently failing tests, and reading tests when they actually are failing is sometimes hard to untangle and update (especially when the tests you want to modify need to significantly differ from what they are tied to).
ofc all this is a taste matter, so consider the remarks about testing as NITs
For |
You are correct, it's using The only reason it's not entirely the |
Updates
I'll send another PR for updating docs |
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
Summary of changes:
max_new_tokens
in docs.max_length
tomax_new_tokens
fortext-generation
, which I think would be the right param.return_full_text
andnum_return_sequences
fortext-generation
summarization
,table-question-answering
, andtext2text-generation
validation.conversational
. It was using params instead of inputs, and the inputs are others.Let me know if you disagree with any and I can revert :)