[github CI] add a multi-gpu job for all example tests

[stas00]

As discussed in #8315 (comment) we now have a growing set of tests that do distributed testing and require a multi-gpu setup, and which aren't being tested at the moment by any CI.

This PR adds a new job:

• run all example tests on multi-gpu

Since these are slow, I added it only to the scheduled job.

Question: Is this really needed for every step?

           source .env/bin/activate

It's in all test steps, even when there is multiple test suites run in sequence it's re-run.

Won't it be enough to do it once in the env prep step for all steps in the same job?

@sshleifer, @LysandreJik

[LysandreJik]

This looks good, but as it is right now it would run all the tests in the example folder, many of which are failing. Is the goal to identify which tests are failing in a multi-GPU setup?

Regarding your question concerning the source .env/bin/activate statement, it is actually necessary to do it at all steps. It got me by surprise in the slow TF test suite where I had forgotten it and it used the system interpreter instead.

[stas00]

Ah, thank you for clarifying the state of things, @LysandreJik

Some possible solutions I can think of:

1. fix the situation quickly by adding @require_torch_non_multigpu to any such failing tests and make a plan to port those.
2. rename all good tests to include _multigpu and run the test suite with -k multigpu - but we already have one test that can run on either 1 gpu or many so it'd exclude it despite it being just fine (which makes it is the least favorite suggestion)
3. create a file with a list of passing tests and feed it to pytest $(cat examples/passing_multigpu_tests.txt), which we maintain as tests get ported and eventually remove it.

I think solution (3) is the most sensible to me. (1) will potentially sweep things under the carpet. (2) isn't great because it doesn't cover all bases.

I went ahead and created examples/ported-multigpu-tests.txt and changed the new job to run:

python -m pytest -n 1 --dist=loadfile -s --make-reports=examples_torch_multiple_gpu $(tr '\n' ' ' < examples/ported-multigpu-tests.txt)

I used a little tr to make ported-multigpu-tests.txt easy to maintain:

• one entry per line - or multiple if desired
• it can take whole test files or specific tests

We also have one distributed test under tests - but it automatically works with either number of gpus.

Moreover, we can actually run tests from both tests suites at once - i.e. in the same pytest session.

[stas00]

Regarding your question concerning the source .env/bin/activate statement, it is actually necessary to do it at all steps. It got me by surprise in the slow TF test suite where I had forgotten it and it used the system interpreter instead.

Thank you for clarifying that! I took the liberty to add a note in .github/workflows/self-scheduled.yml to why this is done, to save time in the future others asking the same question.

Perhaps there is a way to make it persistent across steps. A quick look landed: https://github.com/actions/virtual-environments - that is you need to use their venv and then it's persistent across steps I think.

I wish the CIs were using the same API, as currently each one re-invents the wheel. We need huggingface ci interface like transformers that supports any major CI using the same API ;)

[stas00]

Another small nit I'd like to suggest: the ci yml files use multiple_gpu and multi_gpu, tests use multigpu in one word - let's pick one of them and use it consistently?

[LysandreJik]

I would definitely be in favor of adding the require_torch_non_multigpu decorator to these tests, as much as I would be in favor of adding a require_torch to all current examples that require torch (which is all of them, I believe). Right now we can't run the TF CI on examples because it fails on all examples, and while we have no immediate need to do so as there are no examples to test in TF, this is bound to change.

I think the absolute best approach would be to make those tests robust to multi-gpu, as most scripts can handle it but the tests are not designed that way. However, that would be a considerable time investiment for limited return, so I'm fine with the decorator option. I don't feel like that would be to sweep things under the carpet, so I may have misunderstood what you meant, could you elaborate on that?

Ah, I didn't know that you could leverage the GA virtual environment to keep it persistent. I currently don't have the bandwidth to look into it, but if you do, please do! Otherwise we can do this once things settle down.

I have no opinion regarding multigpu vs multi_gpu, but consistency would be most welcome. Maybe multigpu is a bit harder to read, so let's go the multi_gpu route if you agree on that?

[stas00]

I would definitely be in favor of adding the require_torch_non_multigpu decorator to these tests, as much as I would be in favor of adding a require_torch to all current examples that require torch (which is all of them, I believe).

Excellent!

A possible next action:

1. add require_torch to all current examples tests
2. add require_torch_non_multigpu to unported/untested under multi-gpu examples tests
3. make the multigpu CI job run on examples w/o specifying test names

I think the absolute best approach would be to make those tests robust to multi-gpu, as most scripts can handle it but the tests are not designed that way. However, that would be a considerable time investiment for limited return, so I'm fine with the decorator option. I don't feel like that would be to sweep things under the carpet, so I may have misunderstood what you meant, could you elaborate on that?

When I used the idiom "sweeping under the carpet", my concern with require_torch_non_multigpu was that it was actually added to decorate tests that by design shouldn't run on multi-gpu, but here we use it as a band-aid, and then we won't be able to quickly tell which tests are flagged so because they are in line to be upgraded, and which are meant to not run on multigpu (can't be upgraded or if for whatever reason it needs to run on a single gpu). I hope this clarifies my concern.

How about we add a new decorator require_torch_non_multigpu_but_fix_me or something like that, so it's then clear this one needs to be upgraded. If I remember correctly we have a couple require_torch_non_multigpu that are meant to stay so. It'd just be defined as:

require_torch_non_multigpu_but_fix_me = require_torch_non_multigpu

so it's just a visual clue for us.

Or it's possible that I'm overreacting and we somehow will keep track of what was ported and what not.

Ah, I didn't know that you could leverage the GA virtual environment to keep it persistent. I currently don't have the bandwidth to look into it, but if you do, please do! Otherwise we can do this once things settle down.

I'm not 100% sure it is persistent as I haven't used it on GA yet. But one would think so. The current solution works for me.

I have no opinion regarding multigpu vs multi_gpu, but consistency would be most welcome. Maybe multigpu is a bit harder to read, so let's go the multi_gpu route if you agree on that?

multi_gpu is perfect with me. Thank you!

[stas00]

I went ahead and added @require_torch_non_multigpu to unported/untested under multi-gpu examples tests.

find examples -name "test_*" -exec perl -pi -e 's|^    def test_|    \@require_torch_non_multigpu_but_fix_me\n    def test_|' {} \;
find examples -name "test_*" -exec perl -pi -e 's|from transformers.testing_utils import |from transformers.testing_utils import require_torch_non_multigpu_but_fix_me, |' {} \;
make fixup

I cleaned up one test file that doesn't really use gpu and removed the new decorator from there. Plus removed it from all the tests that have been ported.

When the porting is done we can remove this decorator altogether.

On a multi-gpu machine we now have:

pytest examples
====================================================================== test session starts ======================================================================
platform linux -- Python 3.8.5, pytest-6.1.2, py-1.9.0, pluggy-0.13.1
rootdir: /mnt/nvme1/code/huggingface/transformers-examples-multigpu-ci
plugins: hydra-core-1.0.3, forked-1.3.0, xdist-2.1.0, instafail-0.4.2
collected 82 items

examples/test_examples.py ssssss                                                                                                                          [  7%]
examples/bert-loses-patience/test_run_glue_with_pabee.py s                                                                                                [  8%]
examples/deebert/test_glue_deebert.py s                                                                                                                   [  9%]
examples/rag/test_distributed_retriever.py sss                                                                                                            [ 13%]
examples/seq2seq/test_bash_script.py sss                                                                                                                  [ 17%]
examples/seq2seq/test_calculate_rouge.py ......                                                                                                           [ 24%]
examples/seq2seq/test_datasets.py ssssssssssssssss                                                                                                        [ 43%]
examples/seq2seq/test_finetune_trainer.py s.s                                                                                                             [ 47%]
examples/seq2seq/test_fsmt_bleu_score.py ssss                                                                                                             [ 52%]
examples/seq2seq/test_make_student.py sssss                                                                                                               [ 58%]
examples/seq2seq/test_seq2seq_examples.py ssssssssssssssssss                                                                                              [ 80%]
examples/seq2seq/test_seq2seq_examples_multi_gpu.py s.                                                                                                    [ 82%]
examples/seq2seq/test_tatoeba_conversion.py ss                                                                                                            [ 85%]
examples/seq2seq/bertabs/test_utils_summarization.py ..........                                                                                           [ 97%]
examples/token-classification/test_ner_examples.py ss                                                                                                     [100%]

========================================================== 18 passed, 64 skipped, 2 warnings in 32.04s =============

We can add a new task to go over tests with @require_torch_non_multigpu - review those and either port them or remove the decorator, since no porting is needed there. And we can now run examples on multigpu.

[sshleifer]

Suggested change

	- name: Run all tests on multi-GPU
	- name: Run tests/ on multi-GPU

[stas00]

But it is RUN_SLOW=1 - so everywhere else in config it says "all tests" for this env var.

[sshleifer]

this breaks if multigpu?

should the decorator be called require_torch_single_gpu?

[stas00]

We don't know, that's why it has "but_fix_me" - I just added it to all tests in examples, and now we need to go over all of them, review and either make each test work under multi-gpu or designate it as non_multi_gpu.

wrt naming let's discuss it in normal comments, since once resolved these get hidden.

[sshleifer]

what does ported mean in this comment?
Converted to support multi-gpu systems?

[stas00]

what does ported mean in this comment?

In my mind it means that a developer looked at the test and decided how it's supposed to operate and acted on it. It is either just fine as is - remove the fixme decorator, it's not fine - recode it to be fine and remove the fixme decorator.

Perhaps call it "_review_me" will be less ambiguous?

I originally started with the approach of explicitly listing tests that have been ported to multi-gpu.
@LysandreJik suggested we run all tests but those unported will get skipped to start with, hence this solution.

[stas00]

So let's discuss how we designate the various single/multiple/flexible gpu decorators:

Let's focus just on pytorch tests for now

We have tests that require:

• >= 0 gpus: @require_torch
• >= 1 gpus: @require_torch_gpu
• >= 2 gpus: @require_torch_multigpu
• < 2 gpu: @require_torch_non_multigpu
• == 1 gpu only: we don't have such decorator yet, so perhaps we need to add require_torch_single_gpu as you suggested?
• == 0 gpu only: we don't have such decorator yet, so perhaps we need to add require_torch_non_gpu?

Please check that I didn't miss any edge cases. The last one I'm not sure if we have such tests - probably not.

The second to last case (== 1 gpu only) is slightly ambiguous - should we not run the test if there is more than 1 gpu? or should the test be smart and set the env so that only 1 gpu is seen? it's easy to do if it's a n_gpu arg, so it can just force it to n_gpu=1 - it'd be trickier if the tested system derives this from the env - then we have to tweak CUDA_VISIBLE_DEVICES. we have such test already - it just sets n_gpu=1 if there is >= 1 - so should such test run on multi-gpu machine or not?

plus we want a distributed test that runs on a single gpu as well - @sshleifer wanted to see that it gives better results than non-distributed - so distributed shouldn't automatically mean multigpu either.

Any other nuances?

[LysandreJik]

I'm not sure we have a need for the last two tests. If a test isn't working on a GPU even though it works on CPU, this is a bug. If a test isn't working on CPU even though it works on a GPU, that is a bug as well. Both of these bugs must be solved ASAP.

I think the other four decorators are enough, and cover the whole test suite. What do you think?

[stas00]

If a test isn't working on CPU even though it works on a GPU, that is a bug as well. Both of these bugs must be solved ASAP.

Your comment then addresses @sshleifer question:

should the decorator be called require_torch_single_gpu?

So you currently can't think of such situation, since it'd have to work on cpu too.

---

I was thinking that perhaps there will be a test checking some feature that is only available on gpu or perhaps it's too slow to run on cpu, yet doesn't require slow on gpu...

I guess if we encounter such situation we can add a new decorator.

[LysandreJik]

Ok this looks good to me. It would be nice to see if some of those can run on multi-GPU and enable them then, but this can be done in another PR.

Thanks @stas00!

[stas00]

So the next stage could be:

1. to make an issue that tracks a list of tests that need to be ported to multigpu
2. invite those in charge of the different sections of examples to review the tests that they maintain, remove this fix_me decorator if it's not needed and otherwise add it to the issue in Create DataParallel model if several GPUs #1 (or perhaps list all the available tests right away and check off what still needs to be worked on)
3. port the tests that need to be ported
   • mechanically it is just a few lines of code to add and update the issue - but it's the best done by those who wrote/maintain the tests since they know the nuances and will see that the tests are still valid
   • the main issue I noticed with porting to distributed is that the tests then need either more data or more iterations or a different learning rate. And to support 3+ gpus it really has to have some kind of multiplier so as n_gpus grows the number of data needs to grow proportionally.

or we can just leave it as is and port as devs choose to/find need to...