-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🤏 New models for tests #2287
base: main
Are you sure you want to change the base?
🤏 New models for tests #2287
Conversation
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. |
@@ -136,9 +135,7 @@ def test_setup_chat_format(self): | |||
self.assertEqual(modified_tokenizer.eos_token, _chatml.eos_token) | |||
self.assertEqual(modified_tokenizer.pad_token, _chatml.pad_token) | |||
self.assertEqual(modified_tokenizer.bos_token, _chatml.bos_token) | |||
self.assertEqual(len(modified_tokenizer), (original_tokenizer_len + 2)) |
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.
no special token need to be added
self.assertEqual((self.model.get_input_embeddings().weight.shape[0] % 64), 0) | ||
self.assertEqual(self.model.get_input_embeddings().weight.shape[0], (original_tokenizer_len + 64)) |
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.
Same here, no modification needed
] | ||
) | ||
@require_bitsandbytes | ||
@require_peft | ||
@unittest.skip("You need a GPU with bf16 support in order to run these tests") | ||
def test_dpo_lora_bf16_autocast(self, name, loss_type, pre_compute, gen_during_eval): |
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.
not used
max_length=512, | ||
max_prompt_length=512, |
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.
those param are optional now, so I remove them
self.assertEqual(len(query[0]), 9) | ||
self.assertEqual(len(query[0]), 8) |
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.
The tokenizer has changed, so this value has also changed.
self.assertListEqual(tokenized_dataset["prompt_input_ids"][0], [5377, 11141]) | ||
self.assertListEqual(tokenized_dataset["prompt_attention_mask"][0], [1, 1]) | ||
self.assertListEqual(tokenized_dataset["answer_input_ids"][0], [318, 1365, 621, 8253, 13]) | ||
self.assertListEqual(tokenized_dataset["prompt_input_ids"][0], [31137]) | ||
self.assertListEqual(tokenized_dataset["prompt_attention_mask"][0], [1]) | ||
self.assertListEqual(tokenized_dataset["answer_input_ids"][0], [374, 2664, 1091, 16965, 13]) |
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.
The tokenizer has changed, so these value have also changed.
self.assertListEqual(processed_dataset["prompt_input_ids"][0], [50256, 5377, 11141]) | ||
self.assertListEqual(processed_dataset["prompt_attention_mask"][0], [1, 1, 1]) | ||
self.assertListEqual(processed_dataset["prompt_input_ids"][0], [31137]) | ||
self.assertListEqual(processed_dataset["prompt_attention_mask"][0], [1]) | ||
self.assertListEqual( | ||
processed_dataset["completion_input_ids"][0], [50256, 5377, 11141, 318, 1365, 621, 8253, 13, 50256] | ||
) | ||
self.assertListEqual(processed_dataset["completion_attention_mask"][0], [1, 1, 1, 1, 1, 1, 1, 1, 1]) | ||
self.assertListEqual( | ||
processed_dataset["completion_labels"][0], [-100, -100, -100, 318, 1365, 621, 8253, 13, 50256] | ||
processed_dataset["completion_input_ids"][0], [31137, 374, 2664, 1091, 16965, 13, 151645] | ||
) | ||
self.assertListEqual(processed_dataset["completion_attention_mask"][0], [1, 1, 1, 1, 1, 1, 1]) | ||
self.assertListEqual(processed_dataset["completion_labels"][0], [-100, 374, 2664, 1091, 16965, 13, 151645]) |
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.
The tokenizer has changed, so this value has also changed.
"trl-internal-testing/tiny-random-BartForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-BigBirdPegasusForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-BlenderbotForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-BlenderbotSmallForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-FSMTForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-LEDForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-LongT5ForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-M2M100ForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-MarianMTModel", | ||
"trl-internal-testing/tiny-random-MBartForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-MT5ForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-MvpForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-PegasusForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-PegasusXForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-PLBartForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-ProphetNetForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-SwitchTransformersForConditionalGeneration", | ||
"trl-internal-testing/tiny-random-T5ForConditionalGeneration", | ||
"qgallouedec/tiny-T5ForConditionalGeneration", | ||
"qgallouedec/tiny-BartModel", |
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.
Only use the two most popular enc-dec models
self.model = AutoModelForCausalLM.from_pretrained(self.model_id) | ||
self.ref_model = AutoModelForCausalLM.from_pretrained(self.model_id) | ||
self.reward_model = AutoModelForSequenceClassification.from_pretrained("EleutherAI/pythia-14m", num_labels=1) | ||
self.reward_tokenizer = AutoTokenizer.from_pretrained("EleutherAI/pythia-14m") | ||
self.reward_tokenizer.chat_template = SIMPLE_CHAT_TEMPLATE |
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.
The model already have a chat template
self.t5_model = AutoModelForSeq2SeqLM.from_pretrained(model_id) | ||
self.t5_tokenizer = AutoTokenizer.from_pretrained(model_id) | ||
self.t5_tokenizer.chat_template = SIMPLE_CHAT_TEMPLATE |
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.
T5 doesn't have a chat template
self.tokenizer = AutoTokenizer.from_pretrained(self.model_id) | ||
self.tokenizer.chat_template = "{% for message in messages %}{{'<|im_start|>' + message['role'] + '\n' + message['content'] + '<|im_end|>' + '\n'}}{% endfor %}{% if add_generation_prompt %}{{ '<|im_start|>assistant\n' }}{% endif %}" |
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.
The model already have a chat template
self.model = AutoModelForSequenceClassification.from_pretrained(self.model_id) | ||
self.model.config.pad_token_id = self.tokenizer.pad_token_id |
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.
needed for batched forward in the model
@@ -205,6 +205,7 @@ def setUp(self): | |||
ignore_index=self.ignore_index, | |||
) | |||
|
|||
@unittest.skip("This test must be updated.") |
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.
This test seems very related to the way the codellama/CodeLlama-7b-Instruct-hf tokenizer works (especially on BOS and EOS) and is not general. It should be rewritten in the future.
["trl-internal-testing/tiny-random-paligemma"], | ||
["trl-internal-testing/tiny-random-llava-1.5"], | ||
("qgallouedec/tiny-Idefics2ForConditionalGeneration",), | ||
# ("qgallouedec/tiny-PaliGemmaForConditionalGeneration",), |
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.
paligemma doesn't have a chat template (not meant for chat). We've to find a way to test it. Dedicated method? We can do it in the future
What does this PR do?
Fixes #2177
The models currently used for testing lack uniformity and practicality. Specifically:
trl-internal-testing
,facebook
,philschmid
,hf-internal-testing
, etc.).tiny-random-MistralForCausalLM
,dummy-GPT2-correct-vocab
,gpt2
,pythia-14m
).This PR introduces the following improvements:
trl-internal-testing
).When approved, I'll move every models into
trl-internal-testing
namespace (instead ofqgallouedec
). And probably remove all the old testing models intrl-internal-testing
.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.