Skip to content
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

Replace assert statements with exceptions #24856

Merged
merged 22 commits into from
Jul 17, 2023
Merged

Replace assert statements with exceptions #24856

merged 22 commits into from
Jul 17, 2023

Conversation

syedsalman137
Copy link
Contributor

@syedsalman137 syedsalman137 commented Jul 17, 2023

What does this PR do?

I have replaced the assert statements with appropriate exceptions in the directory src/transformers/models/ with all models beginning with a and b letters.

Also, I have corrected error handling at places where except statements were handling AssertionError, even thought it was never to be raised. Here is an example:

try:
    if pointer.shape != array.shape:
        raise ValueError(f"Pointer shape {pointer.shape} and array shape {array.shape} mismatched")
except AssertionError as e:    # Incorrect line
    e.args += (pointer.shape, array.shape)
    raise

I changed the above to:

try:
    if pointer.shape != array.shape:
        raise ValueError(f"Pointer shape {pointer.shape} and array shape {array.shape} mismatched")
except ValueError as e:    # Corrected the line
    e.args += (pointer.shape, array.shape)
    raise

Fixes #12789

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?

@ArthurZucker @sgugger

try-except block was using AssesrtionError in except statement while the expected error is value error. Fixed the same.
try-except block was using AssesrtionError in except statement while the expected error is ValueError. Fixed the same.
Note: While raising the ValueError args are passed to it, but later added again while handling the error (See the code snippet)
try-except block was using AssesrtionError in except statement while the expected error is ValueError. Fixed the same.
Note: While raising the ValueError args are passed to it, but later added again while handling the error (See the code snippet)
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! Note that the conversion scripts are exempt form the issue of replacing asserts to exceptions, since they are not really modules of the library.

Some of your modifications break the copied from mechanism. Could you quickly run make fix-copies to fix that?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jul 17, 2023

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

@sgugger sgugger merged commit d015401 into huggingface:main Jul 17, 2023
3 of 4 checks passed
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
* Changed AssertionError to ValueError

try-except block was using AssesrtionError in except statement while the expected error is value error. Fixed the same.

* Changed AssertionError to ValueError

try-except block was using AssesrtionError in except statement while the expected error is ValueError. Fixed the same.
Note: While raising the ValueError args are passed to it, but later added again while handling the error (See the code snippet)

* Changed AssertionError to ValueError

try-except block was using AssesrtionError in except statement while the expected error is ValueError. Fixed the same.
Note: While raising the ValueError args are passed to it, but later added again while handling the error (See the code snippet)

* Changed AssertionError to ValueError

* Changed AssertionError to ValueError

* Changed AssertionError to ValueError

* Changed AssertionError to ValueError

* Changed AssertionError to ValueError

* Changed assert statement to ValueError based

* Changed assert statement to ValueError based

* Changed assert statement to ValueError based

* Changed incorrect error handling from AssertionError to ValueError

* Undoed change from AssertionError to ValueError as it is not needed

* Reverted back to using AssertionError as it is not necessary to make it into ValueError

* Fixed erraneous comparision

Changed == to !=

* Fixed erraneous comparision

Changed == to !=

* formatted the code

* Ran make fix-copies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise exceptions instead of using assertions for control flow
3 participants