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.

Sign up for GitHub

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

Jump to bottom

Run doctest for new files #25588

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Run doctest for new files #25588

merged 1 commit into from
Aug 21, 2023

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Aug 18, 2023 •
edited
Loading

What does this PR do?

The effect could be see in this run (see the artifact).

From Sir @ArthurZucker

new files should always be tested

the documentation_tests.txt is only for legacy models taht are not tested

but whenever someone adds something, (a new pipeline, a new model etc) should be automatic

ydshieh
ydshieh commented Aug 18, 2023
View reviewed changes
utils/tests_fetcher.py Outdated
x for x in test_files_to_run if x in documentation_tests and x not in slow_documentation_tests
x
for x in test_files_to_run
if x in new_files or (x in documentation_tests and x not in slow_documentation_tests)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new files are always added

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 18, 2023 •
edited
Loading

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

sgugger
sgugger reviewed Aug 18, 2023
View reviewed changes
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 for working on this. It would be better to either:
a) reverse the list in the doctest file to include the files not tested yet (cause the docstrings haven't been fixed yet) in which case new files are automatically tested

or

b) put a check in the utils that checks the doctest file is properly sorted to make sure it includes new files

Either way, the new files would be always tested afterward (whereas here they are only tested when added). My preference goes for a if it's not too much work.

@ydshieh
Copy link
Collaborator Author

ydshieh commented Aug 18, 2023

@ArthurZucker also mentioned a to me. I am happy with that, but we have to be a bit careful not to include files that has no doc example in that opposite list (if we want to use that list as an indication of missing doctest - it depends what we interpret it and use it)

@ydshieh ydshieh force-pushed the fix_doc_test_files branch from 826355b to 3acc08e Compare August 18, 2023 13:00
ydshieh
ydshieh commented Aug 18, 2023
View reviewed changes
utils/tests_fetcher.py Outdated
# Do not run slow doctest tests
with open("utils/slow_documentation_tests.txt") as fp:
slow_documentation_tests = set(fp.read().strip().split("\n"))

# So far we don't have 100% coverage for doctest. This line will be removed once we achieve 100%.
test_files_to_run = [
x for x in test_files_to_run if x in documentation_tests and x not in slow_documentation_tests
x for x in test_files_to_run if x not in not_doctested and x not in slow_documentation_tests
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The files slow_documentation_tests won't be in not_doctested - they are doctested, but only on daily CI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

once PR approved, I will remove the file documentation_list.txt

@ydshieh
Copy link
Collaborator Author

ydshieh commented Aug 18, 2023

Added a not_doctested.txt. This list needs some more work, in particular src/transformers/pipelines. We should remove them out from this file so we can test them - but we have to make sure they work first!

(so far on main, they are not doctested)

I would prefer to separate that work from this PR.

@ydshieh ydshieh requested a review from sgugger August 18, 2023 13:07
ArthurZucker
ArthurZucker reviewed Aug 18, 2023
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

don't think that init files for example have anydoctest, so should probably not be here, and would prefer if only files that have doc example be here! (tests/* probably don't no?)

@ydshieh ydshieh requested a review from ArthurZucker August 18, 2023 13:28
sgugger
sgugger reviewed Aug 18, 2023
View reviewed changes
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 for diving into this! I think we can have stricter rules: only look in the english doc for md files and only in the src fodler for .py files. Everything else will never be doc-tested, and this will make the not_doctested file easier to read.

Also please include this file in the doctest_file check (so it's always sorted in alphabetical order).

utils/not_doctested.txt Outdated Show resolved Hide resolved
utils/not_doctested.txt Outdated Show resolved Hide resolved
utils/not_doctested.txt Outdated Show resolved Hide resolved
utils/not_doctested.txt Outdated Show resolved Hide resolved
utils/not_doctested.txt Outdated Show resolved Hide resolved
ArthurZucker
ArthurZucker approved these changes Aug 18, 2023
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Much better! Fine for me 😉

sgugger
sgugger approved these changes Aug 18, 2023
View reviewed changes
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 for iterating!

@ydshieh ydshieh force-pushed the fix_doc_test_files branch from 95d4310 to e05d404 Compare August 21, 2023 08:29
@ydshieh ydshieh force-pushed the fix_doc_test_files branch from 6c5558a to 18fe148 Compare August 21, 2023 08:55
@ydshieh
Copy link
Collaborator Author

ydshieh commented Aug 21, 2023

I am going to merge this PR without removing utils/documentation_tests.txt yet. It's still used by the doctest workflow file, and changing that file to use the opposite logic needs some extra work.

Will do it in a follow up PR.

@ydshieh ydshieh merged commit f09db47 into main Aug 21, 2023
@ydshieh ydshieh deleted the fix_doc_test_files branch August 21, 2023 09:08
@HuggingFaceDocBuilderDev
Copy link

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

sgugger pushed a commit that referenced this pull request Aug 21, 2023
@ydshieh @sgugger
Run doctest for new files (#25588)
5ddafec 
fix

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
@ydshieh @parambharat
Run doctest for new files (huggingface#25588)
cefd472 
fix

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
@ydshieh @blbadger
Run doctest for new files (huggingface#25588)
ca40370 
fix

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
Reviewers

@ArthurZucker ArthurZucker ArthurZucker approved these changes

@sgugger sgugger sgugger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ydshieh @HuggingFaceDocBuilderDev @sgugger @ArthurZucker