[Doctests] Refactor doctests + add CI by ArthurZucker · Pull Request #22987 · huggingface/transformers

ArthurZucker · 2023-04-25T11:40:57Z

What does this PR do?

Wow! The fix is crazy simple but I broke my head finding the correct way to include this in the cleanest way.
We are keeping pytest --doctest-module 🥳 Basically it just came down to:

change the doctest.DocTestParser()'s default regex compilation
rewrite _pytest.doctest utilities that are private to use this parser!

TODOS:

change parser
add CI Job
Filter jobs that can't run on CUDA! This is pretty important
update doc
fiind a way to default --doctest-glob but that's a small nit
add test, to test the parser mostly
add check file is doctested if it has some doctring

HuggingFaceDocBuilderDev · 2023-04-25T12:14:02Z

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

ArthurZucker · 2023-04-26T08:12:06Z

Local tests run, I now have this strange error:

_____ ERROR collecting src/transformers/models/whisper/modeling_whisper.py _____
import file mismatch:
imported module 'transformers.models.whisper.modeling_whisper' has this __file__ attribute:
  /home/circleci/.pyenv/versions/3.8.12/lib/python3.8/site-packages/transformers/models/whisper/modeling_whisper.py
which is not the same as the test file we want to collect:
  /home/circleci/transformers/src/transformers/models/whisper/modeling_whisper.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

ydshieh · 2023-04-26T14:39:26Z

@ArthurZucker Regarding the error you mentioned in the above comment, could you provide the command you used to launch?

Also, for this PR to be merged, two thing we should check are:

for a single modeling file, how long it will take to run the doctest against it on CircleCI, and if it will fit in the available memory.
- (we should probably run the doctest against a few existing models)
There should NOT have multiple modeling files being included in test_to_run for doctest.
- This PR currently checks if a file is in utils/documentation_tests.txt, but that file doesn't contain all existing modeling files if I remember correctly.

ArthurZucker · 2023-04-26T16:26:44Z

Regarding

There should NOT have multiple modeling files being included in test_to_run for doctest.
This PR currently checks if a file is in utils/documentation_tests.txt, but that file doesn't contain all existing modeling files if I remember correctly.

Actually the CI checks doc for all files in the diff that end in .py and .mdx. This is prone to changes! Fully open to recommandations.

For slow test, I skip all codeblocks that includ "cuda" in it, we can refine the filter.

ArthurZucker · 2023-04-27T10:10:52Z

CUDA tests are properly skipped! :

(11 durations < 0.005s hidden.  Use -vv to show these durations.)
=================================================================================================================================================================== short test summary info ====================================================================================================================================================================
PASSED docs/source/en/testing.mdx::testing.mdx
PASSED docs/source/en/testing.mdx::testing.mdx
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperForAudioClassification.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperForAudioClassification.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperForAudioClassification.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperForConditionalGeneration.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperForConditionalGeneration.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperForConditionalGeneration.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperModel.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperModel.forward
PASSED src/transformers/models/whisper/modeling_whisper.py::transformers.models.whisper.modeling_whisper.WhisperModel.forward
PASSED docs/source/en/model_doc/wav2vec2.mdx::wav2vec2.mdx
PASSED docs/source/en/model_doc/wav2vec2.mdx::wav2vec2.mdx
SKIPPED [1] <doctest testing.mdx[0]>:1: Codeblock line xxx in xxxx has cuda involved. Skipping
SKIPPED [1] <doctest wav2vec2.mdx[0]>:1: Codeblock line xxx in xxxx has cuda involved. Skipping

Results (19.65s):
       3 passed
       2 skipped
(py39) arthur_huggingface_co@arthur-gpu-2:~/transformers$

ArthurZucker · 2023-04-27T10:11:03Z

Warning will be imporved

sgugger

Thanks for working on this. I have a couple of comments.

sgugger · 2023-04-28T13:08:36Z

-```

-2. Then, you can use the following line to automatically test every docstring example in the desired file: 
+Just run the following line to automatically test every docstring example in the desired file: 


Much simpler, nice!

ydshieh · 2023-04-29T04:19:14Z

Looked this PR and played a bit with it: so far so good 👍

One thing I found:

SKIP_CUDA_DOCTEST=1 pytest -v --doctest-modules --doctest-glob="*.mdx" docs/source/en/model_doc/longt5.mdx

The doctest is running while I assume it will be skipped as it has cuda thing.

ydshieh

Thank you a lot for this awesome improvement! I have a few nit comments, especially for adding a bit more comments to explain things along the code.

It seems SKIP_CUDA_DOCTEST=1 is not working however.

ydshieh · 2023-04-29T04:20:20Z

    resource_class: Optional[str] = "xlarge"
    tests_to_run: Optional[List[str]] = None
    working_directory: str = "~/transformers"
+    timeout :int = None


could we add a comment to mention what it is for?
(i.e. not to confuse the readers with pytest timeout and this timeout)

+1. Alternatively, another var name like ci_timeout so the code is self documenting

ydshieh · 2023-04-29T04:21:31Z

+        },
+    ],
+    tests_to_run="$(cat pr_documentation_tests.txt)",
+    pytest_options={"-doctest-modules":None, "doctest-glob":"*.mdx","dist":"loadfile", "rvsA":None},


-doctest-modules seems incorrect and should be doctest-modules

also, the format should be better: spaces after : or ,

doctest-modules alone doesn’t work, since it’s only adding a single -

doctest-modules alone doesn’t work, since it’s only adding a single -

For such unusual situation, I would recommend to leave some comments in the code (and if not, in the PR page) to facilitate the review as well as for future HF developers.

I'm not sure I follow, why is it adding only a single -? I agree with @ydshieh, this is non-obvious and to someone coming cold to the code I think confusing without further context.

@amyeroberts I think it is because we use --doctest-modules without argument. But in create_circleci_config.py we have

pytest_flags = [f"--{key}={value}" if value is not None else f"-{key}" for key, value in all_options.items()]

So we need put -doctest-modules and the above line will change it to --doctest-modules (that is what is required)

This is exceptional however, and we need to make it clear in some way :-)

In this case, I'd say we should update this logic as it's not properly handling command line options :) e.g. something like this:

PYTEST_BOOL_FLAG = ['doctest-modules',] ... pytest_flags = [] for key, value in all_options.items(): if key in PYTEST_BOOL_FLAG: flag = f"--{key}" elif value is not None: flag = f"--{key}={value}" else: flag = f"-{key} pytest_flags.append(flag)

I do agree :-)

ydshieh · 2023-04-29T04:23:25Z

-```

-2. Then, you can use the following line to automatically test every docstring example in the desired file: 
+Just run the following line to automatically test every docstring example in the desired file: 


ydshieh · 2023-04-29T04:23:59Z

+
+
+def preprocess_string(string, skip_cuda_tests):
+    codeblock_pattern = r"(```(?:python|py)\s*\n\s*>>> )((?:.*?\n)*?.*?```)"


probably a docstring to explain a bit what this does

ydshieh · 2023-04-29T04:24:22Z

+            ] += f'from pytest import skip;skip("Codeblock line {line_num} uses `cuda`. Skipping" \
+                                ,allow_module_level=True);'
+    codeblocks = "".join(codeblocks)
+    return codeblocks


some comments along the code lines would be appreciated.

ArthurZucker · 2023-04-30T05:22:46Z

What should be detailed is that only the codeblocks (and not the entire file) should be skipped. This might be why longt5 is not skipped!
I’ll be off for a while, I leave this in your hands! 🤗🤗

amyeroberts

Thanks for working on this - resulting code looks really nice and makes things a lot simpler.

Just a few nits to clarify some part. Could you also add in the PR description what "the fix" is i.e. intention of the PR?

amyeroberts · 2023-05-02T11:44:33Z

    resource_class: Optional[str] = "xlarge"
    tests_to_run: Optional[List[str]] = None
    working_directory: str = "~/transformers"
+    timeout :int = None


+1. Alternatively, another var name like ci_timeout so the code is self documenting

amyeroberts · 2023-05-03T11:22:23Z

+
+
+def preprocess_string(string, skip_cuda_tests):
+    codeblock_pattern = r"(```(?:python|py)\s*\n\s*>>> )((?:.*?\n)*?.*?```)"


amyeroberts · 2023-05-03T11:25:43Z

+    means that there are no extra lines at the end of our snippets. The `# doctest: +IGNORE_RESULT` marker is also
+    added anywhere a `load_dataset` call is made as a print would otherwise fail the corresonding line.


I'm not able to parse this final sentence. For:

"anywhere a load_dataset call is made as a print would otherwise fail the corresonding line"

It's read to me as a print call would fail after a load_dataset line - is this right?

load_dataset itself will print something, so we need # doctest: +IGNORE_RESULT when load_dataset is called.

amyeroberts · 2023-05-03T11:50:26Z


+doc_test_job = CircleCIJob(
+    "pr_documentation_tests",
+    additional_env={"TRANSFORMERS_VERBOSITY":"error", "DATASETS_VERBOSITY":"error", "SKIP_CUDA_DOCTEST": "1"},


Just for my own understanding and doulbe checking - this means cuda docstests are skipped for the doc test job?

On circleCI only, which does not have GPU runners.

amyeroberts · 2023-05-03T11:50:48Z

-        run: |
-          python3 utils/prepare_for_doc_test.py src docs
-
      - name: Run doctests


✨ so much cleaner ✨

ydshieh · 2023-05-03T13:27:02Z

For info: I will take over this PR to try to merge it earlier.

ydshieh · 2023-05-03T16:08:35Z

Convert to draft for now, as more changes to deal with cuda is required.

ydshieh · 2023-05-03T19:31:27Z

+    for i, codeblock in enumerate(codeblocks):
+        if "load_dataset(" in codeblock and "# doctest: +IGNORE_RESULT" not in codeblock:
+            codeblocks[i] = re.sub(r"(>>> .*load_dataset\(.*)", r"\1 # doctest: +IGNORE_RESULT", codeblock)
+        if (">>>" in codeblock or "..." in codeblock) and re.search(r"cuda|to\(0\)|device=0", codeblock) and skip_cuda_tests:


The previous regular expression with re.match is just wrong.

and we also need to check ...

ydshieh · 2023-05-03T19:33:56Z

+    modified_string = ""
+    if not is_cuda_found:
+        modified_string = "".join(codeblocks)
+    return modified_string


As discussed offline with @sgugger , for all doc example blocks in a single docstring, we should skip all the doc examples if one of them contains cuda stuff. Otherwise, we may have errors like NameError: name 'classifier' is not defined

ydshieh · 2023-05-03T19:34:10Z

+        if "load_dataset(" in codeblock and "# doctest: +IGNORE_RESULT" not in codeblock:
+            codeblocks[i] = re.sub(r"(>>> .*load_dataset\(.*)", r"\1 # doctest: +IGNORE_RESULT", codeblock)
+        if (">>>" in codeblock or "..." in codeblock) and re.search(r"cuda|to\(0\)|device=0", codeblock) and skip_cuda_tests:
+            is_cuda_found = True


Co-authored-by: Sylvain Gugger <[email protected]>

Co-authored-by: amyeroberts <[email protected]>

This reverts commit 627f447.

Revert "[Doctests] Refactor doctests + add CI (#22987)" This reverts commit 627f447.

* intiial commit * new styling * update * just run doctest in CI * remove more test for fast dev * update * update refs * update path and fetch upstream * update documentatyion trests * typo * parse pwd * don't check for files that are in hidden folders * just give paths relative to transformers * update * update * update * major refactoring * make sure options is ok * lest test that mdx is tested * doctest glob * nits * update doctest nightly * some cleaning * run correct test on diff * debug * run on a single worker * skip_cuda_test tampkate * updates * add rA and continue on failure * test options * parse `py` codeblock? * we don't need to replace ignore results, don't remember whyu I put it * cleanup * more cleaning * fix arg * more cleaning * clean an todo * more pre-processing * doctest-module has none so extra `- ` is needed * remove logs * nits * doctest-modules .... * oups * let's use sugar * make dataset go quiet * add proper timeout * nites * spleling timeout * update * properly skip tests that have CUDSA * proper skipping * cleaning main and get tests to run * remove make report? * remove tee * some updates * tee was removed but is the full output still available? * [all-test] * only our tests * don't touch tee in this PR * no atee-sys * proper sub * monkey * only replace call * fix sub * nits * nits * fix invalid syntax * add skip cuda doctest env variable * make sure all packages are installed * move file * update check repo * revert changes * nit * finish cleanup * fix re * findall * update don't test init files * ignore pycache * `-ignore-pycache` when running pytests * try to fix the import missmatch error * install dec * pytest is required as doctest_utils imports things from it * the only log issues were dataset, ignore results should work * more cleaning * Update .circleci/create_circleci_config.py Co-authored-by: Sylvain Gugger <[email protected]> * Apply suggestions from code review Co-authored-by: amyeroberts <[email protected]> * [ydshieh] empty string if cuda is found * [ydshieh] fix condition * style * [ydshieh] fix * Add comment * style * style * show failure * trigger CI --------- Co-authored-by: Sylvain Gugger <[email protected]> Co-authored-by: Yih-Dar <[email protected]> Co-authored-by: amyeroberts <[email protected]> Co-authored-by: ydshieh <[email protected]>

Revert "[Doctests] Refactor doctests + add CI (huggingface#22987)" This reverts commit 627f447.

* intiial commit * new styling * update * just run doctest in CI * remove more test for fast dev * update * update refs * update path and fetch upstream * update documentatyion trests * typo * parse pwd * don't check for files that are in hidden folders * just give paths relative to transformers * update * update * update * major refactoring * make sure options is ok * lest test that mdx is tested * doctest glob * nits * update doctest nightly * some cleaning * run correct test on diff * debug * run on a single worker * skip_cuda_test tampkate * updates * add rA and continue on failure * test options * parse `py` codeblock? * we don't need to replace ignore results, don't remember whyu I put it * cleanup * more cleaning * fix arg * more cleaning * clean an todo * more pre-processing * doctest-module has none so extra `- ` is needed * remove logs * nits * doctest-modules .... * oups * let's use sugar * make dataset go quiet * add proper timeout * nites * spleling timeout * update * properly skip tests that have CUDSA * proper skipping * cleaning main and get tests to run * remove make report? * remove tee * some updates * tee was removed but is the full output still available? * [all-test] * only our tests * don't touch tee in this PR * no atee-sys * proper sub * monkey * only replace call * fix sub * nits * nits * fix invalid syntax * add skip cuda doctest env variable * make sure all packages are installed * move file * update check repo * revert changes * nit * finish cleanup * fix re * findall * update don't test init files * ignore pycache * `-ignore-pycache` when running pytests * try to fix the import missmatch error * install dec * pytest is required as doctest_utils imports things from it * the only log issues were dataset, ignore results should work * more cleaning * Update .circleci/create_circleci_config.py Co-authored-by: Sylvain Gugger <[email protected]> * Apply suggestions from code review Co-authored-by: amyeroberts <[email protected]> * [ydshieh] empty string if cuda is found * [ydshieh] fix condition * style * [ydshieh] fix * Add comment * style * style * show failure * trigger CI --------- Co-authored-by: Sylvain Gugger <[email protected]> Co-authored-by: Yih-Dar <[email protected]> Co-authored-by: amyeroberts <[email protected]> Co-authored-by: ydshieh <[email protected]>

Revert "[Doctests] Refactor doctests + add CI (huggingface#22987)" This reverts commit 627f447.

ArthurZucker changed the title ~~[Doctests] Refactor doctests, don't merge experimental~~ [Doctests] Refactor doctests + add CI + add check Apr 26, 2023

ArthurZucker marked this pull request as ready for review April 26, 2023 06:45

ArthurZucker commented Apr 27, 2023

View reviewed changes

Comment thread docs/source/en/testing.mdx Outdated

ArthurZucker commented Apr 27, 2023

View reviewed changes

Comment thread docs/source/en/testing.mdx Outdated

ArthurZucker commented Apr 27, 2023

View reviewed changes

Comment thread src/transformers/models/whisper/modeling_whisper.py Outdated

ArthurZucker commented Apr 27, 2023

View reviewed changes

Comment thread utils/check_repo.py Outdated

ArthurZucker changed the title ~~[Doctests] Refactor doctests + add CI + add check~~ [Doctests] Refactor doctests + add CI Apr 28, 2023

ArthurZucker requested review from amyeroberts, sgugger and ydshieh April 28, 2023 09:27

sgugger reviewed Apr 28, 2023

View reviewed changes

ArthurZucker mentioned this pull request Apr 28, 2023

[Doctest] Add new checks #23046

Closed

sgugger approved these changes Apr 28, 2023

View reviewed changes

Comment thread .circleci/config.yml

ydshieh reviewed Apr 29, 2023

View reviewed changes

amyeroberts approved these changes May 3, 2023

View reviewed changes

ydshieh marked this pull request as draft May 3, 2023 16:08

ydshieh force-pushed the doct-tests branch from 7811553 to 00e7a7b Compare May 3, 2023 19:25

ydshieh reviewed May 3, 2023

View reviewed changes

ArthurZucker and others added 16 commits May 9, 2023 08:03

-ignore-pycache when running pytests

2616855

try to fix the import missmatch error

9c98bc6

install dec

62c211c

pytest is required as doctest_utils imports things from it

4ae65a2

the only log issues were dataset, ignore results should work

a1d8d10

more cleaning

02fe69a

Update .circleci/create_circleci_config.py

0e1a0e8

Co-authored-by: Sylvain Gugger <[email protected]>

Apply suggestions from code review

e3e27fd

Co-authored-by: amyeroberts <[email protected]>

[ydshieh] empty string if cuda is found

b5fff26

[ydshieh] fix condition

f15ef74

style

f5b4428

[ydshieh] fix

e6c531a

Add comment

3bef5cc

style

5c120a8

style

23ce0e1

show failure

3689837

ydshieh force-pushed the doct-tests branch from ff8b7cb to 3689837 Compare May 9, 2023 08:24

trigger CI

3f4fb14

ydshieh merged commit 627f447 into main May 9, 2023

ydshieh deleted the doct-tests branch May 9, 2023 18:34

ydshieh mentioned this pull request May 9, 2023

Hot Fix #23244

Closed

sgugger added a commit that referenced this pull request May 9, 2023

Revert "[Doctests] Refactor doctests + add CI (#22987)"

c7831e1

This reverts commit 627f447.

sgugger mentioned this pull request May 9, 2023

Revert "[Doctests] Refactor doctests + add CI" #23245

Merged

sgugger added a commit that referenced this pull request May 9, 2023

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

69ee462

Revert "[Doctests] Refactor doctests + add CI (#22987)" This reverts commit 627f447.

sgugger added a commit that referenced this pull request May 9, 2023

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

fee5b5e

Revert "[Doctests] Refactor doctests + add CI (#22987)" This reverts commit 627f447.

ydshieh mentioned this pull request May 10, 2023

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

Merged

gojiteji pushed a commit to gojiteji/transformers that referenced this pull request Jun 5, 2023

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

01a81b9

Revert "[Doctests] Refactor doctests + add CI (huggingface#22987)" This reverts commit 627f447.

novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023

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

b6a5199

Revert "[Doctests] Refactor doctests + add CI (huggingface#22987)" This reverts commit 627f447.



		def preprocess_string(string, skip_cuda_tests):
		codeblock_pattern = r"(```(?:python\|py)\s\n\s>>> )((?:.?\n)?.*?```)"

		means that there are no extra lines at the end of our snippets. The `# doctest: +IGNORE_RESULT` marker is also
		added anywhere a `load_dataset` call is made as a print would otherwise fail the corresonding line.

Uh oh!

Conversation

ArthurZucker commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

HuggingFaceDocBuilderDev commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArthurZucker commented Apr 26, 2023

Uh oh!

ydshieh commented Apr 26, 2023

Uh oh!

ArthurZucker commented Apr 26, 2023

Uh oh!

ArthurZucker commented Apr 27, 2023

Uh oh!

ArthurZucker commented Apr 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ydshieh commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydshieh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurZucker commented Apr 30, 2023

Uh oh!

amyeroberts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

ArthurZucker commented Apr 25, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Apr 25, 2023 •

edited

Loading

ydshieh commented Apr 29, 2023 •

edited

Loading