noamaanMulla-03/transformers-issues huggingface dataset:
Clean up vision tests
Author: NielsRogge
Pull request: yes

# What does this PR do?

This is a follow-up of #16799. It took me way too long to realize I don't need to overwrite `test_attention_outputs` and `test_hidden_states_outputs` as I can just set the `seq_length` attribute of the ModelTester. 😂 

Comments:
_The documentation is not available anymore as the PR was closed or merged._
[Trainer] Move logic for checkpoint loading into separate methods for easy overriding
Author: calpt
Pull request: yes

# What does this PR do?

This PR does a small refactoring in the `Trainer` class, specifically it moves the logic for the following two steps out of the training loop into separate helper methods:

- loading a pre-existing checkpoint into the Trainer before the training starts is moved into the `_load_from_checkpoint()` method.
- loading the best evaluated model checkpoint after training has completed is moved into the `_load_best_model()` method.

The PR does not change any existing logic in any way.

## Motivation

In [our library](https://github.com/Adapter-Hub/adapter-transformers), we implement a custom Trainer class that subclasses your great built-in Trainer class. However, as we don't save full model checkpoints during training, the mentioned steps for checkpoint loading are not applicable to our use case. Moving this logic to separate methods would be super helpful to us (and potentially others), since we could easily override these helper methods without modifying the training loop itself.

## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

 @sgugger

(cc @hSterz)

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Confusing interaction between training dataloaders and datasets in Trainer
Author: sanderland
Pull request: no

Trainer documentation suggests to override `get_train_dataloader` for custom behaviour. 

However, the `train` method does not respect this.

It directly goes back to `train_dataset`
```
train_dataset_is_sized = has_length(self.train_dataset)
```
And then it gets `num_examples` from `train_dataloader`, rather than the length it just checked.
```
# this accesses train_dataloader.dataset, which could very well not exist
num_examples = (
    self.num_examples(train_dataloader) if train_dataset_is_sized else total_train_batch_size * args.max_steps
)
```

I suggest changing it to something like
```
try:
  num_examples = self.num_examples(train_dataloader)
  # other code for train_dataset_is_sized being true before
except NameError: # no length
  # other code for train_dataset_is_sized being false before
  num_examples = total_train_batch_size * args.max_steps
```

This way one can override `num_examples` to handle more exotic dataloaders, even in the absence of a train_dataset property.

Happy to create a PR, just wanted to check if there is a good reason for the current setup.



Comments:
I'm not sure this is the only thing that would require some changes, but I'm happy to review a PR.

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.
Infer padding in GPT2ForSequenceClassification when inputs_embeds and attention_mask are given
Author: davidleonfdez
Pull request: no

# 🚀 Feature request

Infer padding in `GPT2ForSequenceClassification` when `inputs_embeds` and `attention_mask` are given

## Motivation

When `config.pad_token_id is not None` and `inputs_embeds` are passed instead of `input_ids`, the forward method of `GPT2ForSequenceClassification` raises a warning telling that _"GPT2ForSequenceClassification will not detect padding tokens in `inputs_embeds`. Results may be unexpected if using padding tokens in conjunction with `inputs_embeds.`"_.

This happens because you have nowhere to look for the padding tokens start index when `input_ids` is not given. As a consequence, the last position (of the tensor) is taken as the classification token.

In practice, one may need to pass a batch of `inputs_embeds` with different sequence lengths and padded and I think there are options to solve or workaround this.

## Your contribution

One alternative that comes to my mind is to infer the sequence lengths from the attention mask (only when we don't have `input_ids`)
For instance:
a) Optimistic approach: the sequence length is given by the position of the last 1 in the corresponding row of the attention mask
or
b) Not so optimistic approach: the sequence length is given by the position of the last 1 in the attention mask only if all the previous positions are 1's and only if this is the case for every row of the attention mask; in any other case, `sequence_lengths = -1` and the warning would be raised.

Examples for approach b):

```
attention_mask = [
   [1, 1, 1, 1, 1],
   [1, 1, 1, 0, 0]
]
```
=> We infer sequence lengths = [5, 3] (in the code I think it should be [4, 2] actually)

```
attention_mask = [
   [0, 1, 1, 1, 1],
   [1, 1, 1, 0, 0]
]
```
=> We don't infer anything, sequence lengths = [-1, -1] (in the code I think it's just `sequence_lengths = -1` as broadcasting happens)


For reference, the modification would be done here: https://github.com/huggingface/transformers/blob/8a69e023bf81160e64848b256441e64a4cb47992/src/transformers/models/gpt2/modeling_gpt2.py#L1406-L1411

Do you see it reasonable or is it too much of an assumption?

A safer alternative is to add a new parameter that is only used when `inputs_embeds' are passed, but I'm not sure it's justified enough just for this specific case.

I can submit a PR that implements this or a different solution.

Thanks in advance.

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.
Update no_trainer examples to use new logger
Author: muellerzr
Pull request: yes
Labels: name: PyTorch

name: Examples

# Update `no_trainer` examples to use the new Accelerate logger

## What does this add?

- Accelerate recently added a [new logger](https://github.com/huggingface/accelerate/pull/337/) to help deal with repeat logs across all processes. If it should be logged on all, a new kwarg `main_process_only=False` should be passed in. 

This helps also solve an annoyance users were pointing out about repeat logs leading to misunderstandings of how the internal API was acting on

## What parts of the API does this impact?

### User-facing:

The examples now show using the new `get_logger()` function from Accelerate

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

rereview for propagate sanity check then all good 🤗
Allow all imports from transformers
Author: LysandreJik
Pull request: yes

This PR enables doing `from transformers import *` when just the base `transformers` install is done. This should always have been possible, but due to some errors in the imports, it currently failed with a `sentencepiece` import error. This PR fixes that for both the FNet and CPM tokenizers.

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Clean up setup.py
Author: sgugger
Pull request: yes

# What does this PR do?

This PR cleans up a bit the README in two ways:
- Remove support for Python 3.6 as we said the current release was the last one with Python 3.6
- Clean up a bit the authors field, description and keywords to emphasize the multimodal support.

Since the Hugging Face team is growing, I propose to replace the authors field by something more generic than adding names, let me know if you have a better idea

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Adding to docs how to train CTRL Model with control codes. 
Author: ludoro
Pull request: no

# 🚀 Feature request

At the moment there is no explanation in the docs how to train a CTRL model with user defined `control codes`. 

## Motivation

At the moment there is no explanation in the docs how to train a CTRL model with user defined `control codes`. I think it should be added because control codes are an important part of CTRL model.

## Your contribution

 I am currently struggling on coming up with ideas on how to do that using transformer interface, but I'd love to open a PR after I understand how to do that.

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/master/CONTRIBUTING.md) are likely to be ignored.

Did anyone figure out how to do this?
Onnx support for Zero Shot Classification Pipeline
Author: subhamkhemka
Pull request: no

Hi

Thanks for creating this library.

I am using the Zero Shot Classification Pipeline for my use cases which runs for millions of sentences and I currently use a gpu for faster performance.

Onnx currently supports following pipelines-

SUPPORTED_PIPELINES = [
    "feature-extraction",
    "ner",
    "sentiment-analysis",
    "fill-mask",
    "question-answering",
    "text-generation",
    "translation_en_to_fr",
    "translation_en_to_de",
    "translation_en_to_ro",
]

It would be helpful if I could use onnx runtime to deploy the pipeline. Hence asking for support for Zero Shot Classification Pipeline.

Comments:
Hello @subhamkhemka, we're moving away from pipeline support in ONNX to support a more easily extendable setup relying on the model themselves. Could you take a look at the new approach and let us know if it works for you? https://huggingface.co/transformers/serialization.html#configuration-based-approach

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/master/CONTRIBUTING.md) are likely to be ignored.

@LysandreJik 
Zero Shot Classification is a little bit tricky, is there an example of ONNX configuration for Zero Shot Classification? Thanks
[FlaxBert] Add ForCausalLM
Author: sanchit-gandhi
Pull request: yes

# What does this PR do?
Adds cross-attention blocks to the following module classes:

- FlaxBertModule
- FlaxRobertaModule (in part through copying FlaxBertModule)
- FlaxBigBirdModule (in part through copying FlaxBertModule)
- FlaxElectraModule (in part through copying FlaxBertModule)


Adds the following ForCausalLM model classes:

- FlaxBertForCausalLM
- FlaxRobertaForCausalLM (in part through copying FlaxBertForCausalLM)
- FlaxBigBirdForCausalLM (in part through copying FlaxBertForCausalLM)
- FlaxElectraForCausalLM (in part through copying FlaxBertForCausalLM)

Adds the following model tests:

- FlaxRobertaForCausalLM
- FlaxBigBirdForCausalLM
- FlaxElectraForCausalLM

Note: FlaxBertForCausalLM is excluded due to the name mismatch with the PyTorch equivalent BertLMHeadModel. It is implicitly tested through the FlaxRobertaForCausalLM model tests, as well as in the following encoder-decoder model tests:

- Bert-2-Bert (encoder-decoder)
- Wav2Vec2-2-Bert (speech encoder-decoder)

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

> Looks good to me - @sanchit-gandhi could you check though which models don't pass with `1e-5` and ideally why?
> 
> Overall `4e-2` is fine for me though cc @ydshieh what do you think?

Keep `1e-5` is much better, because so far I can always find some issues when I find something higher than `1e-5` (well, sometimes it took quite some time to figure out)
Move test model folders
Author: ydshieh
Pull request: yes

# What does this PR do?

As discussed offline, this PR moves model specific test folders (e.g. `tests/bert`) to `tests/models` (e.g. `tests/models/bert`)

In addition to the necessary changes on `import`, the following changes are made:

- In some test files regarding processors (tokenizer/feature extractor, etc.), change
```
SAMPLE_ROBERTA_CONFIG = os.path.join(os.path.dirname(os.path.abspath(__file__)), ".../fixtures/dummy-config.json")
```
to
```
SAMPLE_ROBERTA_CONFIG = get_tests_dir("fixtures/dummy-config.json")
```
(see [the commit](https://github.com/huggingface/transformers/pull/17034/commits/ee9956cf4181932c821d4c3c28677ac33660496a))

- The changes (**to be reviewed particularly**)

  - `.circleci/config.yml`
  - `.github/workflows/self-scheduled.yml`

  - `src/transformers/commands/add_new_model.py`
  - `src/transformers/commands/add_new_model_like.py`

  - `utils/check_repo.py`
  - `utils/notification_service.py`
  - `utils/test_fetcher.py`

### Remarks:

  - The `self-push` result is [here](https://github.com/huggingface/transformers/actions/runs/2256959215)
    - The slack report job has `Artifact was not found, job was probably canceled.`, but this issue exists for some time. My plan is to continue the task of changing self-push report format (and fix this issue)
    - The `run_tests_flax_gpu` failure is just the same as in other runs. This is not in the scope of this PR.
  - The scheduled CI (partial) result is [here](https://github.com/huggingface/transformers/actions/runs/2254833118). The report is available on Slack.
    - On the GitHub Actions page, the jobs have name like `Model tests (models/albert, single-gpu-docker)`. It becomes a bit long (with `models/`).
    - Same for the Slack report
        ```
        0 |         0 |         3 |         0 |         0 | models_auto
        ```
    - So far I only ran a subset of models. From the results, I think the PR is ready. We can run a full suite of tests before merge.

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

Hi, @stas00 Thank you for the feedbacks.

- Regarding `difficult to read` (the long command): totally agreed! I had the same feeling, and thought might be a good idea to create a tiny python script and just call it. Otherwise, we can use what you proposed above (after some tests).
- About `parents[4]`: thank you for the information!
- I can check `TestCasePlus` later.

I would prefer to merge as it is now, and work on these points in another PR. The main reason is that I ran the full suite of tests, the results look all good, and would like to merge with a version that has been fully tested :-)

Merged now (after rebase on main for the merged `flax_bert` and `yolos` PRs).
Bug: Finetuning large models resume checkpoint error
Author: lorr1
Pull request: no

When finetuning a large model (e.g. Eleuther 6B), you shard the checkpoints upon saving [here](https://github.com/huggingface/transformers/blob/c79bbc3ba54a81dab2eac13d89f264ed64cb2460/src/transformers/modeling_utils.py#L193). However, upon resuming the checkpoint (and loading the best checkpoint after training), you confirm if there is a valid checkpoint assuming weights are no sharded [here](https://github.com/huggingface/transformers/blob/dced262409177586bb510b6b724c762fb89da0e8/src/transformers/trainer.py#L1196). This causes an error upon resuming training.



Comments:
Indeed, I saw that yesterday and am working on a fix.

Should be fixed by the PR mentioned above :-)

Thanks!!
Make Trainer compatible with sharded checkpoints
Author: sgugger
Pull request: yes

# What does this PR do?

The `Trainer` is currently incompatible with the new sharded checkpoint feature in two places:
- resuming from a checkpoint
- loading the best model at the end of training

In both cases, the model state dict is loaded back inside the model **but** there is no model save file if the model was above the default size for sharding, resulting in errors (as was pointed out by #16976 ).

This PR addresses this by:
1. Creating a new function `load_sharded_checkpoint` that does the same thing as `model.load_state_dict` for regular model files, but loads a sharded checkpoint (and errors in case of missing/unexpected keys when `strict=True`).
2. Use that function inside the Trainer in the two places mentioned above.

A test is added to make sure resuming works from a sharded checkpoint.

Fixes #16976 

Comments:
_The documentation is not available anymore as the PR was closed or merged._
Fix tokenizers pretrained saving/loading
Author: n1t0
Pull request: yes

Fix #5571

I managed to fix the saving part, but I don't really know what to do for the loading part (in `_from_pretrained`).

Comments:
Hey @n1t0, great job, saving works now! :)
Do you think `AddedToken` instances should be serialized with additional meta data? 

I managed to tweak JSON (de)serialization with custom hooks to process objects properly but it is difficult to infer the deserializing type from a single set of attributes, passing `__class__` meta data helps to make guesses at what types to deserialize. 

Superseded by #6026
Rework LongFormer to make it compatible with ONNX
Author: mfuntowicz
Pull request: yes
Labels: name: WIP

- [x] Remove implicit `bool` -> `int` conversion while padding attention_mask with `False` value.
- [x] Remove call to `einsum` where `matmul` + `transpose` can be used (makes optimizations easier for ONNX)
- [ ] Diagonal matrix computation try to avoid ScatterND with negative steps / index
- [ ] Use `torch.div(a, b, rounding_mode='trunc')` instead of `floor_divide` 
- [ ] Check [function](https://github.com/huggingface/transformers/blob/master/src/transformers/models/longformer/modeling_longformer.py#L783) reporting `torch.Tensor` return type which doesn't return anything

Comments:
I'm fine with this rework as long as all the slow tests are passing :-) 

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/master/CONTRIBUTING.md) are likely to be ignored.

Hey github actions, I need to find some time to continue my work here ! :D
Make LazyModule picklable
Author: lhoestq
Pull request: yes

From this issue https://github.com/huggingface/transformers/issues/12549 it seems that it could be nice to have the `transformers` module picklable, since it can be useful for the `datasets` library's caching for example.

The only object that is currently not picklable is the `_LazyModule`.

In this PR I just made this object picklable, and so `transformers` becomes picklable as well.

This should hopefully help with issue https://github.com/huggingface/transformers/issues/12549

Comments:
This doesn't work sadly, because it then sets the wrong `__file__` and `__path__` attribute to the `transformers` module:
```py
import transformers
transformers.__file__
```
will return something like `'/home/sgugger/git/transformers/src/transformers/file_utils.py'` instead of `'/home/sgugger/git/transformers/src/transformers/__init__.py'`.

This will then probably mess up lots of things that depend on those attributes.

I can look at another solution when I have some time.

Indeed ! Thanks for checking
At least I tried x)

There must be a way to keep _LazyModule inside `__init__.py` and make it picklable. Currently the issue is that it can't be imported like this `from transformers import _LazyModule`. But as soon as we enable its import, it will be possible to pickle 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/master/CONTRIBUTING.md) are likely to be ignored.

Resolved by #12567
Conversational pipeline: pop instead of get to avoir duplicate kwargs
Author: LysandreJik
Pull request: yes

The following two tests fail:
```
FAILED tests/test_pipelines_conversational.py::ConversationalPipelineTests::test_integration_torch_conversation
FAILED tests/test_pipelines_conversational.py::ConversationalPipelineTests::test_integration_torch_conversation_encoder_decoder
```

For the following reason:

```
    def _forward(self, model_inputs, minimum_tokens=10, **generate_kwargs):
        max_length = generate_kwargs.get("max_length", self.model.config.max_length)
    
        n = model_inputs["input_ids"].shape[1]
        if max_length - minimum_tokens < n:
            logger.warning(f"Conversation input is to long ({n}), trimming it to ({max_length} - {minimum_tokens})")
            trim = max_length - minimum_tokens
            model_inputs["input_ids"] = model_inputs["input_ids"][:, -trim:]
            if "attention_mask" in model_inputs:
                model_inputs["attention_mask"] = model_inputs["attention_mask"][:, -trim:]
        conversation = model_inputs.pop("conversation")
        model_inputs["max_length"] = max_length
>       output_ids = self.model.generate(**model_inputs, **generate_kwargs)
E       TypeError: generate() got multiple values for keyword argument 'max_length'
```

This PR fixes that.

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/master/CONTRIBUTING.md) are likely to be ignored.

This was already fixed (by changing removing the assignment statement (making the fix a bit cleaner).

Can't find the associated PR atm tho (was done approx at the same time)
Assert evaluation_strategy not no when load_best_model_at_end
Author: sgugger
Pull request: yes

# What does this PR do?

Since using `--load_best_model_at_end` overrides the `save_strategy` by the `evaluation_strategy`, this PR adds a defensive check to make sure that strategy is not "no" (otherwise nothing is ever saved).

Fixes #12685 

Comments:
> I see that practically:
>
> load_best_model_at_end cancels out save_strategy=steps.
> load_best_model_at_end has no impact on save_strategy=epoch.

That is not completely correct. One should also add that if `evaluation_strategy=steps`, a save is done every `eval_steps` and if `evaluation_strategy=epoch`, a save is done every epoch. Basically the model need to be saved every time there is an evaluation, to keep track of the best checkpoint.

To be honest, it makes absolutely no sense to use `--load_best_model_at_end` if the `evaluation_strategy` and `save_strategy` are not the same (and in the case of steps, with the same number of steps) so perhaps this is what the assert should be. The current implementation tries to avoid having the user input the same thing twice, but maybe it is too confusing.

That works too.

My initial suggestion was to only flag to the user the silent override of `save_steps`, but if we can do better, then by all means let's do that!

Superseded by #12786