Skip to content

#12789 Replace assert statements with exceptions#13909

Merged
sgugger merged 3 commits intohuggingface:masterfrom
djroxx2000:fix-utils_qa-assertions
Oct 7, 2021
Merged

#12789 Replace assert statements with exceptions#13909
sgugger merged 3 commits intohuggingface:masterfrom
djroxx2000:fix-utils_qa-assertions

Conversation

@djroxx2000
Copy link
Copy Markdown
Contributor

@djroxx2000 djroxx2000 commented Oct 6, 2021

What does this PR do?

Replaces the assertions in utils_qa.py file at examples/tensorflow/question-answering, examples/flax/question-answering and examples/pytorch/question-answering (bound by strict copy check) with ValueError and EnvironmentError exceptions as necessary.
Also fixed a typo changing predicitions -> predictions in postprocess_qa_predictions_with_beam_search function.

Contributes towards fixing issue #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?

@djroxx2000
Copy link
Copy Markdown
Contributor Author

djroxx2000 commented Oct 6, 2021

Can someone please take a look at my PR. The error log mentions a config.json file to be missing in a tmp directory and then goes on to throw 2 assertion errors. But the files where I have made my changes are not mentioned in the logs anywhere. I'm having trouble understanding why my changes may have caused a missing config file. Sorry about the force-push, it was required for a little cleanup.

…tion-answering and examples/tensorflow/question-answering
Copy link
Copy Markdown
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 a lot for fixing those! I've left suggestions in the flax utils (but the others are the same). Could you put them in the three files?

``logging`` log level (e.g., ``logging.WARNING``)
"""
assert len(predictions) == 2, "`predictions` should be a tuple with two elements (start_logits, end_logits)."
if not len(predictions) == 2:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not len(predictions) == 2:
if len(predictions) != 2:

Will be easier to understand like this :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure I'll make the changes and push

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have pushed the changes. Thanks a lot for the experience!

all_start_logits, all_end_logits = predictions

assert len(predictions[0]) == len(features), f"Got {len(predictions[0])} predictions and {len(features)} features."
if not len(predictions[0]) == len(features):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not len(predictions[0]) == len(features):
if len(predictions[0]) != len(features):

``logging`` log level (e.g., ``logging.WARNING``)
"""
assert len(predictions) == 5, "`predictions` should be a tuple with five elements."
if not len(predictions) == 5:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not len(predictions) == 5:
if len(predictions) != 5:

assert len(predictions[0]) == len(
features
), f"Got {len(predictions[0])} predicitions and {len(features)} features."
if not len(predictions[0]) == len(features):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not len(predictions[0]) == len(features):
if len(predictions[0]) != len(features):

@sgugger sgugger merged commit 319beb6 into huggingface:master Oct 7, 2021
@sgugger
Copy link
Copy Markdown
Collaborator

sgugger commented Oct 7, 2021

Thanks again!

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.

2 participants