noamaanMulla-03/transformers-issues huggingface dataset:
[`PixtralLarge`] Update Pixtral conversion script to support large format!
Author: ArthurZucker
Pull request: yes

# What does this PR do?
Updates the conversion script

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_34801). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

This should be just about ready! Quick summary of the changes:

- Made sure eps values and activations were handled correctly during conversion
- Make sure the tokenizer gets special tokens assigned correctly during conversion
- Make biases in the multimodal processor a config flag (this is enabled in Pixtral-12B but not in Pixtral-Large)
- `BatchMixFeature.to()` was buggy when the input was a nested list (changes pulled in from #35063)
- `PixtralProcessor` made some strange assumptions when lists are passed (changes pulled in from #35107)
- Some `float32` upcasts in `Pixtral` attention to match the behaviour of the vLLM reference code (vLLM uses `xformers`, which has a custom kernel that does those computations internally in `float32`)
- The Pixtral template needed a lot of rewrites
  - System message handling including datetime
  - Undocumented behaviour in `mistral-common`: When a message has **exactly one text chunk**, plus one or more images, then the text is moved to the end, after the image tokens, even if this is not the order of the chunks passed in by the user. When there are multiple text chunks, we keep the order from the message. If we don't get this exactly right then model generations are garbage.

TODO:
- [x] Should we use `xformers` instead of manual `float32` attention? It would be more accurate + faster, but would add a dependency to the model.
- [ ] Make sure the conversion script still works for older Pixtral-12B.
- [ ] Make sure @zucchini-nlp is okay with the changes in the `Processor`.

This should be ready for final review @ArthurZucker! I did ablation testing and reverted some of the dtype changes in `modeling_pixtral.py` - the results seem good without them and performance/memory improves.
Pipeline: simple API for assisted generation
Author: gante
Pull request: yes

# What does this PR do?

Assisted generation + pipelines has a bad UX at the moment: the user must manually load the assistant model (and the assistant tokenizer, if applicable), which defeats the point of being simple to use.

This PR adds the ability to specify an assistant checkpoint at pipeline definition time -- the pipeline will take care of the rest 🤗 

⚠️ While the feature was added for all pipelines that call `.generate()`, I haven't added a test on all of them. Many pipelines don't forward kwargs properly to `.generate()` which makes testing this transparent [same output, similar runtime] feature hard -- the best way to confirm assisted generation is running is by passing incompatible flags to `.generate()` to make it crash.

Example usage
```py
from transformers import pipeline
import torch

pipe = pipeline(
    "text-generation",
    model="meta-llama/Llama-3.1-8B",
    assistant_model="meta-llama/Llama-3.2-1B",  # This extra line is all that's needed!
    torch_dtype=torch.bfloat16
)
pipe_output = pipe("Once upon a time, ", max_new_tokens=50, do_sample=False)
print(pipe_output[0]["generated_text"])
```


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_34504). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Thanks for the nice PR @gante! 

@Rocketknight1, can you give it a first look as the pipeline owner? Thanks!

@gante will this PR make it to next release? 🙏

Hi @LysandreJik 👋 
Can we try to merge this before the next release?
This will drastically shorten the code examples we used in the HF blog and social posts.
Thanks!

hi @gante, could you please merge when free? 🙏 

@Rocketknight1 PR comments addressed 🫡 Let me know if you are happy with the PR!

Assuming CI is green: do I need to ping more folks for review?

@gante yes, you can go ahead and merge this whenever you're happy!
Fix all output_dir in test_trainer.py to use tmp_dir
Author: nhamanasu
Pull request: yes

# What does this PR do?

Split from the PR: https://github.com/huggingface/transformers/pull/35243

Current `test_trainer.py` has so many direct specifications of `outpu_dir` to TrainingArguments.
I've replaced all of them with tmp_dir so that no unnecessary folders will be made under root dir.

The targets are:
- "./examples"
- "./regression"
- "./generation"
- "./test"
- "None"


### Just a random thought
The current CircleCI doesn't seem to include tests for trainer. Is it as intended?

## 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#create-a-pull-request),
      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).
- [x] 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.

@ydshieh @SunMarc

Comments:
@SunMarc @ydshieh 
Thank you for the valuable reviews!
I've fixed the lines pointed out to make them cleaner.

Thanks! One more nits: there is no content in `.gitignore` being modified. Could you avoid it showing up in diff? (or is it intentional?)

After this last point,  I will merge

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35266). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Thanks!
I noticed .gitignore changed too, but I'm not sure why it makes diff... (it's unintentional)

I'm watching the control characters and whitespaces, but I couldn't find any difference.

I copied .gitignore from newly cloned transformer repo's main branch, and somehow it fixed the unintentional diff!

Maybe it's related to LF/CRLF or file mode, but I'm not sure in the end...

no worry, will merge when CI fishing. Thanks again!

Only one test file changed in this PR and the change logic is not involving any real trainer's logic. Will merge
Exploit symmetry of covariance matrices for faster & more stable diagonalization during initialization of added embeddings  
Author: bohrium
Pull request: yes

Fixes # 34570

see https://github.com/huggingface/transformers/issues/34570

@Rocketknight1

Comments:
Hi @bohrium, this looks good but there are a lot of conflicts because you branched from an old commit of `transformers`!

You should pull the latest version of `main` and then either rebase this PR or close it and make a new branch starting from an up-to-date `main` branch. Once you do that all the issues with version numbers and merge conflicts etc. should go away!
VisualBert: Why isn't the pooler_output used to calculate the logits in VisualBertForQuestionAnswering?
Author: niktheod
Pull request: no

I noticed that in the source code of `VisualBertForQuestionAnswering`, the embedding vector that is used to calculate the logits is not the `pooler_output` that is returned from `VisualBertModel`, but rather the embedding representation of the last text token in `last_hidden_state` (also returned from `VisualBertModel`), as you can see in the [code](https://github.com/huggingface/transformers/blob/052e652d6d53c2b26ffde87e039b723949a53493/src/transformers/models/visual_bert/modeling_visual_bert.py#L1242) below:

        sequence_output = outputs[0]

        # TO-CHECK: From the original code
        index_to_gather = (
            index_to_gather.unsqueeze(-1).unsqueeze(-1).expand(index_to_gather.size(0), 1, sequence_output.size(-1))
        )
        pooled_output = torch.gather(sequence_output, 1, index_to_gather)

Why is that? Usually in similar models the `pooler_output` is used to calculate the logits, which comes from the [CLS] token in the beginning of the text.

Comments:
Since the comment indicates that it's from the original code, my guess is that's what the original authors used! Both the final token and the pooler are viable choices here.

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.
update code owners
Author: ArthurZucker
Pull request: yes

# What does this PR do?

Forgot about modular + when I review something no need for another review

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35576). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
Don't show warning for `inv_freq` buffers
Author: zucchini-nlp
Pull request: yes

# What does this PR do?

We shouldn't show warning about unexpected keys when loading the model, if the state dict contains any saved keys for `inv_freq`. It was working good until the last release, when we moved RoPE under base model for several architectures

Since we are sure the inv frequencies are always recomputed for over a year now, we can make assumption that any key for `rotary_embedding.inv_freq` can be disregarded if the buffer has any RoPE parameters. Another option can be to explicitly try to reconstruct buffers for each attn layer, but I am not sure if we can safely assume all models will have the key similar to `model.layer.{0}.self_attn.rotary_emb.inv_freq`. So I opted to remove anything RoPE related

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35255). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@ArthurZucker Currently I had this bug caught only on llama-2 checkpoints, llama-3 works well because it has no `inv_freq` in its state dict saved. Basically it can be any model changed by https://github.com/huggingface/transformers/pull, depending on how the model checkpoint is saved and whether it has `inv_freq` in its state dict. I don't think there is a way to list all model checkpoints from the hub

Simply loading a model from pretrained will work as a reproducer

```python
AutoModelForCausalLM.from_pretrained("meta-llama/Llama-2-7b-chat-hf")
```

(once ci is red 👿 )
More model refactoring!
Author: Cyrilvallez
Pull request: yes

# What does this PR do?

Add more model refactoring as per https://github.com/huggingface/transformers/pull/35235

This ends up refactoring only Cohere and Phi3 for now. Longrope refactor was needed to match original Phi3. After discussion with @gante, Phi3 was and still is the source of truth for longrope, thus the change applied. 

Other models will follow in a separate PR for simplicity.

Adding list here for simplicity:

- [x] Cohere
- [ ] Chameleon
- [ ] DBRX
- [x] Gemma
- [x] Gemma2
- [x] GLM
- [ ] gpt_neoX
- [x] GPT2
- [x] Granite
- [ ] Jamba
- [ ] JetMoe
- [ ] Mimi
- [x] Mistral
- [x] Mixtral
- [ ] Mllama
- [ ] Moshi
- [ ] Nemotron
- [ ] OPT
- [x] Phi
- [x] Phi3
- [ ] PhiMoe
- [x] Qwen2
- [ ] qwen2Moe
- [ ] qwen2VL
- [ ] SableML
- [x] StartCoder2
- [ ] Idefics1,2,3
- [x] Olmo
- [x] Olmo2
- [ ] Siglip
- [ ] Whisper


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35359). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
Re-add missing __all__ for Cohere and Phi3
Author: Cyrilvallez
Pull request: yes

# What does this PR do?

As per the title. They got lost somehow in the refactor.

cc @ArthurZucker 

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35578). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
is_causal arg appears twice in FAttention call from GPT2Attention.forward()
Author: poedator
Pull request: no
Labels: name: bug

### System Info

4.48.dev ubuntu18, py3.11

### Who can help?

@ArthurZucker based on #35235
@Cyrilvallez  based on  #35342

### Information

- [ ] The official example scripts
- [X] 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

use GPT2 with flash attn.

in https://github.com/huggingface/transformers/blob/608e163b527eaee41e650ffb9eb4c422d2679902/src/transformers/integrations/flash_attention.py#L47

it inserts `is_causal` argument twice, from kwargs and explicitely. causes `TypeError: transformers.modeling_flash_attention_utils._flash_attention_forward() got multiple values for keyword argument 'is_causal'`

the kwargs get `is_causal` from GPT2Attention.forward()


### Expected behavior

use `is_causal` just once

please add GPT2 to your release tests suite

Comments:
Hey @poedator, indeed, thanks for reporting this issue! I opened a PR [here](https://github.com/huggingface/transformers/pull/35390) to fix it!
add Cohere using modular transformers
Author: ArthurZucker
Pull request: yes

# What does this PR do?
Superseeds #31518

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_33247). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
Fix flaky `test_batching_equivalence`
Author: ydshieh
Pull request: yes

# What does this PR do?

I am the king serial killer of flaky tests in `transformers`!

The **the ratio of failures**:
- `tests/models/flaubert/test_modeling_flaubert.py::FlaubertModelTest::test_batching_equivalence`: 
  - 3000 runs: 
    - main: 1.2 %
    - PR: 0 %
- `tests/models/mobilevitv2/test_modeling_mobilevitv2.py::MobileViTV2ModelTest::test_batching_equivalence`: 
  - 300 runs: 
    - main: 2.3 %
    - PR: 0 %
- `tests/models/xlm/test_modeling_xlm.py::XLMModelTest::test_batching_equivalence`: 
  - 300 runs: 
    - main: 3.3 %
    - PR: 0 %

I will apply the same changes to overwritten tests wherever they are flaky too.

Comments:
@gante @zucchini-nlp 

For context: see previous similar fixes #34995

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35564). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

There are some model test cases overwrite the common one with `is_flaky`.
I decide to merge this PR first, and in a follow-up PR to try to remove those overwritten tests.
Fix flaky `SwitchTransformersModelTest::test_training_gradient`
Author: ydshieh
Pull request: yes

# What does this PR do?

Not all experts will get grad in a forward call.

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35586). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
feat: add TP plan for granite
Author: kmehant
Pull request: yes

# What does this PR do?

related to #34789

Most of the pieces fall in place since the architecture code inherits from Llama classes and overrides on top.

## 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#create-a-pull-request),
      Pull Request section?
- [x] 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?

@ArthurZucker and anyone from the community.

Comments:
`tokenizer` train from iterator without pre_tokenizers 
Author: ArthurZucker
Pull request: yes

# What does this PR do?

Fixes the issue reported in #35315.
CC @itazap 

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35396). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
`train_new_from_iterator()` does not work when pre_tokenizer is null
Author: cecheta
Pull request: no
Labels: name: bug

### System Info

transformers version 4.47.1
Ubuntu 20.04.6 LTS
Python 3.10

### Who can help?

@ArthurZucker, @itazap

### Information

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

### Tasks

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

### Reproduction

Follow the steps listed in https://huggingface.co/learn/nlp-course/chapter6/2, however use `microsoft/Phi-3.5-mini-instruct` as the model instead of `gpt2`.

```py
from datasets import load_dataset
from transformers import AutoTokenizer

raw_datasets = load_dataset("code_search_net", "python", trust_remote_code=True)

def get_training_corpus():
    dataset = raw_datasets["train"]
    for start_idx in range(0, len(dataset), 1000):
        samples = dataset[start_idx : start_idx + 1000]
        yield samples["whole_func_string"]

training_corpus = get_training_corpus()

old_tokenizer = AutoTokenizer.from_pretrained("microsoft/Phi-3.5-mini-instruct")

tokenizer = old_tokenizer.train_new_from_iterator(training_corpus, 52000)
```

```
Traceback (most recent call last):
  File "/home/azureuser/tokenizer.py", line 16, in <module>
    tokenizer = old_tokenizer.train_new_from_iterator(training_corpus, 52000)
  File "/anaconda/envs/myenv/lib/python3.10/site-packages/transformers/tokenization_utils_fast.py", line 819, in train_new_from_iterator
    or tokenizer_json["pre_tokenizer"]["type"] == "Sequence"
TypeError: 'NoneType' object is not subscriptable
```





















### Expected behavior

The tokenizer should be trained.

Comments:
Appears to have regressed in #33556

pinging @umarbutler @itazap from PR #33556

Closing as PR is merged!
Small fix rope kwargs
Author: molbap
Pull request: yes

# What does this PR do?

As per title, removing unused `rope_kwargs` for a few models post-refactor.

Comments:
ah, I also removed `_extract_stem_indices` from `MusicgenMelodyFeatureExtractor` documentation, because it's not a public method - I think I've seen this error before but no clue why it pops up again

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35589). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
Fix modular edge case + modular sorting order
Author: Cyrilvallez
Pull request: yes

# What does this PR do?

Fix a very niche edge-case discovered in https://github.com/huggingface/transformers/pull/35147: if the model name appear in a normal word (e.g. `sam` appears in the word `downsample`, then it is replaced by the new name). This is of course unwanted, and a simple check for previous char in the regex fixes it.

Also fix the `topological_sort` function in `create_dependency_mapping.py`: it is currently fully skipping some files that are silently not converted. I had a hard time finding the exact root, so I mostly rewrote it (I believe it is much simpler now), and it works nicely. I also added a check in `modular_model_converter.py` to ensure that we never silently skip files.

Finally, I added back the possibility to convert the examples. Note that this does not slow down `make fix-copies`, which I think was the reason to remove them, as they are not included in `check_modular_conversion.py` (and it's important that we are able to check on them as well). They will not be converted when doing `python utils/modular_model_converter.py --files_to_parse all` though, I added the keyword `python utils/modular_model_converter.py --files_to_parse examples`.

cc @ArthurZucker and @yonigozlan as you touched it recently as well

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_35562). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

I figured if I do that I will just rely too much on the current output, maybe not making it a good test 🙃 Plus as explained before, order is not important for most files