Can we remove force_download=True from tests?
Author: aaugustin
Pull request: no

## 🚀 Feature

I would like to remove `force_download=True` from tests and rely on the cache to keep cached files up to date.

## Motivation

Currently, several tests use `force_download=True` on large model files, e.g.:

https://github.com/huggingface/transformers/blob/9c58b236ef5fbbe5d0cbde4932eb342a73eaa0dc/transformers/tests/modeling_tf_auto_test.py#L49

This prevents caching large models within a test run and across test runs, which is very painful when working on these tests in a local environment. This is the main reason why I'm filing this issue.

In fact, it's so painful that these tests are marked as slow and skipped by default. As a consequence, we're not getting as much value from them as we could. If we downloaded each model only once, perhaps we could run them in CI.

I assume that `force_download=True` was added for robustness, to make sure the cache doesn't contain stale files.

If that's correct, then I believe I can be safely removed, because the current implementation of the cache in `file_utils.py` is sufficiently robust to keep the cache up to date.

The entry point is here:

https://github.com/huggingface/transformers/blob/8efc6dd544bf1a30d99d4b5abfc5e214699eab2b/transformers/configuration_utils.py#L157-L158

which goes here:

https://github.com/huggingface/transformers/blob/8efc6dd544bf1a30d99d4b5abfc5e214699eab2b/transformers/file_utils.py#L192-L196

which always gets an ETag (and, I guess, fails without an Internet connection):

https://github.com/huggingface/transformers/blob/8efc6dd544bf1a30d99d4b5abfc5e214699eab2b/transformers/file_utils.py#L288-L289

As a consequence, you never hit the only situation where the cache may use stale files: if it cannot get an ETag (either because there's no Internet connection, or because the HTTP server doesn't provide an ETag).

(As a side note, file_utils.py is needlessly complex, given that all transformers files are stored on S3.)

Am I missing a reason for `force_download=True`? Did you add it because you encountered another issue?

Comments:
After further research, I'm seeing two possible issues.

**transformers gets files from S3 with https:// URLs, not s3:// URLs.**

I think we want to preserve the capability to use the library offline; we don't want to require an Internet connection to check the ETag in all cases. So we have two possibilities here:

1. Accept that, on a machine without access to the Internet, tests may use stale files. I think that's acceptable.
2. Add a `force_check` option; if it's `True` and we can't fetch the ETag, raise an exception instead of using a possibly stale file. I don't think that's worth the effort.

**files may be truncated if shutil.copyfileobj is interrupted**

The whole point of downloading to a temp file and then moving to the final location is to prevent truncation. By using shutil.copyfileobj, you're reducing the risk of truncation, but not eliminating it.

Here's what I would do:

- download to a temp file with a random name in the destination folder — to ensure it's on the same disk partition and to prevent an expensive copy
- rename it — renaming is atomic in a practical sense in this context

I'm not proposing to fetch the Content-Length and compare it with the file on disk because I can't see a situation where we'd get a truncated file.

----

**Summary of proposal**

- harden the "download to temp file then move" process as described just above
- remove the `force_download=True` option from tests

see related https://github.com/huggingface/transformers/issues/1678