Skip to content

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

Merged
ydshieh merged 4 commits into
mainfrom
revert-23245-revert-22987-doct-tests
May 10, 2023
Merged

Bring back the PR Refactor doctests + add CI to main#23271
ydshieh merged 4 commits into
mainfrom
revert-23245-revert-22987-doct-tests

Conversation

@ydshieh

@ydshieh ydshieh commented May 10, 2023

Copy link
Copy Markdown
Collaborator

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.

@sgugger sgugger left a comment

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.

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

ydshieh commented May 10, 2023

Copy link
Copy Markdown
Collaborator Author

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

sgugger commented May 10, 2023

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown

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

@sgugger sgugger left a comment

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.

Thanks a lot!

Comment thread .circleci/config.yml Outdated
- v0.6-repository_consistency
- run: pip install --upgrade pip
- run: pip install .[all,quality]
- run: pip install pytest

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.

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
@ArthurZucker

Copy link
Copy Markdown
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

ydshieh commented May 23, 2023

Copy link
Copy Markdown
Collaborator Author

@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
Copy Markdown
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 <[email protected]>
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 <[email protected]>
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