fixes bugs to handle non-dict output
Author: alaradirik
Pull request: yes

# What does this PR do?
Fixes OWL-ViT's failing slow tests: `test_torchscript_simple`, `test_torchscript_output_attentions`, `test_torchscript_output_hidden_state`.

The failures were due to explicitly calling output keys instead of calling by the index. The bugs were introduced in this [PR](https://github.com/huggingface/transformers/pull/18734). Switching to indexing to fix the issue: `output.last_hidden_state` -> `output[0]`

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [ X] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?



Comments:
Hi, @alaradirik Thank you for the fix.

However, I am wondering if we can let `self.owlvit` returns `OwlViTOutput` instead of `tuple` here. As you can see (I believe), it's not easy to understand the code when the tuple indices are used, and it also becomes more difficult for debugging in general. Let me know your thought on this 🙏 , thanks.

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

H

> Hi, @alaradirik Thank you for the fix.
> 
> However, I am wondering if we can let `self.owlvit` returns `OwlViTOutput` instead of `tuple` here. As you can see (I believe), it's not easy to understand the code when the tuple indices are used, and it also becomes more difficult for debugging in general. Let me know your thought on this 🙏 , thanks.

Hi @ydshieh, self.owlvit already returns `OwlViTOutput`, which has a `return_dict` argument, the failing tests set `return_dict=False`. I think it'd be better to keep it as it is for consistency as OwlViTModel is almost identical to CLIPModel.

This line
https://github.com/huggingface/transformers/blob/44471422502a4f8cb606f6a8f8d9ae41207f2c2a/src/transformers/models/owlvit/modeling_owlvit.py#L1270
could pass `return_dict=True`, and we can keep using the named outputs in the code.

This doesn't change the method's input and output, but make things easier to read/understand.

Of course, the method `image_text_embedder` itself returns a tuple, and `OwlViTForObjectDetection.forward` will need to handle the tuple as it calls `image_text_embedder`. I am totally fine with this.

This is merely a suggestion (for the readability/debugging in the future). Let's see if @sgugger has any comment, and I let you make the final call :-) 


> This line
> 
> https://github.com/huggingface/transformers/blob/44471422502a4f8cb606f6a8f8d9ae41207f2c2a/src/transformers/models/owlvit/modeling_owlvit.py#L1270
> 
> 
> could pass `return_dict=True`, and we can keep using the named outputs in the code.
> This doesn't change the method's input and output, but make things easier to read/understand.
> 
That makes sense, and it's only a single line of code. I updated the PR, could you take a second look @ydshieh ?



LGTM! Thanks @alaradirik for the fix :-)

Thanks. Sorry about that, @alaradirik, we have to change it back to tuple sadly due  to the limitation of `torchscript`.

> You can't force `return_dict=True` in any part of the model as this mode is not compatible with torchscript (see [here](https://github.com/huggingface/transformers/blob/f85acb4d73a84fe9bee5279068b0430fc391fb36/src/transformers/configuration_utils.py#L385)).
> 
> So this change will make Owl-ViT irremediably incompatible with torchscript I believe.

Thank you @sgugger, I'm reverting to my previous commit then

I don't mean to bother here (i.e. not saying we should change again): but @sgugger I tried the commit that with `return_dict=True`, and `test_torch_fx_xxx` and `test_torchscrip_xxx` all pass under torch 1.12.1 and 1.11.0.

However it I changed `configs_no_init.return_dict = False` to `True` in the tests, it will fail.
It looks like the trace will fail only if `dict` is used at the **final** outputs, but not in the intermediate computation.

(just FYI only)

Oh in that case, feel free to use the dict outputs!

@alaradirik Let's not change again (back to `dict`), and feel free to merge as it is.

I can open a separate PR to use `dict` after a more thorough verification.

> @alaradirik Let's not change again (back to `dict`), and feel free to merge as it is.
> 
> I can open a separate PR to use `dict` after a more thorough verification.

I'm merging this for now but yes, `return_dict`would only be set to True for the intermediate computation in this case