Test fetch v2 #22367

sgugger · 2023-03-24T18:49:59Z

What does this PR do?

This PR rewrites the test fetcher util to be more accurate in the tests collection, and also comes with a restriction on the tests run when a large amount of tests are picked when modifying a core file (like modeling_utils).

The code that extracts the dependencies of a given module now inspects the inits to pinpoint the exact location of imported objects. So for instance if a test file has an import from transformers import BertModel, this new version will detect a dependency on transformers/models/bert/modeling_bert.py. As a comparison, the previous version stopped at transformers/__init__.py. This removes the need for all the complex logic that tried to match a given file with its corresponding tests, we now just look at the dependencies of the test file.

The second change is that when a given file is seen to trigger too many model tests (current trigger is set at half the models, it can evolve), it will only keep the tests relative to a given list of important models. If a PR changes many modeling files, all the tests for those models will still run, but if a PR only changes modeling_utils (for instance), this will trigger the core model tests only. The list of important models is built using:

the most downloaded models in the last 30 days
making sure each pipeline has a model in that list

To bypass this rule, one can add a special command in a commit message (circleCI does not have access to labels, so I can't rely on that):

Including [skip ci] or [ci skip] or [circleci skip] or [skip circleci] or any variants with - or _ instead of a space will skip all tests
Including [test all models] or any variant with the words in another order and/or with - or _ instead of a space will run all tests found without filtering on important models.
Including [test all] or [all test] or any variants with - or _ instead of a space will run all tests.

A couple of adjustments to Transformers should be done (in follow-up PRs) to have the test fetcher be more accurate and more efficient:

make sure all inits don't define any objects. Most of our inits only import all the stuff, and the test fetcher assumes they are all like that. Some inits (like pipeline/__init__.py) define real objects, it would be best to move them to a submodule.
make sure test files test one thing: for instance test_modeling_common.py contains both the common tests and the test of the modeling_utils module. It would be best to split those in two files.

Lastly, this PR adds lots of tests to make sure future work doesn't break the test fetcher :-)

To see how the test fetcher behaves on some examples:

for a modification in modeling_opt.py: only test_modeling_opt is run [fetch summary] [job page]
for a modification in modeling_bert.py (which is imported in all the tests basically) all tests using BERT are run, but filtered to the list of important models [fetch summary] [job page]
for a modification in a pipeline file: all model tests are run, filtered to the list of important models [fetch summary] [job page]
for a modification in the main __init__.py all tests are run, but filtered to the list of important models [fetch summary] [job page]
for a modification in the setup.py all tests are run [fetch summary] [job page]

HuggingFaceDocBuilderDev · 2023-03-24T19:09:35Z

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

ydshieh · 2023-03-29T10:42:08Z

I guess it's ready for a review?

sgugger · 2023-03-29T13:45:18Z

No I haven't finished this PR yet.

LysandreJik

Cool, LGTM! Looking forward to seeing it run!

LysandreJik · 2023-03-30T18:27:11Z

utils/tests_fetcher.py

@@ -667,11 +611,29 @@ def filter_tests(output_file, filters):
        f.write(" ".join(test_files))


+def parse_commit_message(commit_message):


I would tend to favor explicit returns such as a dict {"skip": skip, "all_models": all_models, "test_all": test_all} so that we don't need to look for the docs to understand what to do with the returned value

utils/tests_fetcher.py

LysandreJik · 2023-03-30T18:29:40Z

utils/tests_fetcher.py

+    # Sagemaker tests are not meant to be run on the CI.
+    if "tests/sagemaker" in tests:
+        tests.remove("tests/sagemaker")


to verify with @philschmid

Co-authored-by: Lysandre Debut <[email protected]>

ydshieh · 2023-03-31T07:10:11Z

Hi @sgugger . Thank you a lot for working on this important task! I feel it's better for me to look this work in depth, and I tried to play with the test fetcher (on main and on this PR) to understand it better.

However, the first thing I tried (by following some sentences you mentioned) makes me a somehow confused. Here is what I saw:

On the two branches main (or a new branch from it) and test_fetch_v2, do the following steps:
- change the test file tests/models/bert/test_modeling_bert.py (simply adding some dummy line like foo = 1)
- commit the change
```
git add tests/models/bert/test_modeling_bert.py
git commit -m "dummy commit"
```
- run the test fetcher against the previous commit
```
python utils/tests_fetcher.py  --diff_with_last_commit
```

Now, the results:
TL;DR: test_modeling_bert.py is not included by the new version of test fetcher. But I think it should be included.

on main
(tests/models/bert/test_modeling_bert.py is in TEST TO RUN and in the file test_list.txt)

### DIFF ###

### MODIFIED FILES ###
- tests/models/bert/test_modeling_bert.py

### IMPACTED FILES ###
- tests/models/auto/test_modeling_auto.py
- tests/models/auto/test_modeling_tf_auto.py
- tests/models/bert/test_modeling_bert.py
- tests/models/encoder_decoder/test_modeling_encoder_decoder.py
- tests/models/speech_encoder_decoder/test_modeling_speech_encoder_decoder.py
- tests/models/vision_encoder_decoder/test_modeling_vision_encoder_decoder.py
- tests/models/vision_text_dual_encoder/test_modeling_vision_text_dual_encoder.py

### TEST TO RUN ###
- tests/models/auto/test_modeling_auto.py
- tests/models/auto/test_modeling_tf_auto.py
- tests/models/bert/test_modeling_bert.py
- tests/models/encoder_decoder/test_modeling_encoder_decoder.py
- tests/models/speech_encoder_decoder/test_modeling_speech_encoder_decoder.py
- tests/models/vision_encoder_decoder/test_modeling_vision_encoder_decoder.py
- tests/models/vision_text_dual_encoder/test_modeling_vision_text_dual_encoder.py

on test_fetch_v2
(tests/models/bert/test_modeling_bert.py is NEITHER in TEST TO RUN, NOR in the file test_list.txt)

### MODIFIED FILES ###
- tests/models/bert/test_modeling_bert.py

### IMPACTED FILES ###
- tests/models/auto/test_modeling_auto.py
- tests/models/auto/test_modeling_tf_auto.py
- tests/models/bert/test_modeling_bert.py
- tests/models/encoder_decoder/test_modeling_encoder_decoder.py
- tests/models/speech_encoder_decoder/test_modeling_speech_encoder_decoder.py
- tests/models/vision_encoder_decoder/test_modeling_vision_encoder_decoder.py
- tests/models/vision_text_dual_encoder/test_modeling_vision_text_dual_encoder.py

### TEST TO RUN ###
- tests/models/auto/test_modeling_auto.py
- tests/models/auto/test_modeling_tf_auto.py
- tests/models/encoder_decoder/test_modeling_encoder_decoder.py
- tests/models/speech_encoder_decoder/test_modeling_speech_encoder_decoder.py
- tests/models/vision_encoder_decoder/test_modeling_vision_encoder_decoder.py
- tests/models/vision_text_dual_encoder/test_modeling_vision_text_dual_encoder.py

ydshieh · 2023-03-31T07:18:57Z

As a comparison, the previous version stopped at transformers/init.py.

Is the following block (on main) what you mentioned by the above sentence?

    # We ignore the main init import as it's only for the __version__ that it's done
    # and it would add everything as a dependency.
    if not imported_module.endswith("transformers/__init__.py"):
        ...

[Not question - just to record something so I won't forget later]
I tried to change src/transformers/models/bert/modeling_bert.py, and I can see

src/transformers/__init__.py is given as impacted in both versions
src/transformers/models/gpt2/xxx is given as impacted in the version on main but not the version on this PR
tests/models/gpt2/xxx is NOT given as impacted in the version on main but given in the version on this PR.
- but its in tests to run in bother version

ydshieh · 2023-03-31T07:34:15Z

Well, at least, when src/transformers/models/bert/modeling_bert.py is changed, the test file tests/models/bert/test_modeling_bert.py included 👍 . So the dependency detection seems to work well, and the above situation is just an edge case (to including self)

amyeroberts

No comments other than to say this was a very interesting and enjoyable PR to review ❤️

Thanks for adding!

amyeroberts · 2023-03-31T10:04:28Z

utils/tests_fetcher.py

+
+# List here the models to always test.
+IMPORTANT_MODELS = [
+    # Most downloaded models


Shall we have a reminder somewhere to periodically update this?

Not sure people are going to go read this periodically ;-) It's more of us to think of it when we add a new pipeline for instance.

amyeroberts · 2023-03-31T10:06:27Z

utils/tests_fetcher.py

+        return get_diff(repo, repo.head.commit, parent_commits)
+
+
+# (:?^|\n) -> Non-catching group for the beginning of the doc or a new line.


Detailed comments explaining regex ❤️ ❤️ ❤️

amyeroberts · 2023-03-31T10:44:54Z

tests/repo_utils/test_tests_fetcher.py

+    def test_infer_tests_to_run(self):
+        with tempfile.TemporaryDirectory() as tmp_folder:
+            tmp_folder = Path(tmp_folder)
+            models = models = ["bert", "gpt2"] + [f"bert{i}" for i in range(10)]


What's the reason for reassigning models here?

Just a typo ;-)

sgugger · 2023-03-31T13:50:13Z

@ydshieh, good catch on a modified test file missing from the tests launched. I have only put the dependencies and forgot those. Will fix.

sgugger · 2023-03-31T15:18:37Z

@ydshieh did you want to review more or is it good to merge?

ydshieh · 2023-03-31T16:13:55Z

@ydshieh did you want to review more or is it good to merge?

Hi @sgugger If you feel urgent to merge, go ahead (I can leave comments afterward anyway). Otherwise, I would love to continue the review process despite I am slow.

ydshieh

Thank you @sgugger again for the great work and the patience for my slow review.

I am actually learning things here rather than giving useful reviews, but left 2 nits (typo and var. naming).

utils/tests_fetcher.py

ydshieh · 2023-03-31T17:27:01Z

utils/tests_fetcher.py

    """
-    with open(os.path.join(PATH_TO_TRANFORMERS, module_fname), "r", encoding="utf-8") as f:
+    if cache is not None and module_fname in cache:
+        return cache[module_fname]


Very nice! I actually missed these 2 lines during the review, and I was thinking why not using cache but just adding content at the end.

ydshieh · 2023-03-31T17:29:27Z

utils/tests_fetcher.py

+            f for f in modified_files if f.startswith("tests") and f.split(os.path.sep)[-1].startswith("test")
+        ]
+        # Then we grab the corresponding test files.
+        test_map = create_module_to_test_map(reverse_map=reverse_map, filter_models=filter_models)


good not to create reverse_map twice!

ydshieh · 2023-03-31T17:34:35Z

utils/tests_fetcher.py

+        if commit_flags["test_all_models"]:
+            print("Testing all models found.")
+        if commit_flags["test_all"]:
+            print("Force- launching all tests")


When reading this part, I feel confused between between "test_all_models" and "test_all".
Maybe run_fetched_tests and run_all_tests make it more clear.
Leave you to decide.

(and "Run all fetched tests.)

Going for no_filter instead, as I find it clearer and shorter than run_fetched_tests.

ydshieh · 2023-03-31T17:40:18Z

tests/repo_utils/test_tests_fetcher.py

😮 💯 🔥

Co-authored-by: Yih-Dar <[email protected]>

HuggingFaceDocBuilderDev · 2023-03-31T20:29:54Z

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

* Test fetcher v2 * Fix regexes * Remove sanity check * Fake modification to OPT * Fixes some .sep issues * Remove fake OPT change * Fake modif for BERT * Fake modif for init * Exclude SageMaker tests * Fix test and remove fake modif * Fake setup modif * Fake pipeline modif * Remove all fake modifs * Adds options to skip/force tests * [test-all-models] Fake modif for BERT * Try this way * Does the command actually work? * [test-all-models] Try again! * [skip circleci] Remove fake modif * Remove debug statements * Add the list of important models * Quality * Update utils/tests_fetcher.py Co-authored-by: Lysandre Debut <[email protected]> * Address review comments * Address review comments * Fix and add test * Apply suggestions from code review Co-authored-by: Yih-Dar <[email protected]> * Address review comments --------- Co-authored-by: Lysandre Debut <[email protected]> Co-authored-by: Yih-Dar <[email protected]>

ydshieh self-assigned this Mar 27, 2023

sgugger added 6 commits March 29, 2023 14:42

Test fetcher v2

6c0c413

Fix regexes

87078e1

Remove sanity check

4baaed4

Fake modification to OPT

bc34193

Fixes some .sep issues

62c1f53

Remove fake OPT change

0148d8b

sgugger force-pushed the test_fetch_v2 branch from 1da94b6 to 0148d8b Compare March 29, 2023 18:43

sgugger added 16 commits March 29, 2023 14:43

Fake modif for BERT

f42d95b

Fake modif for init

ff4440d

Exclude SageMaker tests

cdf7fda

Fix test and remove fake modif

9160d22

Fake setup modif

532e4da

Fake pipeline modif

c829fd7

Remove all fake modifs

b9bc26c

Adds options to skip/force tests

076177e

[test-all-models] Fake modif for BERT

586f0c6

Try this way

2126ef7

Does the command actually work?

8d9ae28

[test-all-models] Try again!

fc094b8

[skip circleci] Remove fake modif

d82ffcd

Remove debug statements

9a91bd5

Add the list of important models

be7e290

Quality

e38b00f

sgugger requested review from amyeroberts, ydshieh and LysandreJik March 30, 2023 17:21

LysandreJik approved these changes Mar 30, 2023

View reviewed changes

sgugger and others added 2 commits March 30, 2023 15:37

Update utils/tests_fetcher.py

0819034

Co-authored-by: Lysandre Debut <[email protected]>

Address review comments

b819868

amyeroberts approved these changes Mar 31, 2023

View reviewed changes

sgugger added 2 commits March 31, 2023 09:57

Address review comments

78639d9

Fix and add test

bbac581

ydshieh reviewed Mar 31, 2023

View reviewed changes

ydshieh approved these changes Mar 31, 2023

View reviewed changes

sgugger and others added 2 commits March 31, 2023 15:58

Apply suggestions from code review

63e6bb4

Co-authored-by: Yih-Dar <[email protected]>

Address review comments

a00597a

sgugger merged commit c612628 into main Mar 31, 2023

sgugger deleted the test_fetch_v2 branch March 31, 2023 20:18

		@@ -667,11 +611,29 @@ def filter_tests(output_file, filters):
		f.write(" ".join(test_files))


		def parse_commit_message(commit_message):

		return get_diff(repo, repo.head.commit, parent_commits)


		# (:?^\|\n) -> Non-catching group for the beginning of the doc or a new line.

Test fetch v2 #22367

Test fetch v2 #22367

Uh oh!

Conversation

sgugger commented Mar 24, 2023 • edited by LysandreJik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

sgugger commented Mar 29, 2023

Uh oh!

LysandreJik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydshieh commented Mar 31, 2023

Uh oh!

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

Uh oh!

Uh oh!

ydshieh commented Mar 31, 2023

Uh oh!

amyeroberts 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!

sgugger commented Mar 31, 2023

Uh oh!

sgugger commented Mar 31, 2023

Uh oh!

ydshieh commented Mar 31, 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!

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!

HuggingFaceDocBuilderDev commented Mar 31, 2023

Uh oh!

Uh oh!

sgugger commented Mar 24, 2023 •

edited by LysandreJik

Loading

HuggingFaceDocBuilderDev commented Mar 24, 2023 •

edited

Loading

ydshieh commented Mar 29, 2023 •

edited

Loading

ydshieh commented Mar 31, 2023 •

edited

Loading

ydshieh commented Mar 31, 2023 •

edited

Loading