fixes bugs to handle non-dict output

[alaradirik]

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. 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,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? 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, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

[ydshieh]

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.

[HuggingFaceDocBuilderDev]

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

[alaradirik]

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.

[ydshieh]

This line

transformers/src/transformers/models/owlvit/modeling_owlvit.py

Line 1270 in 4447142

outputs = self.owlvit(

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 :-)

[alaradirik]

This line

transformers/src/transformers/models/owlvit/modeling_owlvit.py

Line 1270 in 4447142

outputs = self.owlvit(

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 ?

[ydshieh]

LGTM! Thanks @alaradirik for the fix :-)

[ydshieh]

Great! Let's make sure the tests pass before merge. Notice that the failing tests are slow tests, so they won't be run on CircleCI. We have to check it manually.

[sgugger]

You can't force return_dict=True in any part of the model as this mode is not compatible with torchscript (see here).

So this change will make Owl-ViT irremediably incompatible with torchscript I believe.

[ydshieh]

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

[alaradirik]

You can't force return_dict=True in any part of the model as this mode is not compatible with torchscript (see here).

So this change will make Owl-ViT irremediably incompatible with torchscript I believe.

Thank you @sgugger, I'm reverting to my previous commit then

[ydshieh]

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)

[sgugger]

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

[ydshieh]

@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]

@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_dictwould only be set to True for the intermediate computation in this case