Raise exceptions instead of using assertions for control flow
Author: willfrey
Pull request: no
Labels: name: Good First Issue

https://github.com/huggingface/transformers/blob/546dc24e0883e5e9f5eb06ec8060e3e6ccc5f6d7/src/transformers/models/gpt2/modeling_gpt2.py#L698

Assertions can't be relied upon for control flow because they can be disabled, as per the following:

```shell
$ python --help
usage: python [option] ... [-c cmd | -m mod | file | -] [arg] ...
...
-O     : remove assert and __debug__-dependent statements; add .opt-1 before
         .pyc extension; also PYTHONOPTIMIZE=x
...
```

From my understanding, this is why mypy has no qualms about using them to narrow types because you can turn them off at runtime and so they incur zero cost.

Would you be open to me changing these assertions to other appropriate exceptions as I encounter them?

Comments:
That is an excellent point, and we welcome any PR that changes assert's to proper exception. We have been doing that for long error messages anyway, and I agree with you it's a better choice (as long as we raise the appropriate exception of course).



I'm adding the good first issue label, this way if someone wants to take care of one file to remove all asserts and replace them with proper exceptions, they can make a PR with it! The exceptions are examples files or conversion scripts (the convert_xxx.py in some model folder) in which we are fine with asserts.

(Don't try to do all files of the library at one ;-) )

Hi @sgugger,

I'd like to take up one of the files to start. Should I pick up modelling_gpt2.py and go ahead?

Any of the files with assert statements is fair game :-)

Hi @sgugger,
I'd be happy to do this. So I need to change `assert` to 'Exception' type error for only one file?
(Not allowed to do this for every file?)

Just adding my two cents but I wouldn’t raise bare `Exception`s but instead raise the appropriate type of exception given the error. 

> On Aug 19, 2021, at 1:44 AM, Ambesh Shekhar ***@***.***> wrote:
> 
> ﻿
> Hi @sgugger,
> I'd be happy to do this. So I need to change assert to 'Exception' type error for only one file?
> (Not allowed to do this for every file?)
> 
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or unsubscribe.


I have created PR #13184 for this issue. Kindly go through it.
Thanks

@sgugger   is this still open ? If so any files with assertion came under the issue right ? Can i join this issue if it's not completed

@jeffin07 You can pick any file that has not already been treated.

@sgugger I am also going to pick several files to kill my time. 

I've created PR #13894 . Please take a look.

I've created PR [#13909](https://github.com/huggingface/transformers/pull/13909) towards this issue. Please take a look

Hello, I'm new to open source. I wanted to know if this issue is still open and if so how do i check which files have not been edited already and which files require changes still?

Hello @kushagrabhushan! You can do a search across the repository and look for the `assert` keyword. Here's an example:
https://github.com/huggingface/transformers/blob/3e218523e87002c572f6424d6d24ac656bcc40be/src/transformers/models/bert/modeling_bert.py#L453

I recommend looking only for files in `src/transformers` and in `tests`. In `src/transformers`, it would be nice to replace them by `ValueError`s, and in `tests`, it would be ideal to replace them by unittest assertions. These can be `self.assertEqual`, `self.assertDictEqual`, etc, according to what the assertion is supposed to do.

I would recommend cloning the repository and using a tool to search for these, as the GitHub search isn't ideal for a search like this.

I would also like to help on this issue. If i understood it right, i should make a separate PR for every single file right? 

Hi, I would like to participate in solving this issue, I will take a look at the files which do not have yet a PR and according to the advices given by the comment of @LysandreJik.

In cases of assertions being made inside for loops, what would be the best practice?
```py
for sentence in sentences:
    if len(sentence["tokens"]) != len(sentence["labels"]):
        raise ValueError(f"Number of tokens {len(words)} and labels tokens {len(labels)} mismatch")
```
or
```py
for sentence in sentences:
    if len(sentence["tokens"]) != len(sentence["labels"]):
        raise ValueError(f"Number of tokens {len(words)} and labels tokens {len(labels)} mismatch in sentence {sentence}")
```
The second approach seems to be a friendly solution for the user since it will show exactly what was the problematic sentence, although, I have seen the first approach used in the PRs. This isn't a critique, I am trying to understand if the first option is better or not.

The second approach is more informative indeed!

I am going to tackle on the file [modeling_distilbert.py](\src\transformers\models\distilbert\modeling_distilbert.py) with @Batese2001

I'd like to work on this file [run_image_captioning_flax.py](https://github.com/huggingface/transformers/blob/main/examples/flax/image-captioning/run_image_captioning_flax.py) 

First time contributing here! I've created PR https://github.com/huggingface/transformers/pull/20478 for this issue. Would appreciate it if anyone could take a look

I will look and give you a comment after it

Best,
John

Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Billy Lee ***@***.***>
Sent: Monday, November 28, 2022 4:10:54 PM
To: huggingface/transformers ***@***.***>
Cc: Chu, John ***@***.***>; Comment ***@***.***>
Subject: Re: [huggingface/transformers] Raise exceptions instead of using assertions for control flow (#12789)

Caution:This email originated from outside of the College. Do not click links or open attachments unless you recognize the sender and know the content is safe. Report anything suspicious to the HelpDesk.


First time contributing here! I've created PR #20478<https://github.com/huggingface/transformers/pull/20478> for this issue. Would appreciate it if anyone could take a look

—
Reply to this email directly, view it on GitHub<https://github.com/huggingface/transformers/issues/12789#issuecomment-1329767859>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIQLY75OGAN2OSYNWM2WWBTWKUNV5ANCNFSM5AT2I5UA>.
You are receiving this because you commented.Message ID: ***@***.***>


First time contributing for me as well! Please have a look at the following PR when you have time! #24757