noamaanMulla-03/transformers-issues huggingface dataset:
Flax T5 models (at least) should use scan over layers technique
Author: colehaus
Pull request: no

### Feature request

See the technique described [here](https://docs.kidger.site/equinox/tricks/#improve-compilation-speed-with-scan-over-layers) and [here](https://github.com/google-research/t5x/blob/main/t5x/examples/scalable_t5/README.md#scan-over-layers). The essence is using a JAX `scan` instead of a python loop to iterate over layers that have the same structure.

### Motivation

The scan over layers technique allows JAX to "see" that the computational structure of each iteration is the same. This can dramatically reduce compile time and also system memory occupied by the JAX compilation cache (i.e. I believe if you have 25 layers in a model, the naive approach will end up with ~25 times as much JIT-compiled code since each layer will result in duplicative output code). My handwritten T5-like model uses ~1/50th of the system memory of the `transformers` Flax T5 models of similar size. It's easy to get system OOM errors with the current Flax implementation if you end up with multiple versions of the model compiled for different sequence lengths.

### Your contribution

It's possible I could submit a PR for this at some point in the future, but I can't be certain.

Comments:
cc @sanchit-gandhi 

Hey @colehaus! Sorry for the late reply here. We've currently decided not to implement `scan` for the Flax models in Transformers. You can see a brief reason for this here: https://github.com/huggingface/transformers/pull/24587#discussion_r1347737942

Happy to re-open the conversation if you feel strongly about this! There was a WIP PR that shows how this could be done generally for Transformers models here: #18341

But currently I tend to view `scan` as a specific feature that can be built on top of the Transformers library by advanced users who require it.

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.
GPT2 tokenizer decode swallows space
Author: daniel-ziegler
Pull request: no

### System Info

- `transformers` version: 4.25.1
- Platform: Linux-5.15.0-56-generic-x86_64-with-glibc2.35
- Python version: 3.9.15
- Huggingface_hub version: 0.11.1
- PyTorch version (GPU?): 1.10.2+cu113 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): 0.4.1 (cpu)
- Jax version: 0.4.1
- JaxLib version: 0.4.1
- Using GPU in script?: no
- Using distributed or parallel set-up in script?: no


### Who can help?

@ArthurZucker 

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

```python
from transformers import AutoTokenizer

tokenizer = AutoTokenizer.from_pretrained("gpt2")
tokens = tokenizer.encode("y is ?")
print(tokens)  # prints [88, 318, 5633]
print(tokenizer.decode(tokens))  # prints "y is?" (wrong!)
```

### Expected behavior

It should roundtrip back to the same string, matching the behavior of OpenAI's reference implementation: https://github.com/openai/gpt-2/blob/master/src/encoder.py

OpenAI's implementation encodes to the same tokens, but correctly decodes them to `"y is ?"`

Comments:
Hi, @daniel-ziegler use `clean_up_tokenization_spaces=False` in `decode` so the code will be - 
`print(tokenizer.decode(tokens, clean_up_tokenization_spaces=False))` 
this solves the problem,

thanks, 
susnato.

Indeed! Thanks for jumping this quickly @susnato 🤗

Thanks, that does work. I still think it's a bug that that isn't the default behavior for reversible tokenizers like GPT2's -- there's only one standard decoding behavior for them and it should be the default.

@daniel-ziegler, I think it's due to the reason that most tokenizers don't preserve the structure such as spaces, and the huggingface team didn't want to have different implementations for both type of tokenizers (which will make the code more complecated!), so it's True by default.

@ArthurZucker When LLMs are used to generate code, keeping `clean_up_tokenization_spaces=True`  can often 
lead to uncompilable code being generated because of the [clean_up_tokenization()](https://github.com/huggingface/transformers/blob/c48787f347bd604f656c2cfff730e029c8f8c1fe/src/transformers/tokenization_utils_base.py#L3798) method. I wanted to know if there are any pitfalls in using `clean_up_tokenization_spaces=False` ?

I have not conducted experiments but overall would just not recommend using GPT2 model and rather Llama. 🤗 

@ArthurZucker Will the same issue not persist in Llama as well? If the next token to be generated is between two quotes, it can still give an error when `clean_up_tokenization_spaces=True`. (`string var1 = "abc"` vs `string var1 = "abc "`)

No it's not the same tokenizer so I don't think it will have the same behaviour
Allow gradient for generate()
Author: whitejeep600
Pull request: no
Labels: name: Feature request

### Feature request

The generate function is decorated with @torch.no_grad() and thus can't be used for model training. It would be better to make calculating gradients optional, rather than impossible, so that the function can be used for tuning. The simplest solution is to remove the decorator altogether, as users can set no_grad themselves before calling if they need to. Are there reasons to disable such usage?

### Motivation

Allow using generate for tuning

### Your contribution

Removing the decorator is a very simple change. I can submit a PR

Comments:
The reason is that generate is for inference only, training requires a custom sampling logic 

> The reason is that generate is for inference only, training requires a custom sampling logic

Thank you for the quick reply.

However, what if I would just like to use the existing sampling algorithms from the generate function? It's a valid training strategy. It would be convenient to have access to the ready-made code for this purpose.
Adding truncation to text-generation pipeline
Author: farzadsbakhtiar
Pull request: no
Labels: name: Feature request

### Feature request

Passing along the `truncation` argument from the `text-generation` pipeline to the tokenizer. 

### Motivation

If you're using a `text-generation` with input text from the user it is likely that their input text is too long. This doesn't cause problems for some models, e.g. `t5` based, but for other models, e.g. `BERT` based it raises and Index Error.   
The workaround is to use the tokenizer and model manually and omit the pipeline. #25994 solves this issues for `fill-mask` pipelines.  

### Your contribution

#27683. I added `tokenizer_kwargs` to the text-generation pipeline's `__call__()`. This is similar to #26234 that did this for `fill-mask` pipelines. I personally think it's better to add `truncation` and `max-length` as a top level arg to the pipeline's constructor and pass it to the tokenizer when it's time.  
I think a refactoring of text based pipelines wouldn't hurt to unify tokenizer calls so you don't have to code this feature for each pipeline separately. 

Comments:
Hey! Sounds good, actually passing tokenizer arguments would be great in general, and better to have full args than kwargs. 
One of the issue with this is that it does not really go well with the hierarchy because this means we cannot pass custom argument of custom tokenizers, but we could just have the args that are in the __call__ + some custom_tokenizer_kwargs for specific tokenizers

Are you thinking something like this?
```
outputs = pipeline(docs, truncation=True, ..., custom_tokenizer_kwargs={'eos_token_id': ...})
```

Which tokenizer args should we add as args to this? Thinking about [not] making the function prototype too big...

Prototyping might be better than just supporting any kwargs! The relevant arguments depend on the task, text generation for example would need padding, max_length, add_special_tokens etc. 

👍 . Will push a PR next week!
Docs: Clarify stride for upcoming token classification pipeline
Author: adrianeboyd
Pull request: no

I just tried out the upcoming `stride` option for token classification pipelines (#21771, very useful!) without being familiar with the non-standard use of `stride` in the underlying tokenizer settings.

I think it would be helpful to also explain in the pipelines API documentation that the `stride` parameter sets the overlap and not the stride. I thought it was the stride and spent a while trying to figure out why the performance was so abysmal.

Comments:
Sounds like something missing indeed. Would you like to open a PR with such documentation?

I'm not confident that I could hit the style that you're looking for in your docs, especially given the history behind the naming.

It might be a lot simpler to document if `stride` were renamed, though, would you potentially consider renaming it for `TokenClassificationPipeline`?

cc @Narsil what do you think?

Indeed the name `stride` is not particularly well chosen, my oversight on this.

Seems we have the same thing in question answering: https://github.com/huggingface/transformers/blob/main/src/transformers/pipelines/question_answering.py#L361
And here: https://github.com/huggingface/transformers/blob/main/src/transformers/pipelines/automatic_speech_recognition.py#L169

I think controlling the overlap is much better in general since when you sending text (or audio) you have no idea of the max_length of the truncated texts, so controlling real stride would mean requiring arithmetic with that maximum size. (stride = tokenizer,model_max_length - overlap)

Given the history of that parameter I'm not sure what we should do. Documenting it better would be a start.

Renaming would warrant a rename if those 2 other pipelines. My current off the bat feeling is that we simply shouldn't. It's ok if it just means something different than for the convolution operator.

For the name `stride`, I choose the same as mentioned for tokenizers: 
stride (int, optional) — The length of the previous first sequence to be included in the overflowing sequence

This parameter is directly passed through the tokenizer in the `preprocess()` method. We can change the name of course, but to keep consistency throughout the documentation, it's better to change all names related to `stride` which in fact refer to the number of overlapping tokens from the previous chunk/sequence.

@luccailliau @amyeroberts 
It seems like an underkill TBH; why not add an alias and deprecate this param name, i.e., renaming it to 'window_overlap'?
This does create very unexpected bugs as people assume it means 'stride.'
Allow backbones not in backbones_supported - Maskformer Mask2Former
Author: amyeroberts
Pull request: yes

# What does this PR do?

Updates configuration creation to allow backbone configs to be pased in which aren't listed in `backbones_supported` - downgrading the exception raised to a warning. 

Tested with:

```python
from transformers import (
    Mask2FormerConfig, 
    MaskFormerConfig, 
    Mask2FormerModel, 
    MaskFormerModel, 
    FocalNetConfig
)

# Test with a backbone not officially supported
backbone_config = FocalNetConfig(out_indices=(-2, -1))

maskformer_config = MaskFormerConfig(backbone_config=backbone_config)
model = MaskFormerModel(maskformer_config)

mask2former_config = Mask2FormerConfig(backbone_config=backbone_config)
model = Mask2FormerModel(mask2former_config)
```

Fixes #24244


## 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?


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

Hi @amyeroberts, thanks for this feature.
Once Mask2FormerModel is instantiated using e.g. a FocalNetConfig or a SwinConfig as its backbone_config, is it possible to load pre-trained weights from one of the corresponding image classification models (e.g. FocalNetForImageClassification or SwinForImageClassification)?

Hi @matteot11 - not yet! But it's in the works: #28214
Fix error in M4T feature extractor
Author: ylacombe
Pull request: yes

# What does this PR do?

Really small PR that fixes an error when calling the SeamlessM4TFeatureExtractor without attention mask. I simply added a check to verify if the attention mask exists before operating on it. I've also modified the test suite to test this in the future.

cc @amyeroberts or @ArthurZucker ! WDYT ?


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

Hey @amyeroberts, thanks for the quick review and the relevant feedback, I've modified the logic a bit, WDYT ? There's also a pending comment on testing for which I'll wait your comment!
Enhancing Code Readability and Maintainability with Simplified Activation Function Selection.
Author: hisushanta
Pull request: yes

This code optimization enhances code readability and maintainability by utilizing aliases, simplified activation function selection, and consistent function definitions.


## Before submitting
- [x] 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?
- [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.
- [x] 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?



Comments:
Yes, because it is defined once globally and can be used throughout the entire codebase, improving code clarity and reducing redundancy.




Sorry 🤗 I mean that these are choices and we chose to have `nn.functional` rather than `F` which for us is less readable 

Thank you for your feedback. I understand that you prefer using nn.functional in your codebase. I chose to use F in this instance for two reasons:

**Conciseness:** Using F is more concise and reduces code clutter, especially when using multiple activation functions.

**Global accessibility:** Defining F globally makes it accessible throughout the codebase, eliminating the need to import nn.functional every time it's needed.

If you approve of my changes, I'd be grateful if you could review them.

Sorry but no, I think we just disagree on this matter but it's alright 🤗 

I understand and respect your position, even though we have different perspectives on this. 
Thanks for considering my input.

If it looks like's good then please merge them.

please can you review this PR?

Hey, as I mentioned before, this is not planned 🤗 

If these other changes are ok.

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

While I wouldn't propose reverting all my changes at this point, I understand some revisions might be needed. Would it be beneficial to close this pull request and open a new one with the refinements we discussed?
Add discriminator to vits
Author: ylacombe
Pull request: yes

# What does this PR do?

Vits is a TTS model that have been supported in HF for a few months now. MMS-tts, Meta's models on a thousand languages, also uses VITS architecture.

Vits is a peculiar model, a GAN, a VAE a flow-based, all at the same time. To train it, one needs a discriminator, which this PR adds to Transformers.

**This PR aims to add VitsDiscriminator and a dedicated model for training VITS. PR #27244 will then add a training script example.**

Main changes:

- adding VitsDiscriminator
- adding VitsModelForPretraining
- allows passing multiple speaker_id, instead of a single one. Works if nb_speaker_id = batch_size
- weight_norm functions - to keep aligned to the original code
- Resizing the speaker embeddings feature
- I haven't touched VitsModel except for the handling of multiple speaker id in the forward method.

cc @amyeroberts and @sanchit-gandhi ! This is a separate PR as requested!

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_27340). All of your documentation changes will be reflected on that endpoint.

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.
[tests] Check device failed in test_small_model_pt_bloom_accelerate
Author: yuanwu2017
Pull request: no

### System Info

transformers              4.37.0.dev0
pytorch                   2.1.2           py3.9_cuda11.8_cudnn8.7.0_0    pytorch
pytorch-cuda              11.8                 h7e8668a_5    pytorch
pytorch-mutex             1.0                        cuda    pytorch
torchaudio                2.1.2                py39_cu118    pytorch
torchtriton               2.1.0                      py39    pytorch
torchvision               0.16.2               py39_cu118    pytorch
accelerate                0.25.0

### Who can help?

_No response_

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

 When running the following case on multi-cards server with devcie_map-auto, It will not always be allocated to device 0,
Because other processes may be using these cards. It will select the devices that can accommodate this model.
pytest tests/pipelines/test_pipelines_text_generation.py::TextGenerationPipelineTests::test_small_model_pt_bloom_accelerate


Error log:
FAILED tests/pipelines/test_pipelines_text_generation.py::TextGenerationPipelineTests::test_small_model_pt_bloom_accelerate - AssertionError: device(type='cuda', index=7) != device(type='cuda', index=0)

### Expected behavior

If just testing whether the model works, It doesn’t need to strictly check the device, because it may eventually be loaded to the CPU due to insufficient GPU memory.

Comments:
Hey! Sure, would you like to open a pr for a fix?
fix bug:divide by zero in _maybe_log_save_evaluate()
Author: frankenliu
Pull request: yes

Hi, everyone.
**This is a new clean PR for previous PR #28102.**
set logging_strategy="steps" and logging_steps=10,
when that one epoch have 100 steps, the should_log will be set to True in last step.
And self._globalstep_last_logged will be assign to self.state.global_step in _maybe_log_save_evaluate() method by line 1917 in trainer.py.
the line 1933 in trainer.py , self.callback_handler.on_epoch_end() will keep the should_log=True, then in line 1934 run _maybe_log_save_evaluate() method (self.state.global_step - self._globalstep_last_logged) will be zero in line 2247.

@muellerzr @pacman100


Comments:
> Thanks for fixing @frankenliu !
> 
> For future PRs, could you make sure to link to the relevant code in the PR description? This will make it quicker and easier to review.

Okay !
Add flag for easily finetuning heads / linear probing to AutoModelforSequenceClassification
Author: 0amp
Pull request: no
Labels: name: Feature request

### Feature request

Previous work has shown that last layer linear probing is cheaper and often generalizes better than normal finetuning (see [1](https://arxiv.org/pdf/2202.10054.pdf)). I imagine this could be implemented as a flag to AutoModelforSequenceClassification so that only the last layer classification head is trained. I believe this can be done manually by setting all the parameters except the last one to not track gradients, but a flag may be easier and encourage adoption. 

It may also be nice to have linear probing available at earlier layers (eg halfway through the model). This could be done through using the output_hidden_states flag during a forward pass. Mid layer linear probing can occasionally be more effective and is a widely used technique in the interpretability literature (see [2](https://direct.mit.edu/coli/article/48/1/207/107571/Probing-Classifiers-Promises-Shortcomings-and), [3](https://proceedings.neurips.cc/paper_files/paper/2019/file/159c1ffe5b61b41b3c4d8f4c2150f6c4-Paper.pdf), [4](https://arxiv.org/abs/2311.03658#:~:text=Informally%2C%20the%20%27linear%20representation%20hypothesis,directions%20in%20some%20representation%20space.), [5](https://arxiv.org/abs/2310.01405), and many others). 

Alternatively, if this were implemented generally for AutoModel (or maybe AutoModelforCausalLM?), it could use a wider variety of models. This feature could also be paired with an update that automatically allows models to be used for sequence classification by appending a final linear layer. 

### Motivation

Finetuning a head is extremely memory efficient and extremely fast (order of 1k parameters for most models, linear probes generally train in seconds, the main bottleneck will just be the forward pass) and oftentimes performs close to finetuning for classification tasks. It has also been shown to perform better OOD. 

### Your contribution

I can provide feedback and testing, have not looked deep enough to know how to fully implement this

Comments:
[docs] Fused AWQ modules
Author: stevhliu
Pull request: yes

Streamlines the quantization docs a bit with the latest AWQ-fused module update :)

- I think the reintroduced Benchmarks in the AWQ section is a duplicate because it's already currently at the end of the doc. I moved the Benchmarks to the end because I think comparing the results of all the quantization schemes makes more sense after having read through them. Looking at the benchmarks now in the AWQ section, you don't have as much context about the other quantization schemes (AutoGPTQ, bitsandbytes), making it a bit more difficult to compare IMO.
- condense the fused/unfused code with the code-switching feature in the docs
- try to improve the table titles of the fused/unfused benchmarks

Comments: