replaced assert with raise ValueError for t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese. #23273

susnato · 2023-05-10T17:35:35Z

What does this PR do?

As said by @sanchit-gandhi, here this PR replaces assert with raise ValueError for models - t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese.

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

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.

@sanchit-gandhi

HuggingFaceDocBuilderDev · 2023-05-10T17:54:50Z

The documentation is not available anymore as the PR was closed or merged.

sanchit-gandhi

Awesome - thanks for the quick fix @susnato! All the changes LGTM - would suggest maybe just putting one of the error messages onto one line then running:

make style

To automatically format as required, otherwise all good from my side!

sanchit-gandhi · 2023-05-11T16:31:48Z

src/transformers/models/longt5/modeling_longt5.py

+            raise ValueError(
+                "self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id."
+                " See LongT5 docs for more information"
+            )


This has gone a bit weird with the formatting - shall we put everything onto one line and then let ruff fix it for us by calling make style?

Suggested change

raise ValueError(

"self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id."

" See LongT5 docs for more information"

)

raise ValueError("self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id. See LongT5 docs for more information")

Ok, trying now.

sanchit-gandhi · 2023-05-11T16:33:19Z

src/transformers/models/mt5/modeling_mt5.py

+                raise ValueError(
+                    f"reordered_layer_past_states[0] shape {reordered_layer_past_states[0].shape} and layer_past_states[0] shape {layer_past_states[0].shape} mismatched"
+                )
+            if len(reordered_layer_past_states) != len(layer_past_states):


susnato · 2023-05-11T16:57:03Z

Hi @sanchit-gandhi just pushed the change that you requested.

sanchit-gandhi · 2023-05-11T17:37:55Z

Awesome thanks - requesting a final review!

amyeroberts

Thanks for updating these!

There's just a few places where the error messages should be updated to be in line with line-width limits, and two which I think are wrong because of a copy-paste

amyeroberts · 2023-05-11T17:51:11Z

src/transformers/models/longt5/modeling_longt5.py

-        )
+        if decoder_start_token_id is None:
+            raise ValueError(
+                "self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id. See LongT5 docs for more information."


Otherwise line is longer than repo limit 165 > 120

Suggested change

"self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id. See LongT5 docs for more information."

"self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id."

" See LongT5 docs for more information."

amyeroberts · 2023-05-11T17:54:08Z

src/transformers/models/mt5/modeling_mt5.py

-        )
+        if decoder_start_token_id is None:
+            raise ValueError(
+                "self.model.config.decoder_start_token_id has to be defined. In MT5 it is usually set to the pad_token_id. See MT5 docs for more information."


Same here re line length

Suggested change

"self.model.config.decoder_start_token_id has to be defined. In MT5 it is usually set to the pad_token_id. See MT5 docs for more information."

"self.model.config.decoder_start_token_id has to be defined. In MT5 it is usually set to the pad_token_id."

" See MT5 docs for more information."

amyeroberts · 2023-05-11T17:55:07Z

src/transformers/models/mt5/modeling_mt5.py

@@ -961,7 +964,8 @@ def forward(
        mask_seq_length = past_key_values[0][0].shape[2] + seq_length if past_key_values is not None else seq_length

        if use_cache is True:
-            assert self.is_decoder, f"`use_cache` can only be set to `True` if {self} is used as a decoder"
+            if not self.is_decoder:
+                raise ValueError("You have to initialize the model with valid token embeddings")


Suggested change

raise ValueError("You have to initialize the model with valid token embeddings")

raise ValueError(f"`use_cache` can only be set to `True` if {self} is used as a decoder")

amyeroberts · 2023-05-11T17:57:00Z

src/transformers/models/t5/modeling_t5.py

-        )
+        if decoder_start_token_id is None:
+            raise ValueError(
+                "self.model.config.decoder_start_token_id has to be defined. In T5 it is usually set to the pad_token_id. See T5 docs for more information."


Suggested change

"self.model.config.decoder_start_token_id has to be defined. In T5 it is usually set to the pad_token_id. See T5 docs for more information."

"self.model.config.decoder_start_token_id has to be defined. In T5 it is usually set to the pad_token_id."

" See T5 docs for more information."

amyeroberts · 2023-05-11T17:57:31Z

src/transformers/models/t5/modeling_t5.py

@@ -990,7 +992,8 @@ def forward(
        mask_seq_length = past_key_values[0][0].shape[2] + seq_length if past_key_values is not None else seq_length

        if use_cache is True:
-            assert self.is_decoder, f"`use_cache` can only be set to `True` if {self} is used as a decoder"
+            if not self.is_decoder:
+                raise ValueError("You have to initialize the model with valid token embeddings")


Suggested change

raise ValueError("You have to initialize the model with valid token embeddings")

raise ValueError(f"`use_cache` can only be set to `True` if {self} is used as a decoder")

susnato · 2023-05-11T18:48:03Z

Hi @amyeroberts, just pushed the changes you requested!
Let me know if any more changes are needed or not.

amyeroberts

Thanks for iterating!

amyeroberts · 2023-05-12T18:29:43Z

Merging as errors are unrelated to this PR and have been resolved on main

…x2struct, mt5, longt5, gptsan_japanese. (huggingface#23273) * replaced assert with raise ValueError * one liner * reverse one liner and cache-decoder check

replaced assert with raise ValueError

4bbe879

sanchit-gandhi approved these changes May 11, 2023

View reviewed changes

one liner

d7906ba

sanchit-gandhi requested a review from amyeroberts May 11, 2023 17:37

amyeroberts reviewed May 11, 2023

View reviewed changes

reverse one liner and cache-decoder check

f641561

amyeroberts approved these changes May 12, 2023

View reviewed changes

amyeroberts merged commit 79743ce into huggingface:main May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replaced assert with raise ValueError for t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese. #23273

replaced assert with raise ValueError for t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese. #23273

	"self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id. See LongT5 docs for more information."
	"self.model.config.decoder_start_token_id has to be defined. In LongT5 it is usually set to the pad_token_id."
	" See LongT5 docs for more information."

	"self.model.config.decoder_start_token_id has to be defined. In MT5 it is usually set to the pad_token_id. See MT5 docs for more information."
	"self.model.config.decoder_start_token_id has to be defined. In MT5 it is usually set to the pad_token_id."
	" See MT5 docs for more information."

	raise ValueError("You have to initialize the model with valid token embeddings")
	raise ValueError(f"`use_cache` can only be set to `True` if {self} is used as a decoder")

	"self.model.config.decoder_start_token_id has to be defined. In T5 it is usually set to the pad_token_id. See T5 docs for more information."
	"self.model.config.decoder_start_token_id has to be defined. In T5 it is usually set to the pad_token_id."
	" See T5 docs for more information."

replaced assert with raise ValueError for t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese. #23273

replaced assert with raise ValueError for t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese. #23273

Conversation

What does this PR do?

Before submitting

Who can review?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment