noamaanMulla-03/transformers-issues huggingface dataset:
Skip some doctests in quicktour
Author: stevhliu
Pull request: yes
Labels: name: Tests

The quicktour includes code snippets that instantiate a generic `dataset["train"]` and `dataset["test"]` (in the `Trainer` sections) that's only meant to be an example a user can copy/paste and replace with their own dataset. This causes the tests to fail since no dataset is actually being loaded. This PR adds the `# doctest: +SKIP` directive to skip the affected code snippets (the alternative option is to include a real dataset in the examples that can be loaded).

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

LGTM, but it might be better to use an already processed dataset we could host somewhere so the whole thing can run (particularly since this also becomes a notebook).

I'll merge this for now so the daily CI is happy and then update it later with a processed dataset!
Fix custom config loading for clip model
Author: avinashsai
Pull request: yes

# What does this PR do?

Fixes # (issue)
In the clip model, CLIPTextTransformer and CLIPVisionTransformer loads the original clip config file eventhough custom parameters are given in the new config file.



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

Hey @avinashsai, I don't understand what you're trying to do: two lines above your changes is a `self.config = config`, so you're working with the same object.

Could you shed some light on what doesn't work so I can help you out? Thanks :)

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.
Group_texts in run_clm.py will add shorter than block_size groups on intermediately sized training sets.
Author: spanglies
Pull request: no
Labels: name: bug

### System Info

SageMaker using transformers 4.17 and attempting to fine-tune GPT2 and GPT-Neo. 

### Who can help?

@sgugger

### Information

- [X] The official example scripts
- [ ] 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

Steps to reproduce:

1: Use a dataset of about 10MB
2: Run run_clm.py on SageMaker using the latest supported version (4.17 at the time of writing)
3: Part way through training receive error stating: [ValueError: expected sequence of length 1024 at dim 1 (got 507)](https://discuss.huggingface.co/t/valueerror-expected-sequence-of-length-1024-at-dim-1-got-507/20390)

### Expected behavior

group_texts in run_clm should drop all sequences that are not the correct blocksize to prevent such an error. 

Additional context: This bug appears to be introduced by commit: 6f1adc43344a4ebe6fb1ecc018df9d6c092370cf
Removing the check for total_length < block_size resolves the issue and training completes without issue.

Comments:
If you remove that line, you will get no training at all, since there is going to be 0 batches left. It is there to ensure a small dataset yields exactly one batch.

It looks like I must have hit some sort of an edge case. I think what happened was that it got to the end of my file and no longer had 1000 examples to provide the function `group_texts`. So in the case where the dataset lines up wrong, it won't work.

Perhaps the `drop_last_batch` flag should be set by default during the group_texts phase and a parse_flag for "small datasets" should be introduced?

Using run_clm.py from the commit by @spanglies solved this issue (I am forever grateful, this has been frustrating). You have to remove references to telemetrics and to check_min_version() to make it work on Sagemaker with estimator/fit.

However the same error shows up in evaluation, which I am disabling for now. Happy to finally have made it to a saved model, as that was my goal for this summer...

Hey glad that helped you @nittonfemton It wouldn't be hard to apply the same filter to the evaluation data. I hadn't because it was also my goal to have a trained model and I was less concerned with validating it. The way I had gotten it to work on sage maker was to apply the commit to v4.17.

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.
[DeepSpeed ZeRO3] Fix performance degradation in sharded models
Author: tjruwase
Pull request: yes

When sharded models were added the deepspeed/zero3 branch of model loading of pretrained weights 

https://github.com/huggingface/transformers/blob/7d5fde991d598370d961be8cb7add6541e2b59ce/src/transformers/modeling_utils.py#L427-L429

got ~N-shards-x-slower, since it wastefully gathered weights that weren't in the `state_dict`:

**So for example with BLOOM-176 which has 72 shards, the loading was ~70x slower under deepspeed zero3 and nvme offload!**

This fix takes care of the situation with sharded models by finding an intersection of `state_dict` keys and the keys of the parameters of the current submodule that is being loaded to and thus only gathering the weights that get updated. And if there is no intersection skipping the rest of the branch altogether.

The 1-shard use case still works as the intersection should be 100%.



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

@sgugger, I was thinking about this discovery and my suggestion is that we do this not just for deepspeed.

The thing is - with sharded models like bloom-176 (72 shards) - 90% of the time this code:

https://github.com/tjruwase/transformers/blob/81dbd0ba6c5422dec33dd31cf076e44d96d2d968/src/transformers/modeling_utils.py#L436

is doing nothing since its `state_dict`'s "payload" doesn't match the params of the submodule it's called for, as most of the time they are in another shard.

Not sure of the cost or the actual promised saving, but it will save many unnecessary `module._load_from_state_dict(*args)` calls.

Especially since the code is already there anyway.

Thoughts?

Yes, we could probably ignore the load when there is no parameter to load indeed. Will make a PR this morning.

Took a stab at it in #18937. Thinking more of it, I'm not sure if we'll get a sensible gain as I expect the calls to `module._load_from_state_dict` to be mostly noops but we can certainly train and measure the difference!

I followed up here: https://github.com/huggingface/transformers/pull/18937#pullrequestreview-1100945027
Add X-CLIP
Author: NielsRogge
Pull request: yes

# What does this PR do?

This PR adds [X-CLIP](https://github.com/microsoft/VideoX/tree/master/X-CLIP), which is a minimal extension of CLIP for video-language pre-training.

To do:

- [x] upload all checkpoints to the hub, as part of the `microsoft` organization



Comments:
Many tests fail due to the following error:

> ModuleNotFoundError: No module named 'transformers.models.xclip'

This is probably because I first called the model folder "xclip", which is now called "x_clip". Still, wondering why it keeps looking for the module models.clip. If anyone has any pointers, that would be greatly appreciated.

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

@sgugger thanks a lot, that solved the issue. There seems to be another (small) issue with run_tests_hub:
```
==================================== ERRORS ====================================
_______________ ERROR collecting tests/utils/test_file_utils.py ________________
tests/utils/test_file_utils.py:26: in <module>
    from transformers import *  # noqa F406
src/transformers/utils/import_utils.py:1021: in __getattr__
    value = getattr(module, name)
src/transformers/utils/import_utils.py:1023: in __getattr__
    raise AttributeError(f"module {self.__name__} has no attribute {name}")
E   AttributeError: module transformers.models.clip has no attribute CLIPProcessor
```
Running `RUN_SLOW=yes pytest tests/utils/test_file_utils.py` passes locally for me.

That would be because you moved `CLIPProcessor` in the non-vision dependent objects in the main init (and rightly so) but did not do the same for the `models/clip/__init__.py`.

@sgugger and @alaradirik - the PR is ready for merge. Kindly asking for your approval :)
Latest Wav2Vec2 pretraining script runs on first GPU only
Author: RK-BAKU
Pull request: no
Labels: name: bug

### System Info

Dear @patrickvonplaten 

I've tried different multi GPU setups (RTX 3090 and A5000) but training always runs only on device 0.

**Tried these commands:**

`accelerate launch --num_processes=2 pretrain.py`
`accelerate launch pretrain.py`

Seems there is some bug in the script because old fairseq training runs on all available devices.
Kindly assist to understand where to dig further.

**Script used for training:**
[https://github.com/huggingface/transformers/blob/main/examples/pytorch/speech-pretraining/run_wav2vec2_pretraining_no_trainer.py](https://github.com/huggingface/transformers/blob/main/examples/pytorch/speech-pretraining/run_wav2vec2_pretraining_no_trainer.py)

### Who can help?

@patr

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

Just followed the instruction on the script page

### Expected behavior

Run on multiple GPUs

Comments:
Got to run accelerate config in order to setup devices.
Wav2Vec2ForPreTraining loss Nan fixed
Author: abdouaziz
Pull request: yes

# What does this PR do?

- [ ] This PR fixe the  loss of   [Wav2Vec2ForPreTraining](https://huggingface.co/docs/transformers/v4.21.3/en/model_doc/wav2vec2#transformers.Wav2Vec2ForPreTraining) which  return Nan values .

- [ ] We fix the error by adding sampled_negative_indices as a target to calculate the loss .


Comments:
[TBD] discrepancy regarding the tokenize method behavior - should the token correspond to the token in the vocabulary or to the initial text
Author: SaulLu
Pull request: no

## Environment info
```
- `transformers` version: 4.17.0
- Platform: Linux-5.4.144+-x86_64-with-Ubuntu-18.04-bionic
- Python version: 3.7.12
- PyTorch version (GPU?): 1.10.0+cu111 (False)
- Tensorflow version (GPU?): 2.8.0 (False)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
```

## Information

When adding a token to a tokenizer with a slow backend or to a tokenizer with a fast backend, if you use the `AddedToken` class with the `lstrip=True` argument, the output of the tokenize method is not the same.

This difference should be put into perspective by the fact that the encoding (the sequence of ids) is identical: the model will see the correct input.

## To reproduce

```python
from transformers import AutoTokenizer, AddedToken

def print_tokenizer_result(text, tokenizer):
    tokens = tokenizer.tokenize(text)
    print(f"tokenize method: {tokens}")

model_name = "patrickvonplaten/norwegian-roberta-base"

tokenizer_init = AutoTokenizer.from_pretrained(model_name)
tokenizer_init.save_pretrained("local_tokenizer")

model_name = "local_tokenizer"
tokenizer_s = AutoTokenizer.from_pretrained(model_name, use_fast=False)
tokenizer_f = AutoTokenizer.from_pretrained(model_name, use_fast=True)

new_token = "added_token_lstrip_false"
tokenizer_s.add_tokens(AddedToken(new_token, lstrip=True))
tokenizer_f.add_tokens(AddedToken(new_token, lstrip=True))

text = "Example with added_token_lstrip_false"
print("Output for the fast:")
print_tokenizer_result(text, tokenizer_f)
print("\nOutput for the slow:")
print_tokenizer_result(text, tokenizer_s)
```

Output:
```
Output for the fast:
tokenize method: ['Ex', 'amp', 'le', 'Ġwith', ' added_token_lstrip_false'] # Note the space at the beginning of ' added_token_lstrip_false'

Output for the slow:
tokenize method: ['Ex', 'amp', 'le', 'Ġwith', 'added_token_lstrip_false']
```

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.
Simplify `is_pad_token_not_equal_to_eos_token_id`
Author: ekagra-ranjan
Pull request: yes

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

Reduces a bool check which simplifies the expression for `is_pad_token_not_equal_to_eos_token_id`


## Who can review?

<!-- 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:

- albert, bert, xlm: @LysandreJik
- blenderbot, bart, marian, pegasus, encoderdecoder,  t5: @patrickvonplaten, @patil-suraj
- longformer, reformer, transfoxl, xlnet: @patrickvonplaten
- fsmt: @stas00
- funnel: @sgugger
- gpt2: @patrickvonplaten, @LysandreJik
- rag: @patrickvonplaten, @lhoestq
- tensorflow: @LysandreJik

Library:

- benchmarks: @patrickvonplaten
- deepspeed: @stas00
- ray/raytune: @richardliaw, @amogkam
- text generation: @patrickvonplaten
- tokenizers: @n1t0, @LysandreJik
- trainer: @sgugger
- pipelines: @LysandreJik

Documentation: @sgugger

HF projects:

- datasets: [different repo](https://github.com/huggingface/datasets)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Examples:

- maintained examples (not research project or legacy): @sgugger, @patil-suraj
- research_projects/bert-loses-patience: @JetRunner
- research_projects/distillation: @VictorSanh

 -->

@patrickvonplaten

Comments:
_The documentation is not available anymore as the PR was closed or merged._
[DETR] Add num_channels attribute
Author: NielsRogge
Pull request: yes

# What does this PR do?

This PR adds a num_channels attribute to DetrConfig, making it possible to use the model on greyscale images (i.e. num_channels = 1).

Fixes #14875

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

In my previous experience with DETR I also have faced the issues with Feature extractor, does it support 1 chanel input now? (I had to do something like that: https://github.com/huggingface/transformers/pull/14933/files#diff-826e0e0303aa4a12e527884e4dc548fe8debb137905260e42ec8b4331c9c2c63R594) @NielsRogge
Update translation requests contact
Author: NimaBoscarino
Pull request: yes

# What does this PR do?
Updates the contact for translation requests to GuggerSylvain (@sgugger - if you're alright with that!)

## Before submitting
- [x] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).

## Who can review?
Documentation: @sgugger, @osanseviero 

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

Doesn't look like I have merge privileges on this repo, could you merge it @sgugger?

Just waiting for the last green tick and will do so!
Fix train_step, test_step and tests for CLIP
Author: Rocketknight1
Pull request: yes

CLIP models were not being tested correctly with `fit()` because the test skipped models without a `hf_compute_loss` method. This skip was added to skip base models like `TFBERTModel` that do not have specific output heads and losses. However, it also skips models like CLIP that do not use `compute_loss` / `hf_compute_loss` methods.

The new test checks whether the model's return type dataclass has a `loss` key, which is a more reliable check. Enabling this reveals the bug in `fit()` for TFClip, so this PR also includes fixes to `train_step` and `test_step` for CLIP and models like it that require `return_loss=True` to be passed, but do not set it by default.

Draft for now because this will likely flush out other bugs or cause other problems!

Fixes #18670.

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

The tests passed. I'm as surprised as everyone else. It's ready for review!

Thanks @Rocketknight1  for quick fix. I am wondering if it makes sense to wrap the loss computation for TFCLIP into a `hf_compute_loss`? 

I also see for `TFSegformerForSemanticSegmentation`, it defines `hf_compute_loss` under its own model class.

If doable, maybe it's good to add `TFSemanticSegmentationLoss`, `TFcontrastiveLoss` class etc. 

Just want to hear some opinions. cc @gante @amyeroberts 

@ydshieh I don't think that's necessary - the new check we use means we don't need to rely on `hf_compute_loss` being present anymore!

Just realized that using `return` instead of `continue` in the tests was skipping a lot of tests, which might have been why it was green so easily. Rerunning everything!

Further updates: Now that we're no longer incorrectly skipping tests, this turned up quite a few bugs! The main source of issues is that some more recent models are returning scalar losses, but both our `test_loss_computation` test and Keras `fit()` expect that the loss has some shape, even if that shape is `(1,)`.
Exit early in load if no weights are in the sharded state dict
Author: sgugger
Pull request: yes

# What does this PR do?

As suggested by @stas00 in #18911, this PR checks whether there any parameters in the state dict to load in the current module and exits early if there are none. This might be useful when loading a huge model with a lot of shards.

@stas00 could you try and see if there is a gain with this or not?

Comments:
_The documentation is not available anymore as the PR was closed or merged._
The configuration is not a valid json file
Author: nicolaspi
Pull request: no
Labels: name: bug

### System Info

The config at this url [https://huggingface.co/openai/clip-vit-large-patch14/resolve/main/config.json](https://huggingface.co/openai/clip-vit-large-patch14/resolve/main/config.json) is not a valid JSON and produces error:
`json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 88 column 3 (char 2317)`


### Who can help?

@patil-suraj

### Information

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

### Tasks

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

### Reproduction

```python
from transformers import CLIPTextModel
CLIPTextModel.from_pretrained("openai/clip-vit-large-patch14")
```

### Expected behavior

The config should be a valid json.

Comments:
👆🏼Same error just started occuring about 30 mins ago across all machines

<img width="502" alt="Screen Shot 2022-09-09 at 3 00 30 PM" src="https://user-images.githubusercontent.com/26133/189424655-2d594077-d358-41aa-a265-86571dabb522.png">


Related to #18962 

Just in case, pinging @patil-suraj given a few commits on that model repo today =) https://huggingface.co/openai/clip-vit-large-patch14/commits/main

is there a way to disable it grabbing the newest?  I disabled my ethernet ran the app, and then turned it back on after it got past that point in the script and I'm running again if someone needs a fast temporary bandaid

Thank you for reporting, this was fixed in [openai/clip-vit-large-patch14#4](https://huggingface.co/openai/clip-vit-large-patch14/discussions/4)
Re-running the instantiation/`from_pretrained` should redownload the correct JSON file.

If you have a set of downloaded cache of the unbroken files, you can get around the problem by forcing an offline only mode and using your cache.

If you don't want to edit your script, add TRANSFORMERS_OFFLINE=1, as seen by document here https://huggingface.co/docs/transformers/installation#offline-mode
If you prefer to edit your python scripts, you can pass local_files_only when calling from_pretrained https://huggingface.co/docs/transformers/main_classes/model


Fixed
update black target version
Author: BramVanroy
Pull request: yes

Considering that setup.yp requires Python 3.7 or higher:

https://github.com/huggingface/transformers/blob/22f72185601d5167a747104b4aca102d0e92524c/setup.py#L417

it might make sense to also have black target only Python 3.7. Just a suggestion though.

Unsurprisingly, this PR may trigger quality check errors.

Note sure who to tag, so assuming for general repo things: @sgugger and @LysandreJik 



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

Looks like it wants to change a looooot of files. This usually creates a hell of merge conflicts in PRs so I would really like to avoid doing it too much. We will switch to black 2023 in January, so how about we change the target at that point too? We can change your PR to leave a comment for now so we don't forget.

> Looks like it wants to change a looooot of files. This usually creates a hell of merge conflicts in PRs so I would really like to avoid doing it too much. We will switch to black 2023 in January, so how about we change the target at that point too? We can change your PR to leave a comment for now so we don't forget.

Sure! 

I didn't quite understand what you mean with the last sentence though. Do you mean closing this PR and opening an issue instead? That works, whatever is best.

Just putting a comment next to the black version pinned in the setup, so that we know to update this at the next version change.

> Just putting a comment next to the black version pinned in the setup, so that we know to update this at the next version change.

Alright. Done.
TF: unpin maximum TF version
Author: gante
Pull request: yes

# What does this PR do?

Unpins TF maximum version. 

As in the scheduled run, a few onnx+tf tests broke. I'd say we merge this PR, and put the newly broken tests on our todo list.

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

Why why why would we merge a PR with red ticks? Every contributor making a PR this weekend and until this is resolved will wonder what they did wrong.

I was thinking of the self-hosted scheduled CIs only when reviewing this PR. You are right - we should keep CircleCI / push CI green.

For the scheduled CI, we can unpin so we know what to fix. But let's do it on Monday if you are ok.