-
Notifications
You must be signed in to change notification settings - Fork 26.2k
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
replaced assert with raise ValueError for t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese. #23273
Conversation
The documentation is not available anymore as the PR was closed or merged. |
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.
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!
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" | ||
) |
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 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
?
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") |
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.
Ok, trying now.
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): |
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.
Nice!
Hi @sanchit-gandhi just pushed the change that you requested. |
Awesome thanks - requesting a final review! |
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.
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
) | ||
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." |
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.
Otherwise line is longer than repo limit 165 > 120
"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." |
) | ||
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." |
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 re line length
"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." |
@@ -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") |
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.
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") |
) | ||
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." |
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.
"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." |
@@ -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") |
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.
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") |
Hi @amyeroberts, just pushed the changes you requested! |
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.
Thanks for iterating!
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
…x2struct, mt5, longt5, gptsan_japanese. (huggingface#23273) * replaced assert with raise ValueError * one liner * reverse one liner and cache-decoder check
…x2struct, mt5, longt5, gptsan_japanese. (huggingface#23273) * replaced assert with raise ValueError * one liner * reverse one liner and cache-decoder check
What does this PR do?
As said by @sanchit-gandhi, here this PR replaces
assert
withraise ValueError
for models - t5, switch_transformers, pix2struct, mt5, longt5, gptsan_japanese.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
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