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

Add doctests to Perceiver examples #19129

Merged
merged 6 commits into from
Sep 23, 2022

Conversation

stevenmanton
Copy link
Contributor

@stevenmanton stevenmanton commented Sep 20, 2022

What does this PR do?

Related to #16292

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?

Taken from #16292 : @patrickvonplaten @ydshieh @patil-suraj

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 20, 2022

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

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 21, 2022

Hi @stevenmanton

Thank you a lot for the PR! Currently the test will fail for the following

FAILED src/transformers/models/perceiver/modeling_perceiver.py::transformers.models.perceiver.modeling_perceiver.PerceiverForImageClassificationConvProcessing.forward
FAILED src/transformers/models/perceiver/modeling_perceiver.py::transformers.models.perceiver.modeling_perceiver.PerceiverForImageClassificationFourier.forward
FAILED src/transformers/models/perceiver/modeling_perceiver.py::transformers.models.perceiver.modeling_perceiver.PerceiverForImageClassificationLearned.forward

We should add the expected outputs in the doc examples.
For other doc examples in this model, the tests pass, but they don't have outputs to test.
For example, PerceiverForOpticalFlow has

>>> logits = outputs.logits

without output and the expected value. We can have something like

>>> list(logits .shape)
expected shapes

Would you like to fix and enhance those doc examples?

Once you have a change (staged or commited), you can run the test like

python utils/prepare_for_doc_test.py src/transformers/models/perceiver/modeling_perceiver.py
pytest --doctest-modules src/transformers/models/perceiver/modeling_perceiver.py -sv --doctest-continue-on-failure

Once the test is run, you can clean up the git status before further changes or push.

Thanks!

@stevenmanton
Copy link
Contributor Author

@ydshieh thanks for the quick feedback! Yes, I noticed those tests were failing, but I thought it might be something about my local environment and the extra newline stuff. I just pushed a fix, which passes locally.

By the way, how did you know those tests were failing? The CI pipelines all seemed to be passing. Did you have to checkout my branch and run it locally?

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 21, 2022

@stevenmanton Thanks for the push. However, instead of changing to

>>> predicted_class = model.config.id2label[predicted_class_idx]

we should change it to

>>> print("Predicted class:", model.config.id2label[predicted_class_idx])
add some output here - so the doctest will test again it

You can find similar work is done

>>> print("Predicted class:", model.config.id2label[predicted_class_idx])
Predicted class: maillot

Otherwise, the example has nothing to be tested.

By the way, how did you know those tests were failing? The CI pipelines all seemed to be passing. Did you have to checkout my branch and run it locally?

Yes. The PR CI on CircleCI does not run doctest :-). It is run after the PR being merged.

@stevenmanton
Copy link
Contributor Author

@ydshieh Thanks for your feedback and patience. I believe I've corrected it. The extra newline stuff is confusing, but if you run prepare_for_doc_test.py (which I think just adds a newline to all the docstrings) on the last commit, all tests pass for me locally.

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 22, 2022

Thanks! As we are going to run the doctest for this model, would you like to add some expected outputs at the following places?

And a few places in this doc example (for logits and loss)

For logits, it would look like to add (the values should be collected from the run)

        >>> list(logits.shape)
        [1, 196, 8192]

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 22, 2022

When running prepare_for_doc_test.py, it will add some empty lines - to make doctest pass. That is why we should stage our change, run that script, run doctest, and discard the change before commit or further changes :-)

@stevenmanton
Copy link
Contributor Author

@ydshieh Ok, I added some more checks for the sizes of logits. They all pass for me locally.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Hi @stevenmanton Thanks a lot of this PR and the follow-up effort :-). It's great, and I just added an extra expected loss value for PerceiverForMaskedLM.

@sgugger As I am not sure if this PR is classified as a PR that I can merge myself, tagging you for a review or let me know if I can merge the doctest PR in the future. Thanks.

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 a lot for this PR!

@ydshieh Always good to have another pair of eyes :-)
Don't hesitate to merge such PRs without my approval in the future if I take too long to review, but I'd appreciate the opportunity to review!

@ydshieh ydshieh merged commit 49bf569 into huggingface:main Sep 23, 2022
@stevenmanton stevenmanton deleted the doctest-perceiver branch September 23, 2022 17:47
oneraghavan pushed a commit to oneraghavan/transformers that referenced this pull request Sep 26, 2022
* Fix bug in example and add to tests

* Fix failing tests

* Check the size of logits

* Code style

* Try again...

* Add expected loss for PerceiverForMaskedLM doctest

Co-authored-by: Steven Anton <antonstv@amazon.com>
Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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.

4 participants