Trigger corresponding pipeline tests if `tests/utils/tiny_model_summary.json` is modified #27693

ydshieh · 2023-11-24T14:31:24Z

What does this PR do?

Trigger corresponding pipeline tests if tests/utils/tiny_model_summary.json is modified.

It happened several times that we merged a PR and pipeline testing failed, because the CI triggered on those PRs didn't cover all files (as tests/utils/tiny_model_summary.json is not a python file)

ydshieh · 2023-11-24T14:32:31Z

utils/tests_fetcher.py

-    # in order to trigger pipeline tests even if no code change at all
-    elif "tests/utils/tiny_model_summary.json" in modified_files:
-        test_files_to_run = ["tests"]
-        repo_utils_launch = any(f.split(os.path.sep)[0] == "utils" for f in modified_files)


this is not working as modified_files is computed by get_modified_python_files(diff_with_last_commit=diff_with_last_commit), so tiny_model_summary is never in that list.

My bad, sorry

ydshieh · 2023-11-24T14:47:19Z

tests/models/phi/test_modeling_phi.py

+    # TODO (ydshieh): Check this. See https://app.circleci.com/pipelines/github/huggingface/transformers/79292/workflows/fa2ba644-8953-44a6-8f67-ccd69ca6a476/jobs/1012905
+    def is_pipeline_test_to_skip(
+        self, pipeline_test_casse_name, config_class, model_architecture, tokenizer_name, processor_name
+    ):
+        return True


missed in the hotfix #27687

HuggingFaceDocBuilderDev · 2023-11-24T15:13:07Z

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

ArthurZucker · 2023-11-28T07:03:04Z

utils/tests_fetcher.py

+        old_keys = set(old_content.keys())
+        new_keys = set(new_content.keys())


keys are always a set

No. 'dict_keys' is not the same as set: 'dict_keys' object has no attribute 'intersection'

a = {1: 2, 2: 3} b = {2: 3, 3: 4} a.keys() b.keys() c1 = set(a.keys()).intersection(set(b.keys())) print(c1) c2 = a.keys().intersection(b.keys()) print(c2)

gives

{2} Traceback (most recent call last): File "C:\Users\33611\Desktop\Project\transformers\temp3.py", line 11, in <module> c2 = a.keys().intersection(b.keys()) AttributeError: 'dict_keys' object has no attribute 'intersection' Process finished with exit code 1

Alright! Thanks

ArthurZucker · 2023-11-28T07:03:56Z

utils/tests_fetcher.py

+        # if both have the same key, check its content
+        for key in common_keys:
+            if old_content[key] != new_content[key]:
+                diff_keys.add(key)


Mmm it's not a diff_key but diff_value

they are the keys which

either only in one set,

or in both sets, but values are different.

I can give it a different name though.

ArthurZucker · 2023-11-28T07:16:11Z

utils/tests_fetcher.py

+        # get the module where the model classes are defined. We want to use the main `__init__` file, but it requires
+        # all the framework being installed, which is not ideal for a simple script like test fetcher.
+        # So we create a temporary and modified main `__init__` and access its `_import_structure`.


why not use inspect on the class? The transformers packages should be installed and

>>> import inspect >>> from transformers import * >>> inspect.getfile(globals()["CLIPConfig"]) '/Users/arthurzucker/Work/transformers/src/transformers/models/clip/configuration_clip.py'

we can split on src and be done with it?

Using inspect will make this test_fethcer require all the dependency being installed, otherwise one will get a dummy object, also the process will become slower due to all the importing.

Mmm but it's more resilient less error prone. Agreed tho that we would need every tf flax and torch dependency which is not optimal.

more resilient less error prone

I agree too.

I don't think the community would run test fetcher. So I am fine if we decide to use inspect (on the CI). Two things to consider:

On CircleCI: the fetch test job might take longer (bigger cache to download, longer installation if cache miss)

On Push CI: the results would look a bit strange/confusing, as Flax is not installed in our CI docker image.

If I understood correctly we need dependencies anyway on the machine that will run the tests no? So what we can do is:

load the object

if it's a dummy, log that dummy object and don't use it (will help you with debugging). That means we don't have the deps on the machine

else, we have the deps, all good

If you confirm that it's ok to install transformers[dev] in the CircleCI job fetch_tests in .circleci/config.yml, then I am OK with the suggestions :-)

currently , test fetcher run in ~ 1m30.

Loading cache of the whole .[dev] will take 5 minutes + a bit more for all the importing

Fetching the test should be fast, so would just do this job last. I thought we had a different machine running the pipeline test and would say let's forward getting the files to update over there if there are any changes to the tiny model!
Cache is so slow :(

forward getting the files to update over there

If the commit(s) in a PR only modifies the tests/utils/tiny_model_summary.json, then fetch_tests in .circleci/config.yml will give an empty job. So there is no job (like torch_job / tf_job) to perform getting the files to update, and we will miss the desired pipeline tests to run.

Thanks for the offline discussion. Yes, your suggestion is a good possibility (other than the fact the file .circleci/create_circleci_config.py might get a bit more complex in order to get the actual tests to run dynamically).

As mentioned, I proposed to merge the current version, and I will work in a follow up PR to implement your suggested idea. So at the end, if there is any unexpected undesired issue, we can at least have a working version.

ArthurZucker

As discussed offline, feel free to merge as is, and work on a follow up PR for less brittle !

ydshieh added 3 commits November 24, 2023 15:27

fix

465f26c

fix

5f6d1a1

fix

f36cf78

ydshieh commented Nov 24, 2023

View reviewed changes

ydshieh requested a review from ArthurZucker November 24, 2023 14:32

ydshieh added 2 commits November 24, 2023 15:44

fix

8a68f60

fix

077a690

ydshieh commented Nov 24, 2023

View reviewed changes

fix

511c81f

ydshieh added 3 commits November 24, 2023 16:15

fix

d7f4ec3

fix

659f557

fix

99e2f0d

ydshieh marked this pull request as draft November 24, 2023 15:37

fix

90401cc

ydshieh marked this pull request as ready for review November 24, 2023 15:41

ArthurZucker reviewed Nov 28, 2023

View reviewed changes

ydshieh requested a review from ArthurZucker November 28, 2023 08:51

ArthurZucker approved these changes Nov 28, 2023

View reviewed changes

fix

b140508

ydshieh merged commit 30e92ea into main Nov 28, 2023

ydshieh deleted the trigger_ci_when_tiny_summary_modified branch November 28, 2023 16:21

		old_keys = set(old_content.keys())
		new_keys = set(new_content.keys())

Trigger corresponding pipeline tests if tests/utils/tiny_model_summary.json is modified #27693

Trigger corresponding pipeline tests if tests/utils/tiny_model_summary.json is modified #27693

Uh oh!

Conversation

ydshieh commented Nov 24, 2023

What does this PR do?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Trigger corresponding pipeline tests if `tests/utils/tiny_model_summary.json` is modified #27693

Trigger corresponding pipeline tests if `tests/utils/tiny_model_summary.json` is modified #27693

HuggingFaceDocBuilderDev commented Nov 24, 2023 •

edited

Loading

ydshieh Nov 28, 2023 •

edited

Loading

ArthurZucker Nov 28, 2023 •

edited

Loading