extend the test files #23043

ydshieh · 2023-04-28T08:07:31Z

What does this PR do?

Extend the test files in cross test CI job.

In #21023, flax bigbird modeling file is changed and the flax test file skips the pt/flax tests. However, the test fetcher is not designed to take into account the corresponding pytorch test file, and we have test failure in main on nightly run.

This PR extends the test files for torch_and_tf and torch_and_flax jobs to avoid such situation.

The effect could be seen in this run

https://app.circleci.com/pipelines/github/huggingface/transformers/63246/workflows/84722f3a-1259-4226-973d-267c74ca9aee/jobs/780372

HuggingFaceDocBuilderDev · 2023-04-28T08:26:02Z

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

ydshieh · 2023-04-28T11:37:13Z

I need to check if the new test files exist before adding them to the list. Will update later

sgugger

I have already given my thoughts a thousand times on how such failures should be fixed, but since I'm being ignored again...

This is not my preferred solution, but if we go for this, I'd like the added tests to be logged somewhere, so we can inspect the results. Otherwise we can't debug if there is a failure of the test fetcher, as the generated config is not exactly readable.

ydshieh · 2023-04-28T15:24:13Z

I have already given my thoughts a thousand times on how such failures should be fixed, but since I'm being ignored again...

@sgugger I don't mean to ignore your suggestion, but the only discussion I remembered is this slack discussion, where you mentioned (to my previous messages) with

Mmmm, maybe there is a post-processing function that could do that yes. (Note that the whole thing for the pipeline will disappear with the new test fecther and the new way pipeline tests are designed).

Therefore I assume the approach in this PR (current version) is fine.

I might forget anything you mentioned earlier somewhere else. In this case, please share the link, and I am happy to take a look your suggestion. Otherwise, feel free to drop what you think the best. Thank you.

This is not my preferred solution, but if we go for this, I'd like the added tests to be logged somewhere, so we can inspect the results. Otherwise we can't debug if there is a failure of the test fetcher, as the generated config is not exactly readable.

If we keep this approach, I am happy to save files of these modified version.

ydshieh · 2023-04-28T15:31:14Z

Note that I am not really in favor of duplicating the tests. We have these tests both in PT/TF or PT/Flax test files. It's already kind of duplication. And if we copy each version to another framework again, that is duplication of duplication.

sgugger · 2023-04-28T15:35:09Z

Let's go with logging the modified test files somehow, so we can inspect the result of the code you add then.

ydshieh · 2023-04-28T16:17:00Z

src/transformers/models/big_bird/modeling_flax_big_bird.py

@@ -45,7 +45,7 @@
 )
 from ...utils import ModelOutput, add_start_docstrings, add_start_docstrings_to_model_forward, logging
 from .configuration_big_bird import BigBirdConfig
-
+a = 1


will revert before merge: just to trigger test to see everything is expected

sgugger · 2023-04-28T17:00:00Z

Looks like it didn't touch any new file though.

ydshieh · 2023-04-28T17:15:17Z

Looks like it didn't touch any new file though.

Yeah, I am bad at getting the correct path. Now it works (if I triggered the test via a change in a file), see

https://app.circleci.com/pipelines/github/huggingface/transformers/63288/workflows/721ab3d6-b1af-4f2d-b036-9bebbdaef2cc/jobs/780955

sgugger

Thanks! One last ask to make it easier to navigate and we should be good to go.

sgugger · 2023-04-28T18:09:46Z

.circleci/config.yml

+            - run: |
+                  if [ -f filtered_test_list_torch_and_tf.txt ]; then
+                      mv filtered_test_list_torch_and_tf.txt test_preparation/filtered_test_list_torch_and_tf.txt
+                  else
+                      touch test_preparation/filtered_test_list_torch_and_tf.txt
+                  fi
+            - run: |
+                  if [ -f filtered_test_list_torch_and_flax.txt ]; then
+                      mv filtered_test_list_torch_and_flax.txt test_preparation/filtered_test_list_torch_and_flax.txt
+                  else
+                      touch test_preparation/filtered_test_list_torch_and_flax.txt
+                  fi


Let's put everything in one report for cross tests (named filtered_crossed_test_list for instance)

sgugger · 2023-04-28T18:10:18Z

.circleci/create_circleci_config.py

+                with open(f_path, "w") as fp:
+                    fp.write(" ".join(job.tests_to_run))


You can append to f_path in your for loop with a title being the job name so that everything ends up in the same file.

ydshieh · 2023-04-28T19:58:14Z

@sgugger Could you check the last commit and see if it is OK? ~~(I will run a test to make sure everything works as expected tomorrow before merge, already tired today with some personal meetings)~~.

See the new run page and here

One thing to note is that, I put everything regarding cross tests into a single file as well as in the cross test jobs. So test_modeling_tf_xxx.py might be in the tests_torch_and_flax job and vice versa in some cases. It doesn't really matter as we correctly install the only necessary libraries in the job.

ydshieh · 2023-04-28T20:02:07Z

.circleci/create_circleci_config.py

+        extended_tests_to_run = sorted(extended_tests_to_run)
+        for job in jobs:
+            if job.job_name in ["tests_torch_and_tf", "tests_torch_and_flax"]:
+                job.tests_to_run = extended_tests_to_run


one single list for all cross test jobs

sgugger

Thanks!

* fix --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

ydshieh force-pushed the extend_test_files branch from 67cafdb to 46934d2 Compare April 28, 2023 09:58

ydshieh requested a review from sgugger April 28, 2023 10:01

ydshieh marked this pull request as ready for review April 28, 2023 10:01

sgugger reviewed Apr 28, 2023

View reviewed changes

ydshieh commented Apr 28, 2023

View reviewed changes

sgugger approved these changes Apr 28, 2023

View reviewed changes

fix

d666101

ydshieh force-pushed the extend_test_files branch from c4f1227 to d666101 Compare April 28, 2023 17:09

sgugger reviewed Apr 28, 2023

View reviewed changes

fix

5421551

fix

35e691f

ydshieh commented Apr 28, 2023

View reviewed changes

sgugger approved these changes Apr 28, 2023

View reviewed changes

ydshieh merged commit dfeb5aa into main Apr 28, 2023

ydshieh deleted the extend_test_files branch April 28, 2023 20:25

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

extend the test files (huggingface#23043)

194b8e5

* fix --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

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

extend the test files (huggingface#23043)

15b6538

* fix --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extend the test files #23043

extend the test files #23043

ydshieh commented Apr 28, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Apr 28, 2023 •

edited

Loading

ydshieh commented Apr 28, 2023

sgugger left a comment

ydshieh commented Apr 28, 2023 •

edited

Loading

ydshieh commented Apr 28, 2023

sgugger commented Apr 28, 2023

ydshieh Apr 28, 2023

sgugger commented Apr 28, 2023

ydshieh commented Apr 28, 2023 •

edited

Loading

sgugger left a comment

sgugger Apr 28, 2023

sgugger Apr 28, 2023

ydshieh commented Apr 28, 2023 •

edited

Loading

ydshieh Apr 28, 2023

sgugger left a comment

		with open(f_path, "w") as fp:
		fp.write(" ".join(job.tests_to_run))

extend the test files #23043

extend the test files #23043

Conversation

ydshieh commented Apr 28, 2023 • edited Loading

What does this PR do?

HuggingFaceDocBuilderDev commented Apr 28, 2023 • edited Loading

ydshieh commented Apr 28, 2023

sgugger left a comment

Choose a reason for hiding this comment

ydshieh commented Apr 28, 2023 • edited Loading

ydshieh commented Apr 28, 2023

sgugger commented Apr 28, 2023

ydshieh Apr 28, 2023

Choose a reason for hiding this comment

sgugger commented Apr 28, 2023

ydshieh commented Apr 28, 2023 • edited Loading

sgugger left a comment

Choose a reason for hiding this comment

sgugger Apr 28, 2023

Choose a reason for hiding this comment

sgugger Apr 28, 2023

Choose a reason for hiding this comment

ydshieh commented Apr 28, 2023 • edited Loading

ydshieh Apr 28, 2023

Choose a reason for hiding this comment

sgugger left a comment

Choose a reason for hiding this comment

ydshieh commented Apr 28, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Apr 28, 2023 •

edited

Loading

ydshieh commented Apr 28, 2023 •

edited

Loading

ydshieh commented Apr 28, 2023 •

edited

Loading

ydshieh commented Apr 28, 2023 •

edited

Loading