noamaanMulla-03/transformers-issues huggingface dataset:
Suggestions on Pipeline_webserver
Author: kihoon71
Pull request: yes

docs: reorder the warning tip for pseudo-code

# What does this PR do?

Fixed #25569, we modified the docs a little bit to clarify the suggested code is intentionally written.

## 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?


## Who can review?

@Narsil 

<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @

 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

Models:

- text models: @ArthurZucker and @younesbelkada
- vision models: @amyeroberts
- speech models: @sanchit-gandhi
- graph models: @clefourrier

Library:

- flax: @sanchit-gandhi
- generate: @gante
- pipelines: @Narsil
- tensorflow: @gante and @Rocketknight1
- tokenizers: @ArthurZucker
- trainer: @sgugger

Integrations:

- deepspeed: HF Trainer/Accelerate: @pacman100
- ray/raytune: @richardliaw, @amogkam

Documentation: @sgugger, @stevhliu and @MKhalusova

HF projects:

- accelerate: [different repo](https://github.com/huggingface/accelerate)
- datasets: [different repo](https://github.com/huggingface/datasets)
- diffusers: [different repo](https://github.com/huggingface/diffusers)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Maintained examples (not research project or legacy):

- Flax: @sanchit-gandhi
- PyTorch: @sgugger
- TensorFlow: @Rocketknight1

 -->


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_25570). All of your documentation changes will be reflected on that endpoint.
[docs] Improve dynamic batching example in pipeline_webserver.md
Author: kihoon71
Pull request: no

Hello @stevhliu,

I recently went through the dynamic batching section of [Using pipelines for a webserver](https://huggingface.co/docs/transformers/pipeline_webserver), and while I appreciate the clarity and details provided, I believe there are some potential pitfalls with the given code that might mislead less experienced users.

### Issue:
The example provided seems like pseudo code so it does not work on trial. 

the example code is below:

```python
(string, rq) = await q.get()
strings = []
queues = []
while True:
    try:
        (string, rq) = await asyncio.wait_for(q.get(), timeout=0.001)  # 1ms
    except asyncio.exceptions.TimeoutError:
        break
    strings.append(string)
    queues.append(rq)
strings
outs = pipe(strings, batch_size=len(strings))
for rq, out in zip(queues, outs):
    await rq.put(out)
```



### Suggestion:
Although it's made clear in the documentation that the code is optimized for readability and not efficiency, users might inadvertently use it as-is without considering the potential problems.
I would recommend updating the documentation with two suggestion.

First, i propose you to place the warning block above the code block and add explanation that the code is pseudo code.

Second one is to make the code at least working. so my suggestion for that is below :

Proposed Working Code:


```python
(string, rq) = await q.get()
strings = []
queues = []
while True:
    try:
        (string, rq) = await asyncio.wait_for(q.get(), timeout=0.001)  # 1ms
        strings.append(string)
        queues.append(rq)
    except asyncio.exceptions.TimeoutError:
        strings.append(string)
        queues.append(rq)
        break
outs = pipe(strings, batch_size=len(strings))
if len(queues) == 1:
    await queues[0].put(outs)
else:
    for rq, out in zip(queues, outs):
        await rq.put(out)
```

This way, we can find out the result at least on a trial and If you have better suggestion, please let me know!

Thank you for considering this feedback.

Best regards,




Comments:
cc @Narsil 

> Although it's made clear in the documentation that the code is optimized for readability and not efficiency, users might inadvertently use it as-is without considering the potential problems.

This is pretty much much why it is not functional code. It is intentional. You CANNOT copy mindlessly.
Fixing it will require a little bit of thinking, most likely to keep on reading and seeing the warnings.

I'm ok with moving around the warning/making it more prominent. As for making the code copy/pastable I'm not too sure we should.
Added missing parenthesis in call to is_fsdp_enabled
Author: marma
Pull request: yes

Calling function is_fsdp_enabled instead of checking if it is not None

# What does this PR do?
It adds the missing parenthesis

Fixes #25584

## Who can review?
@ArthurZucker @younesbelkada



Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_25585). All of your documentation changes will be reflected on that endpoint.
[`Docs` / `BetterTransformer` ] Added more details about flash attention + SDPA
Author: younesbelkada
Pull request: yes

# What does this PR do?

as discussed offline with @LysandreJik 

This PR clarifies to users how it is possible to use Flash Attention as a backend for most used models in transformers. As we have a seen some questions from users asking whether it is possible to integrate flash attention into HF models, whereas you can already benefit from it when using `model.to_bettertransformer()`, leveraging the `BetterTransformer` API from 🤗  optimum. 

The informations are based from the [official documentation of `torch.nn.functional.scaled_dot_product`](https://pytorch.org/docs/master/generated/torch.nn.functional.scaled_dot_product_attention.html?highlight=scaled_dot_product_attention#torch.nn.functional.scaled_dot_product_attention)

In the near future, we could also have a small blogpost explaining this as well

To do list / To clarify list:

- Clarify that it is possible to do that for training as well (I did not added much on the training section)
- Maybe add a few lines in overview of performance and scalability to emphasize this?

Let me know if I missed anything else

cc @fxmarty @MKhalusova @stevhliu 

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

Thanks a lot for the extensive review @stevhliu ! 🎉 

Thanks @fxmarty for all the reviews, @stevhliu this is ready for another pass !
MIssing parenthesis in call to is_fsdp_enabled?
Author: marma
Pull request: no

### System Info

- `transformers` version: 4.32.0.dev0
- Platform: Linux-5.15.0-1028-nvidia-x86_64-with-glibc2.10
- Python version: 3.8.13
- Huggingface_hub version: 0.16.4
- Safetensors version: 0.3.2
- Accelerate version: 0.21.0
- Accelerate config:    not found
- PyTorch version (GPU?): 1.12.0a0+8a1a93a (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?

 @ArthurZucker @younesbelkada

### Information

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

### Tasks

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

### Reproduction

I do not have an example, but rather I found this when debugging another issue. Therefore I do not have a testcase and I am *assuming* that the following is an error.

```
        if (
            (is_deepspeed_zero3_enabled() or is_fsdp_enabled) # <----- HERE
            and torch.distributed.is_initialized()
            and torch.distributed.get_rank() > 0
        ):
            map_location = "meta"
        else:
            map_location = "cpu"
```

### Expected behavior

I expect is_fsdp_enabled is intended to be called rather than tested if None

Comments:
PR here https://github.com/huggingface/transformers/pull/25585

Thanks!
Run doctest (in PRs) only when some doc example(s) are modified
Author: ydshieh
Pull request: yes

# What does this PR do?

Run doctest (in PRs) only when some doc example(s) are modified.

This is a fix for #23327 (which is reverted in #23371 due to the wrong logic) .

This PR implements the correct logic for

> for now the tests are launched on a file if we modify it, but I would only launch it if docstrings are modified (e.g. check the modifications are correct) to go faster.

where I go one step further to make it 

> only launch it if some _**doc examples**_ are modified`

(instead of any docstring)

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Replaces calls to `.cuda` with `.to(torch_device)` in tests
Author: vvvm23
Pull request: yes

`torch.Tensor.cuda()` is a pre-0.4 solution to changing a tensor's device. It is recommended to prefer `.to(...)` for greater flexibility and error handling. Furthermore, this makes it more consistent with other tests (that tend to use `.to(torch_device)`) and ensures the correct device backend is used (if `torch_device` is neither `cpu` or `cuda`).

This could be the case if `TRANSFORMERS_TEST_DEVICE` is not `cpu` or `cuda`. See #25506.

By default, I don't think this PR should change any test behaviour, but let me know if this is misguided.

# What does this PR do?

Replaces calls to `torch.Tensor.cuda()` with `.to(torch_device)` equivalents. This not only ensures consistency between different tests and their management of device, but also makes tests more flexible with regard to custom or less common PyTorch backends.

## 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?

This affects multiple tests an doesn't target any specific modality. However, they are all PyTorch models. @sgugger, hope you don't mind me tagging you again 🙂 


Comments:
Isn't this the case for a lot of other tests? They use the decorator `@require_torch_gpu` to skip the test if `torch_device != cuda`, so the tests will still only be run on GPU as they are meant to be.

I added `torch_device` into jukebox as I didn't see why it shouldn't be there if it is in basically every other test. If there is some extra behaviour or meaning I am missing, it would be helpful to know. In any case, the modifications to Jukebox are limited to two tests: one of which is skipped entirely anyway and the other is `test_fp16_slow_sampling` which I noticed does not have the `require_torch_gpu` decorator – only the `slow` decorator. I'll add one if you think it is worth it here 🙂 

Nice suggestions, I misunderstood what you meant initially by splitting into two lines. Hope it is all good now~

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

This should be good now 👍
[`split_special_tokens`] Add support for `split_special_tokens` argument to encode
Author: ArthurZucker
Pull request: yes

# What does this PR do?
Argument name is totally debatable. Will also require a pull request in `tokenizers`. 
The goal is to be able to simply activate and de-activate the special token splitting. Feature was asked in #22490, and is required for some production type cases, where users pass inputs and we don't want them to be able to hack them

Comments:
_The documentation is not available anymore as the PR was closed or merged._
[`Llama`] remove prompt and fix prefix finetuning
Author: ArthurZucker
Pull request: yes

# What does this PR do?

We are waiting for #25323 to be merged, but following the llama update we are going to remove the default system prompt.

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

cc @sourabh112 as I am not an expert in prefix tuning, there was something reported in the llama repo: https://github.com/facebookresearch/llama-recipes/issues/47#issuecomment-1674492457
[`config.to_dict()`] update test and models that have a composition
Author: ArthurZucker
Pull request: yes

# What does this PR do?

Adresses #23876

Most of the configurations that are `composition` are not json serialzable. 
- A test exists, but is usually not run (`ConfigTester` scarcly used on the full config) 
- The test does test the case when `torch_dtype = torch.float16`

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

3 model config that were not tested need a better update to pass the `test_config`, will work on this once I am back from lunch! 
- blip2
- pix2struct
- clap

Closing as it was fixed
[Time series Informer] fix dtype of cumsum
Author: kashif
Pull request: yes

# What does this PR do?
Fix an issue when training Informer with FP16, the `cumsum` returns float32. 

See report here: https://discuss.huggingface.co/t/how-to-train-on-multiple-gpus-the-informer-model-for-time-series-forecasting/48984/3

Comments:
_The documentation is not available anymore as the PR was closed or merged._
`.to_dict` does not correctly serialize `torch.dtype` in some cases (e.g., vision models)
Author: xenova
Pull request: no

### System Info

- `transformers` version: 4.29.1
- Platform: Windows-10
- Python version: 3.8.3
- 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?: no
- Using distributed or parallel set-up in script?: no


### Who can help?

@sgugger @ArthurZucker @amyeroberts

### Information

- [ ] The official example scripts
- [X] 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

Running:
```python
import json
from transformers import AutoConfig
json_data = AutoConfig.from_pretrained('openai/clip-vit-base-patch16').to_dict()
json.dumps(json_data, indent=4)
```

Results in 
```
TypeError: Object of type dtype is not JSON serializable
```

---

I have identified this problem with the following models:
- `clip`
- `sam`
- `vision-encoder-decoder`

### Expected behavior

torch dtypes should be converted to a string. I believe this is due to these configs redefining their `to_dict` method, without calling `dict_torch_dtype_to_str` on the top-level object.

https://github.com/huggingface/transformers/blob/de9255de27abfcae4a1f816b904915f0b1e23cd9/src/transformers/models/clip/configuration_clip.py#L397-L408


Comments:
Hey! Indeed, the `PretrainedConfig` class calls `dict_torch_dtype_to_str`, and the `text_config` and `vision_config` inherit from it, so they work fine, indeed, the parent's `torch_dtype` attribute can be modified and we don't use `self.to_dict()` . Thanks for reporting 

The configs should be automatically tested IMO, this is currently note the case. It seems that for blip, only the text config is tested, which is why this does not fail. 10 models or more are concerned (mostly when `is_composition=True`. 

I'll open a PR to fix this 

Commenting to prevent it being closed as stale.

Yep, sorry I'll try to get to the original fix taking the comment into account!

This was fixed by #25237
fix z3 init when using accelerate launcher
Author: pacman100
Pull request: yes

# What does this PR do?
1. Corrects the z3 init when using accelerate launcher. Follow up of https://github.com/huggingface/transformers/pull/25227.
It missed setting correct mixed precision in the DS Plugin leading to `RuntimeError: output tensor must have the same type as input tensor` when using Accelerate launcher to run DeepSpeed ZeRO3 training with Trainer. This PR fixes that issue. 

Comments:
_The documentation is not available anymore as the PR was closed or merged._
[`Idefics`] add image_embeddings option in generate-related methods
Author: leot13
Pull request: yes

Update Idefics generate-related functions to allow for precomputed image embeddings

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

>  Computing the encoder hidden states (i.e. the vision/perceiver hidden states in our case) inside the prepare... function seems curious to me.

Yes I'm not sure it is very standard. 
Maybe a more standard way to do it would be to do an encoder_decoder setting with 'encoder_outputs'. 
That would require adding another input 'encoder_outputs' on top of the image_encoder_embeddings/perceiver_embeddings + a get_encoder() function that would get the encoder and compute the embeddings. 
I think it would add a lot of unnecessary logic, so using _prepare_inputs() to prepare the inputs made more sense to me

And if it’s not too much work, a test calling generate to make sure this all works well 🤗 

I think there's a test here` tests/models/idefics/test_modeling_idefics.py`. Do you have something else in mind?


Okay `class IdeficsModelIntegrationTest(TestCasePlus)` already tests it thanks for checking! Feel free to merge
training with IterableDataset is very slow when using a large number of workers
Author: hjq133
Pull request: no

My problem is: 

I have 512 JSONL files (each 1GB) and stored on the Amazon Cloud. I have 32 GPUs. I read this big dataset with IterableDataSet recommended by Huggingface so that I can train on the fly.

`
dataset = load_dataset("json", data_files=data_files, storage_options=storage_options, streaming=True)
`


But when doing training I found that the speed was very slow, especially when I use many GPU workers . 
I found that the reason was because HuggingFace Trainer used DispatchDataloader to read Iterable DataSet.
It is the same with https://github.com/huggingFace/accelerate/issues/158.

Is there a good solution for my problem?
One of the solutions I think is to divide the 512 JSONL files to 32 GPU workers when building the dataset. So each GPU Worker only accesses the corresponding 16 JSONL file. it seems that I should shard the dataset manually in my code according to gpu worker id, and I should not pass the dataloader to accelerator.prepare().

Can HuggingFace Trainer currently support this way of data loading? Or is there any other way to deal with my problem.

Comments:
Hey, this kind of questions should be asked on [the forum ](https://discuss.huggingface.co/), as per the contribution guidelines.
bloom add_prefix_space= True
Author: Dongximing
Pull request: no

### System Info

Hi dear officer 
I use Bloom BloomTokenizerFast as a tokenizer. here is an issue.

Version =4.28.0
when I use BloomTokenizerFast, I find the add_prefix_space= True is not useful.
Here is the code. 
`tokenizer = BloomTokenizerFast.from_pretrained("bigscience/bloom",add_prefix_space = True)
print(tokenizer.add_prefix_space)
print(tokenizer("Hello world")["input_ids"])
print(transformers.__version__)
True
[59414, 8876]
4.28.0
`
here is other code. 
`from transformers import BloomTokenizerFast
tokenizer = BloomTokenizerFast.from_pretrained("bigscience/bloom")
print(tokenizer("Hello world")["input_ids"])
[59414, 8876]
`
I don't know why they will encode the same result. 

please have a look!
Thanks 

### Who can help?

@Arth

### Information

- [ ] The official example scripts
- [X] 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

It should encode different results. since add_prefix_space= True.

### Expected behavior

It should encode different results. since add_prefix_space= True.

Comments:
cc @ArthurZucker and @younesbelkada 

@younesbelkada @ArthurZucker 

cc @ArthurZucker as he is more familiar than me regarding tokenizers

Hey! Thanks for opening this issue. This is half a `tokenizers` issue ( even if you save the tokenizer and modify the `tokenizer_config.json` to set `add_prefix_space=True` in the `pre_tokenizer` the outputs are the same) and half a transformers issue (setting `add_prefix_space=False` and then saving does not change the value saved!) 

Will try to fix it 👍🏻 

Very nice catch, opening a fix right now! There is an issue with sequence pre_tokenizers!
25237 needs a follow up work
Author: stas00
Pull request: no

Looks like the modified `check_config_can_be_init_without_params` in https://github.com/huggingface/transformers/pull/25237 broke the yet unmerged https://github.com/huggingface/transformers/pull/24796

I patched it to skip these tests here so that we could merge it here https://github.com/huggingface/transformers/pull/24796/commits/906485b364e663221199a635a88a8f8814be988c
but most likely this needs to be fixed yet.

FWIW, this part doesn't fail:

```
    def check_config_can_be_init_without_params(self):
        config = self.config_class()
        self.parent.assertIsNotNone(config)
```

The updated test in this PR expects the config class to fail if it's a composition, but in the case of Idefics it succeeds (not sure why it should fail).


Comments:
resolved in https://github.com/huggingface/transformers/pull/24796/commits/2e37e6c8b31a9e03f4298f34bc66dedbd7cb997c