Fix bug in Wav2Vec2's GPU tests
Author: falcaopetri
Pull request: yes

# What does this PR do?

Fixes bugs introduced in #18351. Bugs appeared only when running tests in GPU, as reported and explained in https://github.com/huggingface/transformers/pull/18351#issuecomment-1285251004. 

In summary, some tests introduced in the previous PR:
- Failed when running on the same process
- Failed to call `.cpu()` before calling `.numpy()`

## Who can review?

@ydshieh, @patrickvonplaten 

Comments:
_The documentation is not available anymore as the PR was closed or merged._

Hi @falcaopetri Thank you for the fix. I run the 3 tests against this PR, but for `test_wav2vec2_with_lm_invalid_pool` (bot PT/TF), I get the following error:

```bash
>       self.assertIn("Falling back to sequential decoding.", cl.out)
E       AssertionError: 'Falling back to sequential decoding.' not found in ''
```

i.e. `cl.out` is empty string here.

Could you double check here, please?

`test_wav2vec2_with_lm_pool` is fixed by this PR though!


Hi @ydshieh. This one is trickier.

As I was trying to fix the previous issue, I ended up changing the execution path by changing 

```diff
-processor.batch_decode(logits.numpy())
+processor.batch_decode(logits.cpu().numpy(), pool)
```
(see https://github.com/huggingface/transformers/pull/19803/commits/ca9f3f66d3e9cf786e9db86a1a710bff7057ab2b#diff-1063ef75ba73fe97fec48faf71f5020152ca85811784caaef74d4ca18fc6049fL1674-L1677).

I was aiming to test this line:

https://github.com/huggingface/transformers/blob/d4eb52d13d7af8be06ccb7723e1991a6f8ed8f59/src/transformers/models/wav2vec2_with_lm/processing_wav2vec2_with_lm.py#L399-L402

But that's protected by a https://github.com/huggingface/transformers/blob/d4eb52d13d7af8be06ccb7723e1991a6f8ed8f59/src/transformers/models/wav2vec2_with_lm/processing_wav2vec2_with_lm.py#L391

As I see, we could:
1. Don't test this execution path
2. Add back the `multiprocessing.set_start_method("spawn")` approach.
    - Pros: Previous bug happened because I was calling `set_start_method` twice, but we could re-implement it using a single call
    - Cons: This affects the whole runtime process, and since all GPU's tests are running in the same process, we could potentially break other tests (I didn't find any potential such tests in the code base, but we could have them in the future)

---
One thing that caught my attention is that I'd expect the PR's CI/CD tests to fail. Looking the logs of `tests_torch*` I found:

```
tests_torch: ================ 130 passed, 218 skipped, 46 warnings in 32.78s ================
tests_torch_and_tf: ============================== 4 passed in 21.11s ==============================
```

Is `tests_torch` expected to skip all these tests? I also noticed that `tests_tf` uses `pytest -rA` flag, which makes it easier to debug which tests were skipped. `tests_torch` does not.

Hi @falcaopetri 

- what if we don't pass `pool` here
https://github.com/huggingface/transformers/blob/ca9f3f66d3e9cf786e9db86a1a710bff7057ab2b/tests/models/wav2vec2/test_modeling_flax_wav2vec2.py#L612
would it work and test the target execution path we want?
- Regarding `calling set_start_method twice`, do you mean one in PyTorch test and another one in TensorFlow test?

Regarding tests being skipped in PR CI, that is expected, as the relevant tests here are implemented in `Wav2Vec2ModelIntegrationTest` class, which is decorated with `@slow`, so those tests are run only after a PR being merged into `main`.

> * what if we don't pass `pool` here
>   would it work and test the target execution path we want?

The target execution path is meant to alert non-unix users about the current limitations on these platforms when they use `batch_decode+don't specify a pool`. So yes, we shouldn't pass `pool` here.

https://github.com/huggingface/transformers/blob/d4eb52d13d7af8be06ccb7723e1991a6f8ed8f59/src/transformers/models/wav2vec2_with_lm/processing_wav2vec2_with_lm.py#L391-L403

> * Regarding `calling set_start_method twice`, do you mean one in PyTorch test and another one in TensorFlow test?

I'm not sure if I understood your question, but yes, `set_start_method` was being called on every test that required us to "simulate" we were on a non-unix platform (where `spawn` is default), i.e., it was used in `test_wav2vec2_with_lm_invalid_pool` from PT, TF and Flax tests. `set_start_method` should be called only once within a process though.

----

A little bit of context:
- `pyctcdecode` does not currently work well with `spawn` contexts: if a `spawn` pool is passed, it will print a warning message and ignore the pool (see https://github.com/kensho-technologies/pyctcdecode/commit/a477d796e232b476ee8b877efba98aa2d822232e)
-  The change I've implemented is unrelated to this: `batch_decode` just needed to allow passing a `pool`, since default behavior is to create a new pool for every call (i.e., there was a huge overhead when processing multiple audios)
- Since I was changing the code, I added a "safeguard" to the default behavior: "if no pool is specified, we create a pool only if we are on unix". The target execution path of the failing test is **"no pool specified and not on unix"**: instead of starting a spawn `Pool` that we know will be ignored by `pyctcdecode`, **we skip its creation and warn users.**

So, technically, we could remove this "safeguard", instantiate a `Pool` independently whether it will or won't be used by `pyctcdecode`, and let it handle the user warning if necessary. 

As you mentioned, the failing test mentioned in my comment is because the targe path is not executed (so we don't get the desired warning), and the reason is the `pool` is passed, my suggestion is to remove `pool` in
https://github.com/huggingface/transformers/blob/ca9f3f66d3e9cf786e9db86a1a710bff7057ab2b/tests/models/wav2vec2/test_modeling_wav2vec2.py#L1678
and see if the test will pass (i.e. if the remaining part will do whatever they are expected to do).

If this still fails (and the fix would involve more things), we can probably try to use a method recently intorduced
https://github.com/huggingface/transformers/blob/371337a95b5d82cc9376c2595ed2022a5eb2ee6e/src/transformers/testing_utils.py#L1678
(which could avoid the problem of `calling set_start_method twice`). But we can work on this on our side :-), and I am fine to merge this PR as it is.

Thank you for all the explanation :-) really appreciated, @falcaopetri !




`transcription = processor.batch_decode(logits.cpu().numpy()).text` would fail in Unix envs (e.g., during CI/CD).
This happens because `fork` is the default multiprocessing context, hence `set_start_method('spark')` is required.

`run_test_in_subprocess` is really helpful in this case. I took the liberty to try it out myself (see new commit). I got TF, PT and Flax working correctly. Tested in both CPU and GPU, by running in Colab the following
`RUN_SLOW=yes pytest -k test_wav2vec2_with_lm_invalid_pool tests/models/wav2vec2/test_modeling*_wav2vec2.py`.

----
As a side note, I had to force `"torchaudio<0.12"` on my local env and Colab, otherwise the following failed:
```python
>>> from datasets import load_dataset
>>> ds = load_dataset("common_voice", "es", split="test", streaming=True)
>>> sample = next(iter(ds))
...
File "/opt/miniconda/base/lib/python3.8/site-packages/datasets/features/audio.py", line 273, in _decode_mp3
  array, sampling_rate = torchaudio.load(path_or_file, format="mp3")
File "/opt/miniconda/base/lib/python3.8/site-packages/torchaudio/backend/sox_io_backend.py", line 214, in load
  return _fallback_load_fileobj(filepath, frame_offset, num_frames, normalize, channels_first, format)
File "/opt/miniconda/base/lib/python3.8/site-packages/torchaudio/backend/sox_io_backend.py", line 33, in _fail_load_fileobj
  raise RuntimeError(f"Failed to load audio from {fileobj}")
RuntimeError: Failed to load audio from <_io.BytesIO object at 0x130509bd0>
```

I installed transformers with `[torch-speech,flax,torch,tf]` extras.

Also cc @sanchit-gandhi here

For @sgugger to take a quick look before I can merge


Thanks for all the help @ydshieh! I'm really happy to help improving `transformers` (and to fix my failing tests 😅)!

> Still think this should be in a decorator

@sgugger you mean `run_test_in_subprocess` API right? Given its signature (`run_test_in_subprocess(test_case, target_func, inputs=None, timeout=600)`) I was ready to just use it as a decorator in my tests. Then I saw its usage in `Whisper`'s and realized the pickling concerns. IMHO it would have a much cleaner API if feasible though.

Thanks for the fix @falcaopetri! The new testing logic LGTM!

@falcaopetri Not sure how it's feasible exactly, but will definitely try something :-)

Sorry, I thought I merged this PR, but actually not. Thanks @sgugger