noamaanMulla-03/transformers-issues huggingface dataset:
Incorrect size of input for 1st strided window length in `Perplexity of fixed-length models`
Author: ekagra-ranjan
Pull request: no
Labels: name: bug

### System Info

- `transformers` version: 4.20.1
- Platform: Linux-5.10.133+-x86_64-with-debian-bullseye-sid
- Python version: 3.7.12
- Huggingface_hub version: 0.8.1
- PyTorch version (GPU?): 1.11.0+cpu (False)
- Tensorflow version (GPU?): 2.6.4 (False)
- Flax version (CPU?/GPU?/TPU?): 0.6.0 (cpu)
- Jax version: 0.3.16
- JaxLib version: 0.3.15

### Who can help?

@sgugger, @stevhliu

### Information

- [X] The official example scripts
- [ ] My own modified scripts

### Tasks

- [X] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

1. [The example script for finding the perplexity of fixed length](https://huggingface.co/docs/transformers/perplexity) model using strided windows takes an shorter window size for the 1st set of inputs.  

    For demo, let's us take a subset of test data in the script and print the `begin_loc`, `end_loc`, and `trg_len`. The example script picks a window of [0: 512] for the `input_ids` in 1st pass and then picks [0: 1024] in 2nd pass. The size of `input_ids` in the 1st pass is shorter than expected which should have been [0: 1024] in the 1st pass itself as the model's max length is 1024. 

    This leads to higher overall PPL as the output in the 2nd pass get a smaller context window size. This can be seen in the shared  notebook which prints these stats for the **example script** and the **proposed script** for comparison: https://www.kaggle.com/code/ekagra/perplexity-contrib-small?scriptVersionId=104864785

2. The PPL for GPT2-large when running the example script comes to be 16.44 instead of ~~19.64~~ 16.53. Maybe improvement was made in the tokenizer or in the GPT2 model definition after the example script was written which improves the PPL?

    Sharing notebook for PPL computed on the entire test data for [example script](https://www.kaggle.com/code/ekagra/perplexity-contrib/notebook?scriptVersionId=104846069) and [proposed script](https://www.kaggle.com/code/ekagra/perplexity-contrib?scriptVersionId=104846058).  Both approaches obtain the same PPL of 16.44 as we are taking average over a lot of numbers so the small difference in NLLs in the 1st 2 window size vanishes. The difference however can be seen in the previous notebook which finds PPL over a smaller test data.



If this makes sense then I could raise a PR for this. 

EDIT 1: replaced 19.64 with 16.53 which is the right metric to look for stride size of 512.

Comments:
Yes, it looks like the PPL is not 19.64 as advertised. Would you like to make a PR with the suggested changes? It all looks good to me.

Sure @sgugger. On it.
Pin `detectron2` for CircleCI tests
Author: ydshieh
Pull request: yes

# What does this PR do?

The commit `36a65a0907d90ed591479b2ebaa8b61cfa0b4ef0` in `detectron2 ` break things, see [here](https://github.com/facebookresearch/detectron2/commit/36a65a0907d90ed591479b2ebaa8b61cfa0b4ef0#comments).

Ping the parent commit until that issue is fixed.

Issue found by @amyeroberts 

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

Merge now to avoid  CI failure on PRs.
raise atol for MT5OnnxConfig
Author: ydshieh
Pull request: yes

# What does this PR do?

MT5 is newly added to ONNX tests, but currently failed with
```
AssertionError: mt5, seq2seq-lm -> Outputs values doesn't match between reference model and ONNX exported model: Got max absolute difference of: 0.000148773193359375

AssertionError: mt5, seq2seq-lm-with-past -> Outputs values doesn't match between reference model and ONNX exported model: Got max absolute difference of: 0.00020599365234375
```

[failed job run](https://github.com/huggingface/transformers/runs/7718274393?check_suite_focus=true)

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

I need to check the `# Copied` issue.

Hi, @ydshieh. Does this PR fix the two failing tests?

If not, I can work on it too.

The test is still running. I think it should be fine :-) but will let you know once the test run is finished. Thanks.
Minor update of `run_call_with_unpacked_inputs`
Author: ydshieh
Pull request: yes

# What does this PR do?

Use `type(self).__name__` instead of `str(self).lower()`.

This is a follow-up of [this comment](https://github.com/huggingface/transformers/pull/18097#discussion_r926907848) by @gante.

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

Thanks for making the change 👍
unpin torch to use 1.12.1
Author: ydshieh
Pull request: yes

# What does this PR do?

unpin torch to use 1.12.1

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

Let's maybe add a `!=1.12.0` as well. cc @sgugger, who also has a PR open here: https://github.com/huggingface/transformers/pull/17925

Here are the tests to fix before we can use 1.12.1

  - tests/models/tapas/test_modeling_tf_tapas.py -k "TFTapasModelTest and test_pt_tf_model_equivalence"
  - tests/onnx/test_onnx_v2.py -k "StableDropoutTestCase and test_training"
  - tests/pipelines/test_pipelines_table_question_answering.py -k "TQAPipelineTests and test_slow_tokenizer_sqa_pt"
  - tests/pipelines/test_pipelines_table_question_answering.py -k "TQAPipelineTests and test_small_model_pt"
  - tests/models/tapas/test_modeling_tapas.py::TapasUtilitiesTest::

Most tapas tests will likely work given @sgugger's PR above, as it's probably linked to the version of the torch-scatter dependency

Yes! Thanks, @LysandreJik 

(guess I can close this PR, and just merge #17925)
I will check `StableDropoutTestCase and test_training` though.

Can you push necessary changes directly on #17925 (I'm too lazy to check this PR contains the same fixes as this one 😅 ) The branch is `enable_pt12`.

Close this and work on  #17925 instead
Fix `test_dbmdz_english` by updating expected values
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix #18405

I originally stated this is an expected value - it turns out to be an **input sentence**. But probably it is still intentional to have `the the`.

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

I think we can update the results instead yes:

The original string didn't have the typo:

https://github.com/huggingface/transformers/issues/5077#issuecomment-656398617

So probably my bad in putting it into a test (or I copied from somewhere else, I can't remember)

(off topic) @Narsil How you are able to find that (very) old comment - I can't even find some comments that are just 2-3 months old 😢 

Voilà 🚀 

> you

I copy pasted `Enzo works at the UN` in the GH search bar within `issues` tab.
It doesn't work all the time, but it does work better than searching in the top left bar.

GH search is very hit and miss. Really depends how segregating your keywords are. and you HAVE to be word aligned (which is super annoying when looking for function/method names since I usually only know part of the name)
Update some expected values in `quicktour.mdx` for `resampy 0.3.0`
Author: ydshieh
Pull request: yes

# What does this PR do?

It took me some time to figure out the test failure is due to different `resampy` versions. Some current values work with `0.2.2`, but not with `0.3.0`.


[current failed job](https://github.com/huggingface/transformers/runs/7682932969?check_suite_focus=true)

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Add machine type in the artifact of Examples directory job
Author: ydshieh
Pull request: yes

# What does this PR do?

We have
<img width="257" alt="Screenshot 2022-08-03 174611" src="https://user-images.githubusercontent.com/2521628/182652043-02a031a1-c8b9-457a-8876-130a37099075.png">
even when there are some errors in `Examples directory` test.

(relevant run: https://github.com/huggingface/transformers/actions/runs/2786567567)

Adding the machine type (single-gpu / multi-gpu) in the artifact names should make things work.

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

(I should have run the job and make sure it works before requesting review - ending up a few more commits to fix things, sorry)
disable Onnx test for google/long-t5-tglobal-base
Author: ydshieh
Pull request: yes

# What does this PR do?

For `("longt5", "google/long-t5-tglobal-base")`, we get

```bash
Floating point exception (core dumped)
```
in this call
https://github.com/huggingface/transformers/blob/fc546332d7a9395323f656635362c9e0f3c4161a/src/transformers/onnx/convert.py#L404

Let's disable it for now, so other Onnx tests could be run.

[Failed job run](https://github.com/huggingface/transformers/runs/6892306185?check_suite_focus=true)

Comments:
Adding @regisss as a reviewer as this is suggested by GitHub automatically 😄 

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

I will tag lewis when he is back.

Hi @lewtun !

Could you take a look at this ONNX test? Thank you.
Accept `trust_remote_code` and ignore it in `PreTrainedModel.from_pretrained`
Author: ydshieh
Pull request: yes

# What does this PR do?

Hope my understanding is correct. Let me know if I should apply the same change to `ProcessorMixin`, `PreTrainedTokenizerBase` etc.

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

@sgugger FYI:

I applied the change to TF and Flax `PreTrainedModel` (which is necessary, at least for TF), as well as `PretrainedConfig`.
Leave the tokenizer class and feature extractor class for now
Fix `test_load_default_pipelines_tf` test error
Author: ydshieh
Pull request: yes

# What does this PR do?

My change in #18292 needs to add `tf` under `default` key (for `image-classification`), otherwise we have 

```bash
FAILED tests/pipelines/test_pipelines_common.py::PipelineUtilsTest::test_load_default_pipelines_tf
```
with error message
```bash
         else:
             # normal case - non-translation pipeline
 >           model_id, revision = task_dict["default"]["model"][framework]
 E           KeyError: 'tf'
```



Comments:
_The documentation is not available anymore as the PR was closed or merged._
Fix torchscript tests for GPT-NeoX
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix torchscript tests for GPT-NeoX. The main issue comes from the fact that current `RotaryEmbedding` changes the model structure in `forward`.

This PR creates the necessary embeddings in `__init__`, which basically makes the cache (of embedding) mechanism useless. Furthermore, the attribute names seems a bit confusing now. We could probably add some attribute (ex. `init_sin_cos_cache_seq_len`) in config with a value `<= max_position_embeddings`, but I think it's way too much.

Not certain if it is worth it. However, with a PR opened, we have a reference.

The current failing test is 
https://github.com/huggingface/transformers/runs/7216768053?check_suite_focus=true

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

> LGTM!
> 
> However, could we add the failing test for reference or do we need to add a new test here?

I updated the PR description to include the current failing test. Regarding new tests, I don't think it's necessary, as we just build the necessary tensors in `__init__` instead of in `forward`, and the current set of tests should be enough :-)

(however, let me know if you have some idea of new necessary test cases!)

> > LGTM!
> > However, could we add the failing test for reference or do we need to add a new test here?
> 
> I updated the PR description to include the current failing test. Regarding new tests, I don't think it's necessary, as we just build the necessary tensors in `__init__` instead of in `forward`, and the current set of tests should be enough :-)
> 
> (however, let me know if you have some idea of new necessary test cases!)

Perfect thanks!
Update cache for CircleCI tests
Author: ydshieh
Pull request: yes

# What does this PR do?

After PR #18197, we need to create new cache, otherwise we get some errors, as shown in [this run](https://app.circleci.com/pipelines/github/huggingface/transformers/44104/workflows/1b63ec34-ef95-4678-adc2-773de35342ab/jobs/511895/steps), coming from some checks in `datasets` regarding module imports.

Run all tests with newly created cache + Run torch example tests with new cache loaded --> all pass

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Avoid flaky trainer test failures
Author: ydshieh
Pull request: yes

# What does this PR do?

Avoid flaky trainer test failures on multi GPUs.

I ran the test 1000 times on single/muti GPU VMs.

- single gpu: all 0.0
- multi gpu: a lot of 0.0, but the largest can go 1e-4.

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

> I assume that 1e-7 and 1e-6 were still flakey, right? Just concerned with very low margin where it could potentially hide issues.
> 
> But otherwise looks good. Thank you for fixing, @ydshieh

From Slack report, I could see a few times with `2e-6`. On my manual tests (1000 times), multi GPUs could reach `1e-4`. I choose `1e-5`, thinking the test failure would be super rare.

Super, thank you for sharing the details of why this value was chosen, @ydshieh!

All is good.
skip some gpt_neox tests that require 80G RAM
Author: ydshieh
Pull request: yes

# What does this PR do?

GPT-NeoX requires ~80G RAM to run. Our CI runners have only 60G RAM. Skip a few tests for now.

Do you think it's better to use something like

```python
@unittest.skipUnless(psutil.virtual_memory().total / 1024 ** 3 > 80, "GPT-NeoX requires 80G RAM for testing")
```

The problem is that `psutil` is not in the requirements.



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

What Sylvain said and I'd ask an even more different question - why are we running the same test on many identical models of different sizes. The purpose of our test suite is not to test models on the hub, it's to test the model's code. So such tests should never be there in the first place.

- 99% of the time the tests should be run against tiny random models, most of which reside under  https://huggingface.co/hf-internal-testing - these are functional tests.
- 1% of tests should be against the smallest non-random model to test the quality of the results. And typically these are `@slow` tests.

Of course, the % breakdown is symbolic, the point I was trying to convey is that most tests should be really fast in download and execution.

---------------

If there is a need to test models on the hub, there should be another CI that all it does is loading the models and performs some basic test on them. That CI would need to have a ton of CPU and GPU memory and # of GPUs for obvious reasons - e.g. t5-11b and other huge models.

Hi @stas00 

The related tests here are decorated with `@slow` and run in the daily scheduled CI, not push CI. And only one size is tested `GPT_NEOX_PRETRAINED_MODEL_ARCHIVE_LIST[:1]`.

For `test_model_from_pretrained`, I think we can use tiny random models in `hf-internal-testing` for `GPTNeoX` if we want to keep the test. However, we always have integration tests (like `GPTNeoXModelIntegrationTest`) which are important to have.

Note that on scheduled CI, we use a cache server (FileStore on GCP), so there is no real downloading (e.g. the downloading is very fast, happening between GCP's network).

They also have 16 vCPUs and 60G RAM.

Ah, good point, I missed `[:1]` - why then there is a loop then?

```
for model_name in GPT_NEOX_PRETRAINED_MODEL_ARCHIVE_LIST[:1]:
```

probably should write out explicitly the desired smallest real model then and perhaps it's small enough to fit?



The main point is that GPT-Neo-X does not come with a smaller pretrained model, there is only the 20B version.

> Ah, good point, I missed `[:1]` - why then there is a loop then?
> 
> ```
> for model_name in GPT_NEOX_PRETRAINED_MODEL_ARCHIVE_LIST[:1]:
> ```
> 
> probably should write out explicitly the desired smallest real model then and perhaps it's small enough to fit?

I think this is from old code. We don't want to maintain `...PRETRAINED_MODEL_ARCHIVE_LIST` anymore, and for some models, we do use the explicit checkpoint name.

I will just remove the 2 tests here.

Removed. Will rebase on main later to see if tests all pass

I am ready for the merge :-)
[Do NOT merge 🙏 ] Skip a particular exception in `test_sample_generate`
Author: ydshieh
Pull request: yes

# What does this PR do?

A continuation of #17937 to fix a CI failure
```
            # sample
            probs = nn.functional.softmax(next_token_scores, dim=-1)
>           next_tokens = torch.multinomial(probs, num_samples=1).squeeze(1)
E           RuntimeError: probability tensor contains either `inf`, `nan` or element < 0
```

As @patrickvonplaten , when a broken generation happens due to all `-inf` scores along the vocab dimension, nothing we can do. This is likely to happen only with random models however.

Let's say goodbye to this flaky situation!

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

Let's maybe put this PR on hold for 1,2 weeks to see if #18053 has solved the issue or not :-)

OK, thank you for taking time on this.
Avoid nan during sampling in generate()
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix CI test error
```bash
            # sample
            probs = nn.functional.softmax(next_token_scores, dim=-1)
>           next_tokens = torch.multinomial(probs, num_samples=1).squeeze(1)
E           RuntimeError: probability tensor contains either `inf`, `nan` or element < 0
```
in
https://github.com/huggingface/transformers/runs/6959698965?check_suite_focus=true

The test `test_sample_generate` may still fail at
https://github.com/huggingface/transformers/blob/8f400775fc5bc1011a2674dcfd5408d30d69f678/tests/generation/test_generation_utils.py#L711
for some unknown reason. I think it is better to investigate this in another PR.

Comments:
I have some doubts here, as this will make all tokens having equal probability to be sampled. But with all `-inf`, nothing could be sampled which leads to error. I feel there is no well-defined expected results in such edge cases.

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

Yes, that happens only when all `-inf` along the vocab dim. I will close this PR, and we have to maybe create a doc with all possible flaky tests :-)
Ignore `test_multi_gpu_data_parallel_forward` for `LayoutLMV2`
Author: ydshieh
Pull request: yes

# What does this PR do?

Ignore test_multi_gpu_data_parallel_forward for LayoutLMV2.

The reason to skip is the same as in #17864. (The usage of `add_module`)

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

Ok, so it's always advised to avoid using `add_module`?

@NielsRogge Not really. The test `test_multi_gpu_data_parallel_forward` uses `nn.DataParallel`, but PyTorch recommendes to use `DistributedDataParallel`, see [here](https://pytorch.org/docs/stable/generated/torch.nn.DataParallel.html`.)

However, I don't know if `add_module` works well with `DistributedDataParallel`. It would be good to avoid `add_module` until we decide to remove all `nn.DataParallel`. But in the cases where you really need `add_module`, don't hesitate.

@NielsRogge I guess I need to add more meaningful commit message, so you don't have to double check when clicking the merge button :-)
Update MT5Config (`is_gated_act`)
Author: ydshieh
Pull request: yes

# What does this PR do?

Update MT5Config (`is_gated_act`). Fix #17578. (Just copy-paste from `T5Config`)

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

@patrickvonplaten You forgot to edit the title during merge and it became `fix` in the commit page. No worry, just to be a bit bit slower next time 🙏  Thank you :-)
Fix `TFRemBertModelTest.test_resize_token_embeddings`
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix `TFRemBertModelTest.test_resize_token_embeddings`.

This method

https://github.com/huggingface/transformers/blob/028d4b7c8be2c2fc1146fcc1e9bd253c1a7ea346/src/transformers/modeling_tf_utils.py#L1449

assumes that `word_embedding_weight` has the same shape as `old_lm_head_decoder`, but this is not the case for `TFRemBertModel`, as it has `input_embedding_size` and `output_embedding_size` in config.

This PR checks the shape before checking the values. If shape is not equal, it means the input/output are not equal.

Fix the CI failure [here](https://github.com/huggingface/transformers/runs/6682139350?check_suite_focus=true)

Comments:
A quick look on 
https://github.com/huggingface/transformers/blob/028d4b7c8be2c2fc1146fcc1e9bd253c1a7ea346/src/transformers/modeling_utils.py#L1315

~~I couldn't find equivalent logic about `is_input_output_equals` that was in TF.~~

Guess it is because we use `self.config.tie_word_embeddings` to check in PyTorch.

> # What does this PR do?
> Fix `TFRemBertModelTest.test_resize_token_embeddings`.
> 
> This method
> 
> https://github.com/huggingface/transformers/blob/028d4b7c8be2c2fc1146fcc1e9bd253c1a7ea346/src/transformers/modeling_tf_utils.py#L1449
> 
> assumes that `word_embedding_weight` has the same shape as `old_lm_head_decoder`, but this is not the case for `TFRemBertModel`, as it has `input_embedding_size` and `output_embedding_size` in config.
> 
> This PR checks the shape before checking the values. If shape is not equal, it means the input/output are not equal.
> 
> Fix the CI failure [here](https://github.com/huggingface/transformers/runs/6682139350?check_suite_focus=true)

Sounds like an exception to me! I'm not super well versed in TF word embeddings. Think we don't have the prettiest logic there... 

@Rocketknight1 @gante what would you suggest here? 

Also cc @sgugger 

I confess I don't know much about TF word embeddings, so I'll have to dive deeper to review.

There is one thing I know, though -- it uses TF1 code, and it is on my update list 😅   

The embeddings in TF are using very very dark magic. I would refrain from any change in `modeling_tf_utils` (so let RemBERT fail for now) until it has been cleaned up by the TF team :-)

OK, so I will close this PR today without merge, if everyone is OK

@ydshieh it'd be great if you could open an issue (update TF embeddings) and link this closed PR to it

Closed for now with this issue #17540 opened.