Allow tests in examples to use cuda or fp16,if they are available #5512

Joel-hanson · 2020-07-04T06:08:33Z

The tests in examples didn't use the Cuda or fp16 even if they were available.

The text classification example (run_glue.py) didn't use the fp16 even if it was available but
the device was take based on the availability(Cuda/CPU).
The language-modeling example (run_language_modeling.py) was having --no_cuda argument which
made the test work without Cuda. This example is having an issue when running with fp16
thus it not enabled (got an assertion error for perplexity due to its higher value).
The Cuda and fp16 is not enabled for question-answering example (run_squad.py) as it is having a
the difference in the f1 score.
The text-generation example (run_generation.py) will take the Cuda or fp16 whenever it is available.

Resolves some of #5057

The tests in examples didn't use the cuda or fp16 even if they where available. - The text classification example (`run_glue.py`) didn't use the fp16 even if it was available but the device was take based on the availablity(cuda/cpu). - The language-modeling example (`run_language_modeling.py`) was having `--no_cuda` argument which made the test to work without cuda. This example is having issue when running with fp16 thus it not enabled (got an assertion error for perplexity due to it higher value). - The cuda and fp16 is not enabled for question-answering example (`run_squad.py`) as it is having a difference in the f1 score. - The text-generation example (`run_generation.py`) will take the cuda or fp16 whenever it is available. Resolves some of: huggingface#5057

Joel-hanson · 2020-07-04T06:52:24Z

@sshleifer Hope you are doing well. sorry for the delay, I have created the PR for some of the related issues which were mentioned in #5057.

They never use Cuda or fp16, even if they are available.

I have some doubts which had encountered when making this PR

1. As you had said there where some test which failed when enabling the Cuda or fp16

The Cuda and fp16 are not enabled for question-answering example (run_squad.py) as it is having a difference in the f1 score.
The language-modeling example (run_language_modeling.py) is having an issue when running with fp16.

2. I was not able to find the test_hans.py but was able to find a readme to run it. Is this intentional if not shall I have a test_hans.py file to run the same.

3. This is the list of tests which I got

$ ls -l examples/**/test*.py

examples/bert-loses-patience/test_run_glue_with_pabee.py
examples/seq2seq/bertabs/test_utils_summarization.py
examples/seq2seq/test_seq2seq_examples.py
examples/test_examples.py
examples/token-classification/test_ner_examples.py

I was not able to find the test_summarization_examples.py and test_t5_examples.py. I think I am doing something wrong.

4. I can have made the PR for some tests only, can do the same for others if the current PR satisfies your requirement.

sshleifer · 2020-07-05T18:55:56Z

noted
2)test_hans.py would be nice, but can be added in a separate PR.
You are all set, nothing is wrong.
Would love to see the current PR!

Thanks!

Joel-hanson · 2020-07-18T09:51:16Z

2)test_hans.py would be nice, but can be added in a separate PR.

The test_hans.py was created once via the PR #2239 but I think somehow or for some reason, it was removed

last interaction with the file was in the PR #4213

sshleifer

Did you also add multigpu support? If so you need to test it on a machine/show that it works, and it should probably be a separate PR.

sshleifer · 2020-07-19T02:52:12Z

examples/text-classification/run_glue.py

@@ -85,6 +86,9 @@ def main():
            f"Output directory ({training_args.output_dir}) already exists and is not empty. Use --overwrite_output_dir to overcome."
        )

+    if "cuda" not in str(training_args.device) or not is_apex_available():


Can we just change the test code? Or is this fixing a real bug?

@sshleifer I wasn't sure whether this is a good solution but I have added this for certain reasons.

For the apex to work we need to have cuda. So when was implementing this I had a scenario where the apex was installed but the GPU is not enabled thus I added both the checks. This checks for the cuda first before checking if apex is available or not.

I wasn't sure if the training_args.device will give cuda or cuda:0 when there are multiple GPUs thus I have added a check to know if the training_args.device has a cuda.

Did you also add multi GPU support? If so you need to test it on a machine/show that it works, and it should probably be a separate PR.

I didn't find a way to test this on multiple GPU. I will try my best to find a way to do this. But in examples/text-generation/run_generation.py I have added local_rank because I need to make every test file to have consistent arguments. This doesn't mean it adds a multi GPU support, I can remove this as it is confusing.

I am pretty sure this might be a wrong implementation please correct me if any of the above-mentioned points are not correct and if there is any other best alternative to do this.

Lets check if cuda is available in test_examples.py, and if it is pass the fp16 argument.
Otherwise, don't pass the fp16 argument.

sshleifer · 2020-07-19T02:55:37Z

also I missed this earlier, but what was the issue with run_language_modeling.py? Can you get a traceback?

Joel-hanson · 2020-07-21T17:48:35Z

also I missed this earlier, but what was the issue with run_language_modeling.py? Can you get a traceback?

This is the short long and I have attached the pull log with this comment.

        with patch.object(sys, "argv", testargs):
            result = run_language_modeling.main()
>           self.assertLess(result["perplexity"], 35)
E           AssertionError: 36.684365356893885 not less than 35

test.log

Joel-hanson · 2020-07-31T08:47:44Z

@sshleifer Hope you are doing well.

Is this PR ok or Should I split this into multiple smaller PRs to make things easier?

sshleifer

We should try to avoid changes to run_generation.py and run_glue.py (besides new command line args) and decide whether command line arguments are appropriate to send from text_examples.py

…da and apex is avaliable - run_glue.py: Removed the check for cuda and fp16. - run_generation.py: Removed the check for cuda and fp16 also removed unwanted flag creation.

…examples

Joel-hanson · 2020-08-08T12:38:09Z

Thanks for the review @sshleifer, I have updated the code accordingly and regarding the new command line args, I have removed the args which were related to the multi-GPU support as we can make that a separate PR.

Any Idea why the test run_tests_tf and run_tests_torch_and_tf failing? Is it related to my change?

sshleifer

one comment, thanks for iterating!

sshleifer · 2020-08-09T02:06:11Z

examples/text-generation/run_generation.py

    args = parser.parse_args()

    args.device = torch.device("cuda" if torch.cuda.is_available() and not args.no_cuda else "cpu")
    args.n_gpu = 0 if args.no_cuda else torch.cuda.device_count()

+    logger.warning(


on line 213 you need to add code to actually use args.fp16

if args.fp16: model.half()

@sshleifer I have made this change in run_generation.py, But I have a doubt run_glue.py should also have this change right?
But when I tried I got an error with a message as follows:
msg = 'Found param distilbert.embeddings.word_embeddings.weight with type torch.cuda.HalfTensor, expected torch.cuda.FloatTe...alize, you do not need to call .half() on your model\nbefore passing it, no matter what optimization level you choose.'

you did the right thing!

…into improve-tests-in-examples

sshleifer

LGTM, thanks for the contribution!

sshleifer · 2020-08-11T20:42:38Z

examples/text-generation/run_generation.py

    args = parser.parse_args()

    args.device = torch.device("cuda" if torch.cuda.is_available() and not args.no_cuda else "cpu")
    args.n_gpu = 0 if args.no_cuda else torch.cuda.device_count()

+    logger.warning(


you did the right thing!

LysandreJik

Great, thanks for making this change, very useful! I left a few comments below.

LysandreJik · 2020-08-12T10:38:41Z

examples/test_examples.py

+def is_cuda_and_apex_avaliable():
+    return torch.cuda.is_available() and is_apex_available()
+
+


This should also depend from the USE_CUDA flag, similarly to the rest of the test suite. Even if we decide to set USE_CUDA to True by default, setting USE_CUDA to False should result in the examples not using CUDA imo.

I think you can do so by adding the following line:

from transformers.testing_utils import torch_device

and updating the statement to

return torch.cuda.is_available() and is_apex_available() and torch_device == "cuda"

Good catch @LysandreJik

LysandreJik · 2020-08-12T10:51:05Z

examples/test_examples.py

@@ -131,7 +141,6 @@ def test_run_language_modeling(self):
            --do_train
            --do_eval
            --num_train_epochs=1
-            --no_cuda


Can update this to respect the USE_CUDA flag as well:

Something like

testargs = f""" run_language_modeling.py --model_name_or_path distilroberta-base [...] --num_train_epochs=1 {'--no_cuda' if torch_device != 'cuda' else ''} """.split()

…into improve-tests-in-examples

… examples - The tests in examples which uses cuda should also depend from the USE_CUDA flag, similarly to the rest of the test suite. Even if we decide to set USE_CUDA to True by default, setting USE_CUDA to False should result in the examples not using CUDA

Joel-hanson · 2020-08-15T13:04:44Z

Thanks for the review @LysandreJik, I have updated the PR accordingly.

…into improve-tests-in-examples

LysandreJik

Cool this is great! Thanks for iterating!

LysandreJik · 2020-08-24T08:06:18Z

examples/test_examples.py

+
+
+def clean_test_dir(path):
+    shutil.rmtree(path, ignore_errors=True)


You should import shutil for this to work (that's why the code quality fails)

…into improve-tests-in-examples

codecov · 2020-08-24T15:16:08Z

Codecov Report

Merging #5512 into master will decrease coverage by 0.72%.
The diff coverage is 78.87%.

@@            Coverage Diff             @@
##           master    #5512      +/-   ##
==========================================
- Coverage   80.37%   79.64%   -0.73%     
==========================================
  Files         156      156              
  Lines       28058    28261     +203     
==========================================
- Hits        22552    22509      -43     
- Misses       5506     5752     +246

Impacted Files	Coverage Δ
src/transformers/commands/serving.py	`0.00% <0.00%> (ø)`
src/transformers/commands/user.py	`0.00% <ø> (ø)`
src/transformers/configuration_reformer.py	`100.00% <ø> (ø)`
src/transformers/data/processors/glue.py	`48.91% <0.00%> (-0.18%)`	⬇️
src/transformers/data/test_generation_utils.py	`0.00% <0.00%> (ø)`
src/transformers/generation_utils.py	`96.94% <ø> (ø)`
src/transformers/hf_argparser.py	`67.74% <0.00%> (-1.49%)`	⬇️
src/transformers/modeling_auto.py	`78.73% <ø> (ø)`
src/transformers/modeling_bert.py	`88.42% <ø> (+0.16%)`	⬆️
src/transformers/modeling_electra.py	`82.13% <ø> (ø)`
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a573777...51be82f. Read the comment docs.

For some reason huggingface#5512 re-added temp dir creation code that was removed by huggingface#6494 defeating the purpose of that PR for those tests.

For some reason #5512 re-added temp dir creation code that was removed by #6494 defeating the purpose of that PR for those tests.

…ggingface#5512) * Allow tests in examples to use cuda or fp16,if they are available The tests in examples didn't use the cuda or fp16 even if they where available. - The text classification example (`run_glue.py`) didn't use the fp16 even if it was available but the device was take based on the availablity(cuda/cpu). - The language-modeling example (`run_language_modeling.py`) was having `--no_cuda` argument which made the test to work without cuda. This example is having issue when running with fp16 thus it not enabled (got an assertion error for perplexity due to it higher value). - The cuda and fp16 is not enabled for question-answering example (`run_squad.py`) as it is having a difference in the f1 score. - The text-generation example (`run_generation.py`) will take the cuda or fp16 whenever it is available. Resolves some of: huggingface#5057 * Unwanted import of is_apex_available was removed * Made changes to test examples file to have the pass --fp16 only if cuda and apex is avaliable - run_glue.py: Removed the check for cuda and fp16. - run_generation.py: Removed the check for cuda and fp16 also removed unwanted flag creation. * Incorrectly sorted imports fixed * The model needs to be converted to half precision * Formatted single line if condition statement to multiline * The torch_device also needed to be checked before running the test on examples - The tests in examples which uses cuda should also depend from the USE_CUDA flag, similarly to the rest of the test suite. Even if we decide to set USE_CUDA to True by default, setting USE_CUDA to False should result in the examples not using CUDA * Format some of the code in test_examples file * The improper import of is_apex_available was sorted * Formatted the code to keep the style standards * The comma at the end of list giving a flake8 issue was fixed * Import sort was fixed * Removed the clean_test_dir function as its not used right now

For some reason huggingface#5512 re-added temp dir creation code that was removed by huggingface#6494 defeating the purpose of that PR for those tests.

…ggingface#5512) * Allow tests in examples to use cuda or fp16,if they are available The tests in examples didn't use the cuda or fp16 even if they where available. - The text classification example (`run_glue.py`) didn't use the fp16 even if it was available but the device was take based on the availablity(cuda/cpu). - The language-modeling example (`run_language_modeling.py`) was having `--no_cuda` argument which made the test to work without cuda. This example is having issue when running with fp16 thus it not enabled (got an assertion error for perplexity due to it higher value). - The cuda and fp16 is not enabled for question-answering example (`run_squad.py`) as it is having a difference in the f1 score. - The text-generation example (`run_generation.py`) will take the cuda or fp16 whenever it is available. Resolves some of: huggingface#5057 * Unwanted import of is_apex_available was removed * Made changes to test examples file to have the pass --fp16 only if cuda and apex is avaliable - run_glue.py: Removed the check for cuda and fp16. - run_generation.py: Removed the check for cuda and fp16 also removed unwanted flag creation. * Incorrectly sorted imports fixed * The model needs to be converted to half precision * Formatted single line if condition statement to multiline * The torch_device also needed to be checked before running the test on examples - The tests in examples which uses cuda should also depend from the USE_CUDA flag, similarly to the rest of the test suite. Even if we decide to set USE_CUDA to True by default, setting USE_CUDA to False should result in the examples not using CUDA * Format some of the code in test_examples file * The improper import of is_apex_available was sorted * Formatted the code to keep the style standards * The comma at the end of list giving a flake8 issue was fixed * Import sort was fixed * Removed the clean_test_dir function as its not used right now

For some reason huggingface#5512 re-added temp dir creation code that was removed by huggingface#6494 defeating the purpose of that PR for those tests.

…able (huggingface#5512)" This reverts commit 5d0b971.

Joel-hanson added 2 commits July 4, 2020 10:45

Unwanted import of is_apex_available was removed

e6ceca0

sshleifer reviewed Jul 19, 2020

View reviewed changes

Joel-hanson requested a review from sshleifer August 6, 2020 05:44

sshleifer suggested changes Aug 6, 2020

View reviewed changes

Joel-hanson added 3 commits August 8, 2020 17:43

Made changes to test examples file to have the pass --fp16 only if cu…

a0e8df0

…da and apex is avaliable - run_glue.py: Removed the check for cuda and fp16. - run_generation.py: Removed the check for cuda and fp16 also removed unwanted flag creation.

Merge remote-tracking branch 'upstream/master' into improve-tests-in-…

6ce7131

…examples

Incorrectly sorted imports fixed

48c032f

Joel-hanson requested a review from sshleifer August 8, 2020 12:38

sshleifer suggested changes Aug 9, 2020

View reviewed changes

Joel-hanson added 3 commits August 11, 2020 23:25

Merge branch 'master' of https://github.com/huggingface/transformers …

e9feeca

…into improve-tests-in-examples

The model needs to be converted to half precision

df9fe76

Formatted single line if condition statement to multiline

92152bf

Joel-hanson requested a review from sshleifer August 11, 2020 18:08

sshleifer approved these changes Aug 11, 2020

View reviewed changes

sshleifer requested a review from LysandreJik August 11, 2020 20:42

LysandreJik approved these changes Aug 12, 2020

View reviewed changes

Joel-hanson added 4 commits August 15, 2020 18:31

Merge branch 'master' of https://github.com/huggingface/transformers …

9d83c45

…into improve-tests-in-examples

Format some of the code in test_examples file

1eb5c18

The improper import of is_apex_available was sorted

57b407a

Joel-hanson requested a review from LysandreJik August 15, 2020 13:05

Joel-hanson added 4 commits August 15, 2020 18:37

Formatted the code to keep the style standards

f5825eb

The comma at the end of list giving a flake8 issue was fixed

9d35a4e

Merge branch 'master' of https://github.com/huggingface/transformers …

e99ab61

…into improve-tests-in-examples

Import sort was fixed

224100b

LysandreJik approved these changes Aug 24, 2020

View reviewed changes

Joel-hanson added 2 commits August 24, 2020 20:34

Merge branch 'master' of https://github.com/huggingface/transformers …

78c5bde

…into improve-tests-in-examples

Removed the clean_test_dir function as its not used right now

51be82f

LysandreJik merged commit 4db2fa7 into huggingface:master Aug 25, 2020

stas00 added a commit to stas00/transformers that referenced this pull request Sep 13, 2020

[examples testing] restore code

2034eaf

For some reason huggingface#5512 re-added temp dir creation code that was removed by huggingface#6494 defeating the purpose of that PR for those tests.

stas00 mentioned this pull request Sep 13, 2020

[examples testing] restore code #7099

Merged

LysandreJik pushed a commit that referenced this pull request Sep 14, 2020

[examples testing] restore code (#7099)

3ca1874

For some reason #5512 re-added temp dir creation code that was removed by #6494 defeating the purpose of that PR for those tests.

mfuntowicz pushed a commit that referenced this pull request Sep 18, 2020

[examples testing] restore code (#7099)

be1148a

For some reason #5512 re-added temp dir creation code that was removed by #6494 defeating the purpose of that PR for those tests.

fabiocapsouza added a commit to fabiocapsouza/transformers that referenced this pull request Nov 15, 2020

Revert "Allow tests in examples to use cuda or fp16,if they are avail…

75b58e8

…able (huggingface#5512)" This reverts commit 5d0b971.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow tests in examples to use cuda or fp16,if they are available #5512

Allow tests in examples to use cuda or fp16,if they are available #5512

Joel-hanson commented Jul 4, 2020

Joel-hanson commented Jul 4, 2020 •

edited

Loading

sshleifer commented Jul 5, 2020

Joel-hanson commented Jul 18, 2020

sshleifer left a comment

sshleifer Jul 19, 2020

Joel-hanson Jul 21, 2020

sshleifer Aug 6, 2020

sshleifer commented Jul 19, 2020

Joel-hanson commented Jul 21, 2020

Joel-hanson commented Jul 31, 2020

sshleifer left a comment

Joel-hanson commented Aug 8, 2020

sshleifer left a comment

sshleifer Aug 9, 2020

Joel-hanson Aug 11, 2020

sshleifer Aug 11, 2020

sshleifer left a comment

sshleifer Aug 11, 2020

LysandreJik left a comment

LysandreJik Aug 12, 2020

LysandreJik Aug 12, 2020

Joel-hanson Aug 15, 2020

LysandreJik Aug 12, 2020 •

edited

Loading

Joel-hanson commented Aug 15, 2020

LysandreJik left a comment

LysandreJik Aug 24, 2020

codecov bot commented Aug 24, 2020

		def is_cuda_and_apex_avaliable():
		return torch.cuda.is_available() and is_apex_available()



		def clean_test_dir(path):
		shutil.rmtree(path, ignore_errors=True)

Allow tests in examples to use cuda or fp16,if they are available #5512

Allow tests in examples to use cuda or fp16,if they are available #5512

Conversation

Joel-hanson commented Jul 4, 2020

Joel-hanson commented Jul 4, 2020 • edited Loading

sshleifer commented Jul 5, 2020

Joel-hanson commented Jul 18, 2020

sshleifer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshleifer commented Jul 19, 2020

Joel-hanson commented Jul 21, 2020

Joel-hanson commented Jul 31, 2020

sshleifer left a comment

Choose a reason for hiding this comment

Joel-hanson commented Aug 8, 2020

sshleifer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshleifer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LysandreJik Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Joel-hanson commented Aug 15, 2020

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2020

Codecov Report

Joel-hanson commented Jul 4, 2020 •

edited

Loading

LysandreJik Aug 12, 2020 •

edited

Loading