Sitelet https://web.archive.org/web/20210811152909/https://github.com/huggingface/transformers/issues/12789
Skip to content

/ transformers

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

Jump to bottom

Raise exceptions instead of using assertions for control flow #12789

Open
willfrey opened this issue Jul 19, 2021 · 4 comments
Open

Raise exceptions instead of using assertions for control flow #12789

willfrey opened this issue Jul 19, 2021 · 4 comments
Labels
Good First Issue

Comments

@willfrey
Copy link
Contributor

@willfrey willfrey commented Jul 19, 2021

transformers/src/transformers/models/gpt2/modeling_gpt2.py

Line 698 in 546dc24

assert batch_size > 0, "batch_size has to be defined and > 0"

Assertions can't be relied upon for control flow because they can be disabled, as per the following:

$ 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?
@sgugger
Copy link
Member

@sgugger sgugger commented Jul 21, 2021

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).
@sgugger
Copy link
Member

@sgugger sgugger commented Jul 27, 2021

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!

(Don't try to do all files of the library at one ;-) )
@Josh1108
Copy link

@Josh1108 Josh1108 commented Aug 6, 2021

Hi @sgugger,

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

@sgugger sgugger commented Aug 9, 2021

Any of the files with assert statements is fair game :-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Good First Issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@willfrey @sgugger @Josh1108