fix run_seq2seq.py; porting trainer tests to it #10162
Conversation
| if model.config.decoder_start_token_id is None and isinstance(tokenizer, (MBartTokenizer, MBartTokenizerFast)): | ||
| assert ( | ||
| data_args.target_lang is not None and data_args.source_lang is not None | ||
| ), "mBart requires --target_lang and --source_lang" | ||
| if isinstance(tokenizer, MBartTokenizer): | ||
| model.config.decoder_start_token_id = tokenizer.lang_code_to_id[data_args.target_lang] | ||
| else: | ||
| model.config.decoder_start_token_id = tokenizer.convert_tokens_to_ids(data_args.target_lang) |
There was a problem hiding this comment.
Thanks for putting that logic back. @patil-suraj @patrickvonplaten since you better than me, shouldn't this be done by set_tgt_lang_special_tokens inside mBART?
There was a problem hiding this comment.
Can we make a priority on merging this, so that I could finish porting the other tests? And this can be dealt with separately? This PR isn't introducing anything new. Only restoring what was there in first place.
I can open an issue so that it doesn't fall between the cracks.
Thank you!
There was a problem hiding this comment.
set_tgt_lang_special_tokens is a method on MBartTokenizer so this needs to be done outside of the model.
Also, IMO MBartTokenizerFast should also have the lang_code_to_id attribute, not sure why it was treated differently.
There was a problem hiding this comment.
OK, I decided to go ahead and port the other scripts instead of waiting for merging of the first set.
Had to make some more fixes in the script while at it.
So no rush.
There was a problem hiding this comment.
Agree with @patil-suraj here -> IMO MBartTokenizerFast should have the lang_code_to_id so that we don't need an if-else here...but this is not necessarily the responsibility of this PR, so it's fine for me as it is
There was a problem hiding this comment.
Maybe for models such as MBart that don't have an unique decoder_start_token_id, we should maybe in the future just leave config.decoder_start_token_id=None and then throw an error / warning when generating
There was a problem hiding this comment.
Setting it manually here model.config.decoder_start_token_id is the correct thing to do here, but not super pretty IMO -> it would be better to incentivize the user to either insert at init or when calling generate But because of backward comp we can't really change it now for mBART anyways...
stas00
changed the title
|
OK, I decided to go ahead and port the other scripts instead of waiting for merging of the first set. Had to make some more fixes in the script while at it. |
| @@ -60,44 +63,46 @@ def require_apex(test_case): | |||
| return test_case | |||
|
|
|||
|
|
|||
| class TestFinetuneTrainer(TestCasePlus): | |||
| def finetune_trainer_quick(self, distributed=None, extra_args_str=None): | |||
| class TestTrainerExt(TestCasePlus): | |||
There was a problem hiding this comment.
I don't get what Ext means? Is it for extended? I don't really care for the class name, but the filename should have the full name so test_trainer_extended.py.
It's also actually testing the Seq2SeqTrainer in practice.
There was a problem hiding this comment.
Well, I just wanted to differentiate it from test_trainer. I think it'll go through a refinement later to probably separate seq2seq distributed tests from integration tests, as they are in one pile at the moment. This is just because they reuse the same helper methods. It will evolve and improve as we expand more tests.
I don't think the filename has to match class name - this is because we have multiple cases where the filename contains multiple class names.
Bottom line, this is a WIP and will evolve over time, but I had to change the original Finetune to reflect the new reality Totally your call, @sgugger, on how do you prefer I name it.
There was a problem hiding this comment.
I don't mind if the filename doesn't match the class, it's just that test_trainer_ext.py is not very informative and I have no idea what's tested inside just by looking at it ;-)
Since we are in tests/trainer, the trainer part can be dropped, so there is plenty of space to be more informative. With your plans to separate, it could be test_integration and test_distributed once split.
There was a problem hiding this comment.
Yes, that's the idea for when the dust settles - there are at least 3 types of tests in there at the moment.
That is it will not be called test_trainer_ext.py for long. How about I call it test_to_be_sorted.py for now so it's loud and clear it's a pile of things. Or can just rename it back to what it is now under seq2seq.
The purpose of this PR is to transition to the new tool to enable Suraj to move along. And I will work in future PRs to make things intuitive and named well. I will not forget.
This PR:
finetune_trainer.py- I'm almost sure this is far far from complete since so much was just droppedjsonlines- I move the tests dataset into the root of examples so that it can be accessed by a variety of sub-projects.run_seq2seq.pyrun_seq2seq.py@sgugger