Move test model folders #17034

ydshieh · 2022-05-02T08:00:41Z

What does this PR do?

As discussed offline, this PR moves model specific test folders (e.g. tests/bert) to tests/models (e.g. tests/models/bert)

In addition to the necessary changes on import, the following changes are made:

In some test files regarding processors (tokenizer/feature extractor, etc.), change

SAMPLE_ROBERTA_CONFIG = os.path.join(os.path.dirname(os.path.abspath(__file__)), ".../fixtures/dummy-config.json")

to

SAMPLE_ROBERTA_CONFIG = get_tests_dir("fixtures/dummy-config.json")

(see the commit)

The changes (to be reviewed particularly)
- .circleci/config.yml
- .github/workflows/self-scheduled.yml
- src/transformers/commands/add_new_model.py
- src/transformers/commands/add_new_model_like.py
- utils/check_repo.py
- utils/notification_service.py
- utils/test_fetcher.py

Remarks:

The self-push result is here
- The slack report job has Artifact was not found, job was probably canceled., but this issue exists for some time. My plan is to continue the task of changing self-push report format (and fix this issue)
- The run_tests_flax_gpu failure is just the same as in other runs. This is not in the scope of this PR.
The scheduled CI (partial) result is here. The report is available on Slack.
- On the GitHub Actions page, the jobs have name like Model tests (models/albert, single-gpu-docker). It becomes a bit long (with models/).
- Same for the Slack report
```
0 |         0 |         3 |         0 |         0 | models_auto
```
- So far I only ran a subset of models. From the results, I think the PR is ready. We can run a full suite of tests before merge.

HuggingFaceDocBuilderDev · 2022-05-02T08:15:24Z

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

sgugger

Impressive work! My only concern is on the changes in self.scheduled.yml, so I'd really like to launch that CI once and check everything is okay before we merge this.

LysandreJik

Very impressive PR, thanks @ydshieh!

If you're rebasing on main to take care of the conflicts, I would advocate to squash all of your commits in a single one, which means you'll only take care of the conflicts once (similarly to a merge). Having 32 commits to rebase on main must be hard :)

stas00

Super awesome work, @ydshieh! Thank you!

I made a few minor optional suggestions if these resonate with you. But please feel free to ignore.

I wonder if this

transformers/src/transformers/testing_utils.py

Lines 960 to 963 in 1073f00

    
           for up in [1, 2, 3]: 
        
               tmp_dir = path.parents[up] 
        
               if (tmp_dir / "src").is_dir() and (tmp_dir / "tests").is_dir(): 
        
                   break

needs to have another level, or perhaps it's still reaching to the repo's root. I think I coded it originally to have some spare levels.

stas00 · 2022-05-02T19:42:40Z

.github/workflows/self-scheduled.yml

          rm -rf reports

      - id: set-matrix
        name: Identify models to test
        working-directory: /transformers/tests
        run: |
-          echo "::set-output name=matrix::$(python3 -c 'import os; x = list(filter(os.path.isdir, os.listdir(os.getcwd()))); x.sort(); print(x)')"
+          echo "::set-output name=matrix::$(python3 -c 'import os; tests = os.getcwd(); model_tests = os.listdir(os.path.join(tests, "models")); d1 = sorted(list(filter(os.path.isdir, os.listdir(tests)))); d2 = sorted(list(filter(os.path.isdir, [f"models/{x}" for x in model_tests]))); d1.remove("models"); d = d2 + d1; print(d)')"


this is a bit difficult to read, may I propose a simplified version which I hope is easier to follow, plus it adds some new lines and additionally removes __pycache__ dir which shouldn't be in the results.

Suggested change

echo "::set-output name=matrix::$(python3 -c 'import os; tests = os.getcwd(); model_tests = os.listdir(os.path.join(tests, "models")); d1 = sorted(list(filter(os.path.isdir, os.listdir(tests)))); d2 = sorted(list(filter(os.path.isdir, [f"models/{x}" for x in model_tests]))); d1.remove("models"); d = d2 + d1; print(d)')"

echo "::set-output name=matrix::$(python3 -c '\

import os; \

tests = os.getcwd(); \

d = set(filter(os.path.isdir, os.listdir(tests) + [f"models/{x}" for x in os.listdir(f"{tests}/models")])); \

d -= set(["models", "__pycache__"]); \

print(sorted(d))')"

Here is just the python snippet:

python -c '\ import os; \ tests = os.getcwd(); \ d = set(filter(os.path.isdir, os.listdir(tests) + [f"models/{x}" for x in os.listdir(f"{tests}/models")])); \ d -= set(["models", "__pycache__"]); \ print(sorted(d))'

which I tested with, I hope I plugged it in correctly above - i.e. untested.

stas00 · 2022-05-02T19:58:03Z

tests/models/auto/test_configuration_auto.py



-sys.path.append(str(Path(__file__).parent.parent.parent / "utils"))
+sys.path.append(str(Path(__file__).parent.parent.parent.parent / "utils"))


if the number of parent calls is becoming too long, one could do:

Suggested change

sys.path.append(str(Path(__file__).parent.parent.parent.parent / "utils"))

sys.path.append(str(Path(__file__).parents[4] / "utils"))

but it's totally ok like it is right now.

ydshieh · 2022-05-03T08:44:21Z

Hi, @stas00 Thank you for the feedbacks.

Regarding difficult to read (the long command): totally agreed! I had the same feeling, and thought might be a good idea to create a tiny python script and just call it. Otherwise, we can use what you proposed above (after some tests).
About parents[4]: thank you for the information!
I can check TestCasePlus later.

I would prefer to merge as it is now, and work on these points in another PR. The main reason is that I ran the full suite of tests, the results look all good, and would like to merge with a version that has been fully tested :-)

# Conflicts: # .github/workflows/add-model-like.yml # .github/workflows/model-templates.yml

ydshieh · 2022-05-03T12:36:24Z

Merged now (after rebase on main for the merged flax_bert and yolos PRs).

* move test model folders (TODO: fix imports and others) * fix (potentially partially) imports (in model test modules) * fix (potentially partially) imports (in tokenization test modules) * fix (potentially partially) imports (in feature extraction test modules) * fix import utils.test_modeling_tf_core * fix path ../fixtures/ * fix imports about generation.test_generation_flax_utils * fix more imports * fix fixture path * fix get_test_dir * update module_to_test_file * fix get_tests_dir from wrong transformers.utils * update config.yml (CircleCI) * fix style * remove missing imports * update new model script * update check_repo * update SPECIAL_MODULE_TO_TEST_MAP * fix style * add __init__ * update self-scheduled * fix add_new_model scripts * check one way to get location back * python setup.py build install * fix import in test auto * update self-scheduled.yml * update slack notification script * Add comments about artifact names * fix for yolos Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

ydshieh requested review from LysandreJik, sgugger and stas00 May 2, 2022 10:13

sgugger approved these changes May 2, 2022

View reviewed changes

ydshieh force-pushed the move_test_model_folders branch from e4efa64 to 1080be1 Compare May 2, 2022 15:38

LysandreJik approved these changes May 2, 2022

View reviewed changes

stas00 approved these changes May 2, 2022

View reviewed changes

ydshieh added 21 commits May 3, 2022 11:43

test - to be revert

babdc23

temp change to generate new cache - to be reverted

8ff0bde

temp change to generate new cache - to be reverted

d09b62b

clean-up

75ff03c

move test model folders (TODO: fix imports and others)

ffbcad9

fix (potentially partially) imports (in model test modules)

7851bdb

fix (potentially partially) imports (in tokenization test modules)

fed0b1d

fix (potentially partially) imports (in feature extraction test modules)

c439b16

fix import utils.test_modeling_tf_core

f35f1c5

fix path ../fixtures/

2d801d2

fix imports about generation.test_generation_flax_utils

7d1d1a5

fix more imports

ab59261

fix fixture path

ae4827b

fix get_test_dir

faba7b6

update module_to_test_file

8c9600e

fix get_tests_dir from wrong transformers.utils

732ca46

update config.yml (CircleCI)

16e269e

fix style

6b52d1f

remove missing imports

e971915

update new model script

4b0320e

update check_repo

8b470d0

ydshieh added 12 commits May 3, 2022 11:44

update SPECIAL_MODULE_TO_TEST_MAP

3dad271

fix style

790f0d9

add __init__

c31b9c9

update self-scheduled

39966aa

fix add_new_model scripts

c90b35c

check one way to get location back

579cdbf

python setup.py build install

fa9cfd4

# Conflicts: # .github/workflows/add-model-like.yml # .github/workflows/model-templates.yml

fix import in test auto

021ae85

update self-scheduled.yml

825581f

update slack notification script

b83abd8

Add comments about artifact names

0e44f67

fix for yolos

9bc8e1a

ydshieh force-pushed the move_test_model_folders branch from 1080be1 to 9bc8e1a Compare May 3, 2022 09:47

ydshieh merged commit 19420fd into main May 3, 2022

ydshieh deleted the move_test_model_folders branch May 3, 2022 12:42

ydshieh mentioned this pull request May 4, 2022

Fix missing "models" in pipeline test module #17090

Merged

SaulLu mentioned this pull request May 5, 2022

Add missing RetriBERT tokenizer tests #17017

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move test model folders #17034

Move test model folders #17034

ydshieh commented May 2, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented May 2, 2022 •

edited

Loading

sgugger left a comment

LysandreJik left a comment

stas00 left a comment

stas00 May 2, 2022

stas00 May 2, 2022

ydshieh commented May 3, 2022 •

edited

Loading

ydshieh commented May 3, 2022

	for up in [1, 2, 3]:
	tmp_dir = path.parents[up]
	if (tmp_dir / "src").is_dir() and (tmp_dir / "tests").is_dir():
	break

-          echo "::set-output name=matrix::$(python3 -c 'import os; tests = os.getcwd(); model_tests = os.listdir(os.path.join(tests, "models")); d1 = sorted(list(filter(os.path.isdir, os.listdir(tests)))); d2 = sorted(list(filter(os.path.isdir, [f"models/{x}" for x in model_tests]))); d1.remove("models"); d = d2 + d1; print(d)')"
+          echo "::set-output name=matrix::$(python3 -c '\
+import os; \
+tests = os.getcwd(); \
+d = set(filter(os.path.isdir, os.listdir(tests) + [f"models/{x}" for x in os.listdir(f"{tests}/models")])); \
+d -= set(["models", "__pycache__"]); \
+print(sorted(d))')"



		sys.path.append(str(Path(__file__).parent.parent.parent / "utils"))
		sys.path.append(str(Path(__file__).parent.parent.parent.parent / "utils"))

Move test model folders #17034

Move test model folders #17034

Conversation

ydshieh commented May 2, 2022 • edited Loading

What does this PR do?

Remarks:

HuggingFaceDocBuilderDev commented May 2, 2022 • edited Loading

sgugger left a comment

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

stas00 left a comment

Choose a reason for hiding this comment

stas00 May 2, 2022

Choose a reason for hiding this comment

stas00 May 2, 2022

Choose a reason for hiding this comment

ydshieh commented May 3, 2022 • edited Loading

ydshieh commented May 3, 2022

ydshieh commented May 2, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented May 2, 2022 •

edited

Loading

ydshieh commented May 3, 2022 •

edited

Loading