noamaanMulla-03/transformers-issues huggingface dataset:
Trainer accumulates logits
Author: Aktsvigun
Pull request: no

Hi,

I am using `transformers.Trainer` to pre-train a model with MLM. From line 2213 in `trainer.py` I can see the logits obtained on the evaluation step are accumulated: 
`preds_host = logits if preds_host is None else nested_concat(preds_host, logits, padding_index=-100)`

This makes it impossible to use a reasonable validation dataset, since already 1000 examples with `max_length = 512` and `vocab_size = 30522` occupy 1000 * 512 * 30522 / 1024^3 > 14 Gb of memory (e.g. **c4** dataset has validation dataset of size 365 000). This can be corrected if the additional metrics are calculated on each validation step for each batch separately rather than in the end.

This implies lines 2272-2275 should be moved inside the _for loop_. If you agree with all I have stated, I can do it by my own and come up with the merge request.

Comments:
@sgugger 

No, metrics usually can't be computed on a batch-per-batch basis as it usually gives the wrong result when the metric is not a mean (like precision, recall or F1 score).

For metrics in language modeling, you should use your own manual evaluation loop after training.

Fair enough, but if I do not need any metrics at all (need to only track
the loss value)? I still cannot use the validation sample since the logits
will be accumulated anyway.

On Mon, 26 Jul 2021 at 15:39, Sylvain Gugger ***@***.***>
wrote:

> No, metrics usually can't be computed on a batch-per-batch basis as it
> usually gives the wrong result when the metric is not a mean (like
> precision, recall or F1 score).
>
> For metrics in language modeling, you should use your own manual
> evaluation loop after training.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/transformers/issues/12878#issuecomment-886666573>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AIXZKXMPSQLGIY2ZROP7ZQ3TZVJRTANCNFSM5A6RXYNA>
> .
>


If you don't pass any `compute_metrics` function, they won't be accumulated, or you can force it with `prediction_loss_only=True` in your `TrainingArguments`.

Thanks a lot! Did not know about this parameter. To sum up, while pre-training a model, we need to write a custom evaluation loop to use custom metrics? Probably it is worth adding a special parameter for the metrics, indicating whether its computation can be split into batches or not? I can handle this.

Yes, we don't have anything setup in the Trainer for metric accumulation, so basically any time you want to avoid accumulating logits (so all language modeling tasks basically), you will need a custom training loop.

We'll try to come up with an API to make it easier to do a batch-by-batch accumulation but that probably will need a rewrite of some pieces of the Trainer, which in turn might cause some breaking changes. So it's probably going to be for v5

Has this BUG been fixed? Can evaluation be done during training?

Exactly, you can use `eval_accumulation_steps=1`
How to get input sentence embedding from Llama or Llama2?
Author: waterluck
Pull request: no

I'm trying to get the sentence embedding that I input, I checked some common practice to do it, but I'm not sure I'm doing the it right. Who may be help?  @gante  Thanks if you can be help. my code is as below: 

```
 model = LlamaForCausalLM.from_pretrained(
    args.pretrained_name_or_path,
    torch_dtype=torch.float16,
    device_map=device,
)
tokenizer = LlamaTokenizer.from_pretrained(args.pretrained_name_or_path, fast_tokenizer=True)
model.to(device)
model.eval()
tokenizer.pad_token_id = 0
tokenizer.padding_side = "left"

for i in range(0, len(sentences), batch_size):
    batch_sentences = sentences[i: i+batch_size]
    inputs = tokenizer(batch_sentences, padding=True, truncation=False, return_tensors='pt')
    inputs = inputs.to(device)

    with torch.no_grad():
        outputs = model(**inputs, output_hidden_states=True)
        hidden_states = outputs.hidden_states[-1]
        sentence_embeddings = hidden_states[:, -1, :]   # # here is using the **last token's** last layer hidden states as sentence embeddings,
        # or  sentence_embeddings = outputs.hidden_states[-1].mean(dim=1) # here use average sentence embedding. 
        # and I'm not sure which one is better.
        embeddings.append(sentence_embeddings.cpu())

embeddings = torch.cat(embeddings, dim=0)
```



Comments:
Hey @waterluck 👋 

Following our [issues guidelines](https://github.com/huggingface/transformers/blob/main/ISSUES.md), we reserve GitHub issues for bugs in the repository and/or feature requests. For any other matters, we'd like to invite you to use our [forum](https://discuss.huggingface.co/) or our [discord](https://discord.com/invite/hugging-face-879548962464493619) 🤗 If you still believe there is a bug in the code, check [this guide](https://huggingface.co/course/chapter8/5?fw=pt).

Since this is your first issue with us, I'm going to share a few pointers:
1. To get the best embeddings, models trained to embed the whole sentence should be used, not standard generative LLMs. See our [sentence similarity task page](https://huggingface.co/tasks/sentence-similarity) and check out the `sentence-transformers` repo
2. If you want to use an LLM, I'd say to average the hidden states for all the tokens, not simply for the last token (as you have in your commented code)

@gante Well noted the issue guidelines, I'll follow them next time! 
Also great thanks for your kindness in sharing which embedding should be used in by using LLMs. That's helped a lot!
kosmos_processor `sorted_length =sorted([(idx, len(x)) for idx, x in enumerate(text_encoding.input_ids)])` wrong?
Author: YamingZhang
Pull request: no

https://github.com/huggingface/transformers/blob/514de24abfd4416aeba6a6455ad5920f57f3567d/src/transformers/models/kosmos2/processing_kosmos2.py#L214C17-L214C17

The sorted code lacks a sorting function and should be changed to 
`sorted_length =sorted([(idx, len(x)) for idx, x in enumerate(text_encoding.input_ids)], key=lambda x: x[1])`
, is that correct?

Comments:
Mmmm yeah seems like it. Would you like to open a PR? 
fyi @ydshieh 

Sure, I'd be happy to help out

Hi @YamingZhang 

the latest version is

```
                sorted_length = sorted(
                    [(idx, len(x)) for idx, x in enumerate(text_encoding.input_ids)], key=lambda x: x[-1]
                )
```
see #27323.




> Hi @YamingZhang
> 
> the latest version is
> 
> ```
>                 sorted_length = sorted(
>                     [(idx, len(x)) for idx, x in enumerate(text_encoding.input_ids)], key=lambda x: x[-1]
>                 )
> ```
> 
> see #27323.

Thank you very much.
Memory leak on trainer's evaluation
Author: omermazig
Pull request: no

### System Info

I run a training process Based on [this](https://github.com/huggingface/notebooks/blob/main/examples/video_classification.ipynb) notebook, and I use a dataset of 8000 train videos. 


My inference process is memory-heavy. My val dataset's clip sampler is "random_multi", which means that for every video I take 5 clips of it, pass them to the model, and aggregate the results together for classification.

My problem is that there seems to be a memory leak in the evaluation process. I run on colab, and I can see that at each epoch, the memory usage stays the same throughout the epoch, goes up on evaluation, and stays there for the next epoch, and so on. So eventually after 2 or 3 epochs I'm running out of memory (I'm getting the generic "RuntimeError: Failed to load video after 10 retries", but I see the memory is full so I'm pretty sure that's the problem).

Is there an explanation for it? Is there something being saved **in memory** between epochs and if so, is there a way to release it? What should I do?

Thank you!

### Who can help?

@muellerzr and @pacman100

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

No way really. If it's needed I can supply my own notebook.

### Expected behavior

The memory usage should stay the same between epochs, and the memory needed for the evaluation process at the and of each epoch should be freed by the time the next epoch starts

Comments:
@omermazig is the notebook 1:1 that one, bar your dataset? Do you experience the memory leak occurring in that notebook as well? When testing locally, I couldn't indentify any memory leaks in the basic scripts that we have (like `run_glue.py`)

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.

Sorry for the late response. Apparently this assumption:

>  (I'm getting the generic "RuntimeError: Failed to load video after 10 retries", but I see the memory is full so I'm pretty sure that's the problem)

was wrong, and after I stopped working with a dataset from google drive, and copy it to the machine before training (instead of working with the path to google drive), this ceases to happen.
update Openai API call method
Author: Strive-for-excellence
Pull request: yes

# What does this PR do?


Fixes #27623


## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [ ] 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?
@ArthurZucker 


Comments:
This is also same problem in the AzureOpenAiAgent.
```
class AzureOpenAiAgent(Agent):
.....
    def _chat_generate(self, prompt, stop):
        result = openai.ChatCompletion.create(
            engine=self.deployment_id,
            messages=[{"role": "user", "content": prompt}],
            temperature=0,
            stop=stop,
        )
        return result["choices"][0]["message"]["content"]

```
However, since I don't have access to the AzureOpenAi API, I'm unable to test this code. Can someone do this job?
Possible bug for DPRContextEncoderTokenizer
Author: Hannibal046
Pull request: no

### System Info

- `transformers` version: 4.34.0
- Platform: Linux-4.15.0-213-generic-x86_64-with-glibc2.27
- Python version: 3.9.18
- Huggingface_hub version: 0.17.3
- Safetensors version: 0.4.0
- Accelerate version: 0.23.0
- Accelerate config:    - compute_environment: LOCAL_MACHINE
        - distributed_type: MULTI_GPU
        - mixed_precision: no
        - use_cpu: False
        - debug: False
        - num_processes: 4
        - machine_rank: 0
        - num_machines: 1
        - gpu_ids: 0,1,2,3
        - rdzv_backend: static
        - same_network: True
        - main_training_function: main
        - downcast_bf16: no
        - tpu_use_cluster: False
        - tpu_use_sudo: False
        - tpu_env: []
- 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?

@ArthurZucker
@younesbelkada


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

Hi, the original DPR code use `all zeros` for `token_type_ids` while HF version uses 0 and 1 (original BERT implementation) for tokens before and after `[SEP]` token.

https://github.com/facebookresearch/DPR/blob/a31212dc0a54dfa85d8bfa01e1669f149ac832b7/dpr/models/biencoder.py#L230-L231

### Expected behavior

give all zeros for `token_type_ids`

Comments:
@lhoestq Could you please check this?

Also, I found another mismatched point where original DPR would pad to `max length`(256) and insert an `[SEP]` token at the end:
https://github.com/facebookresearch/DPR/blob/a31212dc0a54dfa85d8bfa01e1669f149ac832b7/dpr/models/hf_models.py#L310-L315

Hello, would you like to open a PR for a fix? 😉 

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.
Code refactor for nested conditional statements
Author: YeonwooSung
Pull request: yes

# What does this PR do?

Simple code refactoring for "src/transformers/hf_argparser.py" and "src/transformers/image_utils.py".

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [v] 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.
- [v] 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:
> Hey! Would recommend you to install `ruff==1.5` to make sure some of the quality check pass. Not against refactoring as long as the CI are all green!

Thanks. Will definitely try this!

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

> hey! this doesn't seem to improve readability so won't accept it sorry!

Fair enough! Thanks. Will close this pull request, since this does not improve the readability
[example/recipe request] CodeLlama supervised fine tuning for infilling
Author: alexpeys
Pull request: no

### Feature request

I am interested in fine tuning CodeLlama 7/13B to do some infilling.

I have data that I want to fine tune on of the form: {prefix} {suffix} {middle}

I am trying to understand how to format my supervised fine tuning dataset.

Should I be setting up my fine tuning data as:
```
prompt="_<PRE> {prefix} _<SUF> {suffix} _<MID>", completion="{middle} ▁<EOT>"

or

prompt="{prefix} <FILL_ME> {suffix}", completion="{middle} ▁<EOT>"

or

some secret third thing
```
The documentation https://huggingface.co/docs/transformers/main/model_doc/code_llama#transformers.CodeLlamaTokenizer seems to suggest that the second will work and the tokenizer will do all the token healing etc... as it does for generation, but I am not sure.



### Motivation

Fine tuning for infilling is a little trickier than just standard PROMPT, RESPONSE fine tuning!

### Your contribution

Happy to write a basic example script if someone wants to guide me through the correct way to structure the prompt given the CodeLlamaTokenizer.

Comments:
Hey! Feel free to share your code on the hub! It's the best way I believe to share something like this 😉 You should even be able to write a `Community Blogpost` !

Regarding the format, the tokenizer will format the prompt 1. if you give it `"{prefix} <FILL_ME> {suffix}"`. The target probably needs what you mentioned: `"{middle} ▁<EOT>"`

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.

@alexpeys Hi... do you have any basic script or initial scribbled idea to achieve this? Am trying to finetune codellama for infilling task. It would be helpful.
memory efficient attention (Flash V2) initial support - encoder-only and not combined with relative attention
Author: YoelShoshan
Pull request: yes

# What does this PR do?

adds support for memory efficient attention, based on using xformers library memory_efficient_attention() op.

it only supports the following:
1. Encoder-only (no causal attention)
2. Relative attention (which gets injected into the "scores" (the results of Q@K.transpose()) is NOT supported, as it would case Flash v2 to not be used.



Comments:
[`WIP`] Multi-adapter saving support for PEFT
Author: younesbelkada
Pull request: yes

# What does this PR do?

To be potentially merged after https://github.com/huggingface/transformers/pull/26407 
This PR adds the multi-adapter support for `save_pretrained` to be consistent with PEFT API that saves all adapters when calling `save_pretrained`. Note the default adapter is always saved in the root directory of `save_directory`.

cc @LysandreJik @BenjaminBossan @pacman100 


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_26411). 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.

@younesbelkada What's the status?

@BenjaminBossan I think this might be too much of an edge case for the work it requires, I propose to keep that PR as open and if some interest arises from the community I'll work on 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.
Order of  `loss/gradient_accumulation_steps` and `backward(loss)`
Author: haoxiangsnr
Pull request: no

### System Info

Hi, @muellerz @pacman100 . I have a question about the gradient accumulation. Should we divide the `gradient_accumulation_steps` before `accelerator.backward(loss)`?

https://github.com/huggingface/transformers/blob/e3a4bd2bee212a2d0fd9f03b27fe7bfc1debe42d/src/transformers/trainer.py#L2787

It seems that `self.accelerator.accumulate(model)` [here](https://github.com/huggingface/transformers/blob/e3a4bd2bee212a2d0fd9f03b27fe7bfc1debe42d/src/transformers/trainer.py#L1889C17-L1889C57) is just a wrapper for `no_sync()` and will not automatically help divide the loss by the `gradient_accumulation_steps`.

### Who can help?

_No response_


Comments:
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.

Accelerate will also divide the loss if `accelerator.backward(loss)` is used, so it should already be done in https://github.com/huggingface/transformers/blob/e3a4bd2bee212a2d0fd9f03b27fe7bfc1debe42d/src/transformers/trainer.py#L2785.

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.
Extended semantic segmentation to image segmentation
Author: merveenoyan
Pull request: yes

This PR extends semantic segmentation guide to cover two other segmentation types (except for the big fine-tuning part) and compares them. cc @NielsRogge as discussed

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

I addressed all the comments. Sorry I deprioritized it shortly.

@MKhalusova can you merch if you approve? 👉 👈  🥹 

doc tests is still looking for `semantic_segmentation.md` although I've put a redirection 🧐 
<img width="591" alt="Screenshot 2023-11-20 at 17 42 47" src="https://github.com/huggingface/transformers/assets/53175384/49f5eab6-4198-44fc-ab90-7dd2909c60d1">


We need green CI checks before we can merge. It looks like the `utils/check_doctest_list.py` check is failing. You can run it locally to debug and fix. 

@MKhalusova it's my fault, I suggested to rename the file for consistency. I opened [this PR](https://github.com/merveenoyan/transformers/pull/1/files) to @merveenoyan's repo, which fixes the problem locally. It also fixes an issue with `check_task_guides.py`. Please, let me know if there's a better way to deal with this kind of situation :)

I also saw a `semantic_segmentation.md` in the `ko` translation, but refrained from renaming it because there's no `_redirect.yml` for Korean. Should we add it?

@pcuenca Generally, we tend to avoid renaming/removing files. I have no experience safely doing so, perhaps @stevhliu could advise? 

It's not a big deal, we can just go back to @merveenoyan's version before the rename if that's simpler.

Regarding redirects. For example, `perf_infer_gpu_many: perf_infer_gpu_one`. There is a chicken-egg situation because hf.co/docs uses [_redirects.yml that is on main currently](https://github.com/huggingface/transformers/blob/main/docs/source/en/_redirects.yml). Unless, this PR gets merged hf.co/docs for now will not redirect`perf_infer_gpu_many: perf_infer_gpu_one`

@MKhalusova according to Mishig's response we need to merge before it turns red and then it will be green, so maybe you can make the call in this case. 

There are two different things here.

- The redirection works for me in the docs generated for the PR, these both work:
https://moon-ci-docs.huggingface.co/docs/transformers/pr_27039/en/tasks/semantic_segmentation (old)
https://moon-ci-docs.huggingface.co/docs/transformers/pr_27039/en/tasks/image_segmentation (new)

- There are other CI scripts that fail because of references to the old name, as shown in this PR: https://github.com/merveenoyan/transformers/pull/1/files
These tests won't become green if we merge.

Given the increased complexity and that @MKhalusova said we generally try to avoid renames, I'd suggest we remove the rename and keep the same filename it had before. Sorry for introducing noise!

Can this be merged by someone with write access?

I can merge it :)
Update TVP arxiv link
Author: amyeroberts
Pull request: yes

# What does this PR do?

TVP on the doc page wasn't rendering the architecture image which highlighted two issues: 

* Link in HTML tag had been copied from TVLT. This PR updates to point to the TVP paper
* TVP architecture pic hadn't been added ([resolved here](https://huggingface.co/datasets/huggingface/documentation-images/blob/main/transformers/model_doc/tvp_architecture.png)) 


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