noamaanMulla-03/transformers-issues huggingface dataset:
Fix integration with Accelerate and failing test
Author: muellerzr
Pull request: yes

# What does this PR do?

This PR brings back the logic when gathering and calculating metrics, which was borked with https://github.com/huggingface/transformers/pull/24028. Proof is the fact that tests now pass that were failing related to the Trainer

Fixes # (issue)

Solves https://github.com/huggingface/transformers/issues/24391

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

As Sylvain is on vacation, cc @amyeroberts 


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

@muellerzr I'm not sure if it's this or the previous PR that is causing the issue, but it still seems to be hanging on moving the loss tensor to the cpu.

@winglian can you provide a reproducer?

I'll have to distill something down later, but I can confirm the issue happens on multi-gpu, but when using single gpu, it seems to work properly.

A repr will definitely be needed here, because so far at least all the official example scripts don't hang for me. (Though there was something with gradient accumulation fixed with https://github.com/huggingface/transformers/pull/24756). Ping me once you have that and I can take a look. (Though sooner is better so I can try and have it for the next release 😄 )
[InstructBLIP] Fix bos token of LLaMa checkpoints
Author: NielsRogge
Pull request: yes

# What does this PR do?

This PR adds a fix for InstructBLIP as discussed offline with @gante.

The InstructBLIP models trained with Vicuna (LLaMa) checkpoints used inconsistent model/tokenizer files during training, hence the authors include [this line](https://github.com/salesforce/LAVIS/blob/4a85b17846ee62f09c40f37cc955dd33c2abec68/lavis/models/blip2_models/blip2_vicuna_instruct.py#L372) to fix this. This is not required for the models that use Flan-T5 checkpoints. cc @ArthurZucker 

However this made me wonder, let's say someone trains a new InstructBLIP model with LLaMa as language model, and which has the tokenizer and model's config properly set. Then the line introduced in this PR might not be what we want? cc @gante  



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

> However this made me wonder, let's say someone trains a new InstructBLIP model with LLaMa as language model, and which has the tokenizer and model's config properly set. Then the line introduced in this PR might not be what we want? cc @gante

@NielsRogge I don't think there's much more we can do: the model was probably trained with the incorrect assumption that `0` is the BOS token, in which case a post-generation fix is the only way to stay consistent with the original repo. 

(Have we double-checked that the results do change if we change the config's BOS token in the InstructBLIP config? If they don't, then we can simply update the config.)

I tried that but that doesn't fix it. Guess this is the only solution.

Just so I understand this fix, why doesn't updating the checkpoint's tokenizer's BOS token ID to resolve this? 

> why doesn't updating the checkpoint's tokenizer's BOS token ID to resolve this?

@amyeroberts TL;DR it's impossible to keep the same behavior as the original model with a config fix 💔 

### Full story

Here's what happens when we use InstructBLIP:
1. the tokenized prompt (starting with token = 2, from the [tokenizer BOS token](https://huggingface.co/Salesforce/instructblip-vicuna-7b/blob/main/tokenizer_config.json#L2)) is being passed to the custom `InstructBlipForConditionalGeneration.generate`. 
2. We compute its embeddings from `input_ids` and we pass it to the default `generate`. We do not pass `input_ids` to `generate`.
3. If we don't pass `input_ids` to `InstructBlipForConditionalGeneration.generate`, it is initialized with the default BOS token (from the model config) before embedding it.
4. The default BOS token in the model config is 0 ([source](https://huggingface.co/Salesforce/instructblip-vicuna-7b/blob/main/config.json#L97)), and not 2

As a result:
- If we fix the tokenizer such that BOS is 0 -> prompted input will diverge from the main repo, the first token is now 0 instead of 2 = different embeddings = different generation
- If we fix the model config such that the BOS is 2 -> unprompted input will diverge from the main repo, the first token is now 2 instead of 0 = different embeddings = different generation


Feel free to merge :)
Skip some slow tests for doctesting in PRs (Circle)CI
Author: ydshieh
Pull request: yes

# What does this PR do?

For doctest: each`.md` file is seen as a single test by pytest, and some takes more time (say `task_summary.md`) than others.
Let's allow a 5 minute timeout per test. 

For the job step, it still has the total 1200s timeout.

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

Hi!

- This file `.circleci/create_circleci_config.py` is running on CircleCI. See #23245
- If we don't accept the change is this PR: we can simply skip some (doc) test file that takes longer time to run)

- This PR addresses the per test timeout issue while there is a global timeout: motivated by #23318.
  - Usually I will try to respect the 120s  timeout (per test). But **since in this doctests on CircleCI (where we have 1200s global timeout), I think overall it's fine (?)**



Can we skip the longest tests? We are trying to rationalize the costs of circleCI so want to make sure we don't run something too beefy on it, especially since all those tests are run on GPU nightly.

> Can we skip the longest tests? 

Sure! We will need to have two lists: the exiting `utils/documentation_tests` and a new `slow_doctest_to_ignore`.

(I am not sure how to mark some doctest file as slow doctest as we have done for usual tests)

That works for me!

Me too! 

The latest version should skip the slow doctests.

(I don't really run the full doctest on CircleCI to get all of them: we just update the list if we see some files being slow by doctest)

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_24753). All of your documentation changes will be reflected on that endpoint.
Fix lr scheduler not being reset on reruns
Author: muellerzr
Pull request: yes

# What does this PR do?

This PR ensures that a new learning rate scheduler is created each time we rerun the `inner_training_loop`, so that if we have an lr such as `linear`, a new LR is generated based on the new batch size and step count. I don't believe a new optimizer is needed here to be recreated, just the scheduler as adjusting the bs and lr *shouldn't* matter? But if we think it is we can go ahead and add a reset to the optimizer as well.

Fixes # (issue)

The true solution to https://github.com/huggingface/transformers/pull/24521

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@sgugger


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_24758). All of your documentation changes will be reflected on that endpoint.
Allow existing configs to be registered
Author: sgugger
Pull request: yes

# What does this PR do?

If a model has a class defined both on the Hub and locally, there is a clash appearing when loading it in the auto API and `trust_remote_code=True` coming from [this line](https://github.com/huggingface/transformers/blob/253d43d46d1291633fb21116b737f2bd8799d3da/src/transformers/models/auto/auto_factory.py#L421). This PR fixes it.

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_24760). All of your documentation changes will be reflected on that endpoint.
:bug: Handle empty gen_kwargs for seq2seq trainer prediction_step function
Author: gkumbhat
Pull request: yes

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

All Trainers expose function `prediction_step`. For `Seq2SeqTrainer`, it seems like the `prediction_step` function is relying on availability of `_gen_kwargs` attribute, which will get set automatically, if `prediction_step` will get called from other functions like `predict` or `evaluate`. However, if someone calls `prediction_step` directly, then this field will not get set and currently will throw `AttributeError: 'Seq2SeqTrainer' object has no attribute '_gen_kwargs'`. 

In this PR, I am trying to resolve above issue by accepting `gen_kwargs` as an argument to `prediction_step` function, in addition to automatically get it from `self` if it has been set previously while falling back to empty `{}` in case its not set in either of those methods. 


## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.


cc: @sgugger

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Unpin protobuf in docker file (for daily CI)
Author: ydshieh
Pull request: yes

# What does this PR do?

I forgot to unpin protobuf (in the docker file) in my previous PR #24599.

Currently, CircleCI is testing against with protobuf 4, but daily CI  is still v3. Let's move on on daily CI too.

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Loading mT5 checkpoint will load from UMT5 class
Author: MattYoon
Pull request: no

### System Info

- `transformers` version: 4.31.0.dev0
- Platform: Linux-5.15.0-41-generic-x86_64-with-glibc2.31
- Python version: 3.9.16
- Huggingface_hub version: 0.14.1
- Safetensors version: 0.3.1
- PyTorch version (GPU?): 2.0.0+cu117 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: Yes
- Using distributed or parallel set-up in script?: No

### Who can help?

@ArthurZucker 

### 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

```Python
from transformers import AutoModelForSeq2SeqLM, AutoTokenizer

model = AutoModelForSeq2SeqLM.from_pretrained('google/mt5-small')
print(type(model))
#transformers.models.umt5.modeling_umt5.UMT5ForConditionalGeneration
```

### Expected behavior

@ArthurZucker Thank you for the recent integration of umT5. However, from the latest branch of transformers, loading normal mT5 will load from UMT5 class. Of course this does not happen with 4.30.2.

Comments:
cc @ArthurZucker 

Hey! Indeed one of our CI test is failing because of that. Looking into it now! 

Yep, the issue is that in the `CONFIG_MAPPING_NAMES` `umt5` maps to mt5 (since they have the same configuration file). This is messing with the overall mapping. A custom coming has to be create, or find a way to properly update! 😉 

Hmm. The values in `CONFIG_MAPPING(_NAMES)` is used as keys when creating `MODEL_MAPPING`. We should remove the entries of `umt5` in `CONFIG_MAPPING_NAMES` and other mappings.

Those models should be loaded in a non-auto way. 



We can't just remove every mapping, some of our checks and doc require them. Let's just add a config for UMT5.
lr_scheduler not updated when auto_find_batch_size set to True and batch_size decays
Author: thomas-schillaci
Pull request: no
Labels: name: bug

### System Info

- `transformers` version: 4.26.0
- Platform: Linux-4.18.0-372.26.1.el8_6.x86_64-x86_64-with-glibc2.28
- Python version: 3.9.16
- Huggingface_hub version: 0.12.0
- PyTorch version (GPU?): 1.12.1 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: <fill in>
- Using distributed or parallel set-up in script?: <fill in>

### Who can help?

@sgugger

### Information

- [X] The official example scripts
- [ ] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [X] My own task or dataset (give details below)

### Reproduction

# Issue:

When setting `auto_find_batch_size=True` in `TrainingArguments`, if the batch_size decays because batches can't fit in memory, the learning_rate scheduler won't get updated.

In my case, I finetune bert-base-cased on a custom dataset using:
```
training_args = TrainingArguments(
    per_device_train_batch_size=512,
    auto_find_batch_size=True,
    num_train_epochs=3
)
```
My batch_size decays three times, and the learning rate decays to zero before the end of the first training epoch.

# Trace:

Line 1162 of `trainer.py` is `if self.lr_scheduler is None:`, on its first call it evaluates to True, but when the batch_size decays, it is called again but this time it evaluates to False which prevents the lr_scheduler from being updated on line 1163.

I think we could replace it by:
`if self.args.optimizers[1] is None:`

### Expected behavior

If `max_steps` changes because the batch_size decays, the lr_scheduler should be updated to reflect this change.
Using the default linear lr_scheduler, I expect the learning rate to go from its initial value at the beginning of training to zero at the end of training.

Comments:
cc @muellerzr 

@muellerzr  I would like to pick up this issue and fix it, Looking to write a failing testcase for this bug, Any pointers ? 

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

It's been a while @muellerzr , could you have a look?

@raghavanone @thomas-schillaci could you try building from main and seeing if that fixes the issue? I think https://github.com/huggingface/transformers/pull/24521 fixed this

I'm not sure this fixes the issue, I'm going to comment on the PR directly

Same here. I built it from main.

just ran into this issue today, can confirm that it exists

Hi all, try reinstalling from main, #24758 should have fixed it this time

Hello @muellerzr, it works for me, thank you for the fix!
fix: "UserWarning: Creating a tensor from a list of numpy.ndarrays is…
Author: liucw2012
Pull request: yes

fix "UserWarning: Creating a tensor from a list of numpy.ndarrays is extremely slow. Please consider converting the list to a single numpy.ndarray with numpy.array() before converting to a tensor."

# What does this PR do?
reduce latency of codes below from  0.744675874710083s to 0.013312816619873047s.
```
st = time.time()
inputs = tokenizer(query_list, return_tensors="pt" ,padding=True)
print(time.time() - st)
```

Fixes #24764


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@ArthurZucker


Comments:
Fix eval_accumulation_steps leading to incorrect metrics
Author: muellerzr
Pull request: yes
Labels: name: trainer

name: bug

# What does this PR do?

Uses the logic in the `GradientState` to know when we've reached the end of training and should sync the gradients. 

Doing so relies on [this](https://github.com/huggingface/accelerate/blob/main/src/accelerate/accelerator.py#L862-L869) code, which already checks for the case of if a dataloader has no length and works properly

Fixes # (issue)
Solves https://github.com/huggingface/transformers/issues/24734

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@sgugger 

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Fix non-deterministic Megatron-LM checkpoint name
Author: janEbert
Pull request: yes

# What does this PR do?

`os.listdir`'s order is not deterministic, which is a problem when querying the first listed file as in the code (`os.listdir(...)[0]`).

This can return a checkpoint name such as `distrib_optim.pt`, which does not include desired information such as the saved arguments originally given to Megatron-LM.

Instead, we try out different file names used by Megatron-LM (`model_rng.pt` was mentioned in other parts of the script; I'm assuming this is for backward-compatibility).

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [X] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@pacman100 wrote most of the code in there and made a Twitter post about this functionality, hope you're the right person to tag. :)

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

Thanks for the review! Actually that's only the case if `--use-distributed-optimizer` is not given! Otherwise an extra file called `distrib_optim.pt` is created on the most recent Megatron-LM commit. :)

>Otherwise an extra file called distrib_optim.pt is created on the most recent Megatron-LM commit. :)

Cool, thank you for the info!