Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace assertion with exception #16720

Merged
merged 12 commits into from
Apr 12, 2022

Conversation

anmolsjoshi
Copy link
Contributor

What does this PR do?

Replaces assert with Exceptions as per #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?

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.

@sgugger

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 12, 2022

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

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.

Wow, that's a lot of work, thanks a lot! Could you a quick search for the pattern if self.config.pad_token_id is None or logits_shape[0] != 1: in case I missed any? The or needs to be replaced by an and.

Also, since it touches so many modeling files, I'd like a second review on this @LysandreJik if you can.

src/transformers/models/ctrl/modeling_tf_ctrl.py Outdated Show resolved Hide resolved
src/transformers/models/gpt_neo/modeling_gpt_neo.py Outdated Show resolved Hide resolved
src/transformers/models/ctrl/modeling_ctrl.py Outdated Show resolved Hide resolved
src/transformers/models/gptj/modeling_gptj.py Outdated Show resolved Hide resolved
src/transformers/models/gptj/modeling_tf_gptj.py Outdated Show resolved Hide resolved
src/transformers/models/led/modeling_led.py Outdated Show resolved Hide resolved
src/transformers/models/led/modeling_led.py Outdated Show resolved Hide resolved
src/transformers/models/led/modeling_led.py Outdated Show resolved Hide resolved
@LysandreJik LysandreJik self-requested a review April 12, 2022 12:25
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Impressive PR! Thanks for working on it, @anmolsjoshi. Apart from Sylvain's comments, didn't find anything else that needs to be changed.

anmolsjoshi and others added 7 commits April 12, 2022 07:56
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@anmolsjoshi
Copy link
Contributor Author

@sgugger @LysandreJik thanks for the review! I have incorporated all the requested changes.

@anmolsjoshi
Copy link
Contributor Author

@sgugger unsure why the PR Documentation check is failing

@sgugger
Copy link
Collaborator

sgugger commented Apr 12, 2022

Failure is unrelated to this PR and is fixed independently.
Thanks a lot for addressing all comments!

@sgugger sgugger merged commit cc034f7 into huggingface:main Apr 12, 2022
@anmolsjoshi anmolsjoshi deleted the controlFlowAssertions branch April 12, 2022 17:40
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
* Updated assertions to exceptions

* updated assertions to exceptions

* bug fixes

* fix-copies

* Update modeling_ctrl.py

* Update src/transformers/models/ctrl/modeling_tf_ctrl.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

* Update src/transformers/models/gpt_neo/modeling_gpt_neo.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

* Update src/transformers/models/gptj/modeling_gptj.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

* Update src/transformers/models/gptj/modeling_tf_gptj.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

* Update modeling_led.py

* Update modeling_led.py

* Update modeling_led.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants