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

Bring back the PR Refactor doctests + add CI to main #23271

Merged
merged 4 commits into from
May 10, 2023

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented May 10, 2023

Reverts #23245

So we can keep the PR #22987 regarding the new doctest way, but without exposing doctest_utils to src/transformers.

@sgugger Let me know if you prefer to move this doctest_utils.py to tests folder.

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.

Can you put the new testing utils (from doctest_utils) in testing_utils, so it all goes in the same place?

@ydshieh ydshieh marked this pull request as ready for review May 10, 2023 17:50
@ydshieh
Copy link
Collaborator Author

ydshieh commented May 10, 2023

Can you put the new testing utils (from doctest_utils) in testing_utils, so it all goes in the same place?

In this case, am I allowed to put the import of pytest and _pytest on the top level of testing_utils? I am asking because I see in that file there is

        try:
            import pytest  # We don't need a hard dependency on pytest in the main library
        except ImportError:
            return test_case

@sgugger
Copy link
Collaborator

sgugger commented May 10, 2023

There is no direct import into testing_utils.py so this should be fine to remove the try except (we will have until next release to make sure it doesn't create a new core dep of Transformers 😅 )

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@ydshieh ydshieh requested a review from sgugger May 10, 2023 19:19
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!

@@ -166,6 +166,7 @@ jobs:
- v0.6-repository_consistency
- run: pip install --upgrade pip
- run: pip install .[all,quality]
- run: pip install pytest
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be necessary now, no?

@ydshieh ydshieh changed the title Revert "Revert "[Doctests] Refactor doctests + add CI"" Bring back the PR Refactor doctests + add CI to main May 10, 2023
@ydshieh ydshieh merged commit 9088fca into main May 10, 2023
@ydshieh ydshieh deleted the revert-23245-revert-22987-doct-tests branch May 10, 2023 20:00
ydshieh added a commit that referenced this pull request May 10, 2023
@ArthurZucker
Copy link
Collaborator

Thanks for taking care of this! Think the filtered list could be obtain in a cleaner way with some bash commands but otherwise great 👍🏻

@ydshieh
Copy link
Collaborator Author

ydshieh commented May 23, 2023

@ArthurZucker It was finally going to a tests_fetcher.py

https://github.com/huggingface/transformers/pull/23277/files

The bash command was just getting too complex ...

@ArthurZucker
Copy link
Collaborator

Nice! Thanks for following up

@ydshieh ydshieh mentioned this pull request Jun 5, 2023
gojiteji pushed a commit to gojiteji/transformers that referenced this pull request Jun 5, 2023
…#23271)

* Revert "Revert "[Doctests] Refactor doctests + add CI" (huggingface#23245)"

This reverts commit 69ee462.

* try not expose HfDocTestParser

* move into testing_utils.py

* remove pytest install

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
…#23271)

* Revert "Revert "[Doctests] Refactor doctests + add CI" (huggingface#23245)"

This reverts commit 69ee462.

* try not expose HfDocTestParser

* move into testing_utils.py

* remove pytest install

---------

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