make the tmp dir configurable/persistent in tokenizer tests
Author: stas00
Pull request: yes

Currently, debugging tokenizers is difficult since the temp dir is random and it gets wiped out at the end of the test run. It can be done, but it takes so much repetitive work.

This PR uses the recently added [`TestCasePlus`](https://github.com/huggingface/transformers/pull/6494) which automatically sets up temp dirs and optionally doesn't remove them at the end of the test. It makes it very easy to configure the temp dir to be fixed rather than random, and also not delete itself.

As a side-effect of inheriting from `TestCasePlus`, the mixin approach of `FooTest(TokenizerTesterMixin, unittest.TestCase)` doesn't work - as it now tries to run the super-class tests directly and not from within the subclass.  Therefore, the code switches to normal sub-classing and instructs `unittest` not to run super-class' tests on its own, using the following machinery as explained [here](https://stackoverflow.com/a/50922971/9201239). Specifically here:

```
from transformers.testing_utils import TestCasePlus
class TokenizerCommonTester(TestCasePlus):
    __test__ = False

# and then the sub-class:
from .test_tokenization_common import TokenizerCommonTester
class BartTokenizationTest(TokenizerCommonTester):
    __test__ = True
```

The PR  makes the code ready to debug by changing just one flag:

```
       DEBUG = False
        # if you need to debug the contents of the tmpdirname, set DEBUG to True, which will then use
        # a hardcoded path and won't delete it at the end of the test
        if not DEBUG:
            self.tmpdirname = self.get_auto_remove_tmp_dir()
        else:
            self.tmpdirname = self.get_auto_remove_tmp_dir(tmp_dir="./tmp/token-test", after=False)
```
So just make `DEBUG` `True` and nothing else needs to be tweaked.

I hope this is useful for developers.

There are a few other test mixins that could be improved the same way, but let's see if this approach is welcomed first.

----

## An alternative solution

If mixin is preferable, then let's leave everything as is and do this instead:

```
# transformers/testing_utils.py
from pathlib import Path
def make_dir(path):
    Path(path).resolve().mkdir(parents=True, exist_ok=True)
    return path

# tests/test_tokenization_common.py
from transformers.testing_utils import make_dir
DEBUG = True
class TokenizerTesterMixin:

    tokenizer_class = None
    test_rust_tokenizer = False

    def setUp(self):
        if not DEBUG:
            self.tmpdirname = tempfile.mkdtemp()
        else:
            self.tmpdirname = make_dir("./tmp/test-tok")

    def tearDown(self):
        if not DEBUG:
            shutil.rmtree(self.tmpdirname)
```



Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/6815?src=pr&el=h1) Report
> Merging [#6815](https://codecov.io/gh/huggingface/transformers/pull/6815?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/5ab21b072fa2a122da930386381d23f95de06e28?el=desc) will **decrease** coverage by `1.46%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/6815/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/6815?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #6815      +/-   ##
==========================================
- Coverage   79.58%   78.11%   -1.47%     
==========================================
  Files         157      157              
  Lines       28588    28588              
==========================================
- Hits        22752    22332     -420     
- Misses       5836     6256     +420     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/6815?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/modeling\_tf\_albert.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9hbGJlcnQucHk=) | `21.47% <0.00%> (-69.44%)` | :arrow_down: |
| [src/transformers/pipelines.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9waXBlbGluZXMucHk=) | `26.26% <0.00%> (-53.69%)` | :arrow_down: |
| [src/transformers/optimization.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9vcHRpbWl6YXRpb24ucHk=) | `34.28% <0.00%> (-48.00%)` | :arrow_down: |
| [src/transformers/optimization\_tf.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9vcHRpbWl6YXRpb25fdGYucHk=) | `33.33% <0.00%> (-24.33%)` | :arrow_down: |
| [src/transformers/modeling\_roberta.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19yb2JlcnRhLnB5) | `74.81% <0.00%> (-22.27%)` | :arrow_down: |
| [src/transformers/modeling\_longformer.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19sb25nZm9ybWVyLnB5) | `71.55% <0.00%> (-20.48%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_auto.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9hdXRvLnB5) | `48.79% <0.00%> (-18.08%)` | :arrow_down: |
| [src/transformers/data/processors/squad.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9kYXRhL3Byb2Nlc3NvcnMvc3F1YWQucHk=) | `13.76% <0.00%> (-14.38%)` | :arrow_down: |
| [src/transformers/modeling\_auto.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19hdXRvLnB5) | `64.36% <0.00%> (-14.37%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_xlm.py](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl94bG0ucHk=) | `88.42% <0.00%> (-4.85%)` | :arrow_down: |
| ... and [24 more](https://codecov.io/gh/huggingface/transformers/pull/6815/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/6815?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/6815?src=pr&el=footer). Last update [5ab21b0...d984fd8](https://codecov.io/gh/huggingface/transformers/pull/6815?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


After sleeping on this, I'm not quite sure of 2 things.

1. the main switch from mixin to normal subclassing - if it's done it should be done for all other common testing mixins - the benefit would be - having simpler access to `unittest.TestCase` and the extended `unittest.TestCasePlus` features. As I proposed in the alternative solution, it's not at all required, as a different solution can be used for temp dirs during debug.

2. a totally unrelated issue of having debugging code in place. Do we want to gradually make the test suite easier to debug, by leaving `if DEBUG: ...` in strategic places (currently, consisting of just one thing - having a fixed tmp dir and not having it removed, but there are probably others). 

   For example, I find myself adding a debug message for various asserts, so it's easier to see what's not matching, but those are usually a 2nd/3rd argument to the assert function (or `msg=`), so it's a smooth feature requiring no `if DEBUG`.

i.e. I'd love to hear what others think - if you think this is a useful discussion - I can open 2 unrelated issues if it helps to make discussing these 2 unrelated issues focused.

My inclination right now is to just provide a quick way to make a fixed temp dir w/o it being deleted, i.e. the alt solution in OP, and leave the original PR for maybe some time in the future if we see other benefits to doing so.

I agree with having a quicker fix for this specific problem and think a bit more about a general way to have a specific debug behavior for our use.

If you're joining in now, please ignore the proposed code (as it also requires changing from Mixin to a subclass), and what this needs is your feedback on this question: **do we want to have a simple DEBUG flag in tests, that once enabled it would switch to not deleting temp dirs and would use a fixed temp dir path, so that it's easy to monitor?** So instead of needing to manually tweak the tests, we have the debug setup already in place. That's the question.

Let me know if perhaps I should scratch that PR and start a new one discussing just that, so that the initial attempts at solving the issue won't be confusing to you, the readers.

And to quickly give you context, we are talking about:
```
    def setUp(self):
        self.tmpdirname = tempfile.mkdtemp()
```
and the modified version is:
```
DEBUG=0
[...]
    def setUp(self):
        super().setUp()
       
        # if you need to debug the contents of the tmpdirname, set DEBUG to True, which will then use
        # a hardcoded path and won't delete it at the end of the test
        if not DEBUG:
            self.tmpdirname = self.get_auto_remove_tmp_dir()
        else:
            self.tmpdirname = self.get_auto_remove_tmp_dir(tmp_dir="./tmp/token-test", after=False)
```
https://github.com/huggingface/transformers/blob/d984fd82bf940c62700919da5735e60f3f883348/tests/test_tokenization_common.py#L69

except the code itself will be different as we can't make it work with mixins in that way.

If it helps, here is the last time a related issue of working with temp dirs has been worked on with a successful PR merge:
https://github.com/huggingface/transformers/pull/6494 - i.e. this is a continuation of the same to other parts of the test suite.


> do we want to have a simple DEBUG flag in tests, that once enabled it would switch to not deleting temp dirs and would use a fixed temp dir path, so that it's easy to monitor?

yes, this would be useful if you can do it in a way that doesn't add overhead for people trying to add new tokenizers.

I didn't look at the code.

I will close it for now and revisit the next time I deal with this unless someone beats me to it.