Skip to content

Add TVLT#20725

Merged
amyeroberts merged 424 commits into
huggingface:mainfrom
zinengtang:main
Feb 15, 2023
Merged

Add TVLT#20725
amyeroberts merged 424 commits into
huggingface:mainfrom
zinengtang:main

Conversation

@zinengtang

@zinengtang zinengtang commented Dec 11, 2022

Copy link
Copy Markdown
Contributor

What does this PR do?

Add TVLT to transformers. This PR implements a original version of the TVLT: Textless Vision-Language Transformer from the original paper from https://arxiv.org/abs/2209.14156
I have provided the model weights here https://huggingface.co/TVLT/tvlt-base

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, Yes.
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? It is discussed on slack.
  • Did you make sure to update the documentation with your changes? I have added the documentation. Please check if they make sense.
  • Did you write any new necessary tests? Yes. Please check if they make sense.

Who can review?

Anyone.

@sgugger

sgugger commented Dec 12, 2022

Copy link
Copy Markdown
Collaborator

Thanks a lot for contributing this model! One first remark is that we tried to avoid all-capitals now in model classes to make all model names consistent across the library, so could you rename your classes to TvltModel, TvltConfig etc... (like BertModel, BertConfig).

Leaving you in the hands of @sanchit-gandhi and @ArthurZucker to finish cleaning the PR and all the tests.

@ydshieh ydshieh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zinengtang

Thank you for adding this model 🚀 !

I reviewed (only) the config/model files so far.
Could you first run make style and apply the changes if any modification is produced.
Some of my comments are about style, which could be probably fixed by this comment.

Then go through the comments (which are not fixed by the above comment).

I will check why CI are all red in the meantime.

Comment thread src/transformers/models/tvlt/modeling_tvlt.py
Comment thread src/transformers/models/tvlt/modeling_tvlt.py
Comment thread src/transformers/models/tvlt/modeling_tvlt.py
Comment thread src/transformers/models/tvlt/modeling_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/modeling_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/modeling_tvlt.py
Comment thread tests/models/tvlt/test_feature_extraction_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/configuration_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/configuration_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/configuration_tvlt.py Outdated

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Very cool model, I like the simplicity 😉
Here are is my first review

Comment thread docs/source/en/index.mdx
Comment thread src/transformers/models/tvlt/configuration_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/feature_extraction_tvlt.py
Comment thread src/transformers/models/tvlt/feature_extraction_tvlt.py
Comment thread src/transformers/models/tvlt/feature_extraction_tvlt.py
Comment thread src/transformers/models/tvlt/modeling_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/modeling_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/processing_tvlt.py
Comment thread tests/models/tvlt/test_modeling_tvlt.py Outdated
Comment thread tests/models/tvlt/test_modeling_tvlt.py Outdated
Comment thread README.md Outdated
Comment thread src/transformers/models/tvlt/feature_extraction_tvlt.py Outdated
@zinengtang

Copy link
Copy Markdown
Contributor Author

@NielsRogge @ArthurZucker
I have a question.
I converted the pixel_feature_extractor to imageprocessor class. Should I write a tester for imageprocessor but I couldn't find any example of it from other models.
Feature extractor will be deprecated in v5 so I don't have to create one with mother class imageprocessor?

@zinengtang

zinengtang commented Dec 17, 2022

Copy link
Copy Markdown
Contributor Author

Another question is that ProcessorMixin is multimodal processor but initialization requires tokenizer
But TVLT only takes in vision and audio. Is there anyway or a processor that can handle this?
Would it be good to propose to support combinations of two feature extractor in initialization of ProcessorMixin?

@ArthurZucker

Copy link
Copy Markdown
Collaborator

Hey!

  1. With regards to testing, you should indeed add a tester. While the FeatureExctractor were replaced with ImageProcessors I think we are still using the test_feature_extraction_...
  2. I think it indeed makes sense to support multiple FeatureExtractors, and it might also be good to support multiple tokenizers. We could also create a MultiModalProcessorMixin class, but it might be a bit too big for this PR.
    I think the best solution for now is to just make your class inherit from PushToHubMixin and copy the relevant functions accordingly!

@NielsRogge

NielsRogge commented Dec 19, 2022

Copy link
Copy Markdown
Collaborator

With regards to testing, you should indeed add a tester. While the FeatureExctractor were replaced with ImageProcessors I think we are still using the test_feature_extraction_...

We can call them test_image_processing_xxx.py now, see #20716 as an example

I think it indeed makes sense to support multiple FeatureExtractors, and it might also be good to support multiple tokenizers. We could also create a MultiModalProcessorMixin class, but it might be a bit too big for this PR.
I think the best solution for now is to just make your class inherit from PushToHubMixin and copy the relevant functions accordingly!

There's no need for a MultiModalProcessorMixin class I think, as the Processor class is exactly meant for multi-modal models. I think we just need to update processing_utils.py to handle the audio modality as well, cc @amyeroberts

@amyeroberts

Copy link
Copy Markdown
Contributor

@NielsRogge @zinengtang Yes, a single TVLTProcessor class is the way to go to handle processing of multiple modalities. These processor classes already handle audio e.g. wav2vec2 so I don't think anything needs to be done to the ProcessorMixin. It should work provided attributes is modified e.g. for CLIPProcessor.

Comment thread src/transformers/models/tvlt/feature_extraction_tvlt.py Outdated
Comment thread README.md Outdated
Comment thread docs/source/en/model_doc/tvlt.mdx Outdated
Comment thread src/transformers/__init__.py
Comment thread src/transformers/__init__.py Outdated
Comment thread src/transformers/__init__.py
Comment thread src/transformers/__init__.py
Comment thread src/transformers/__init__.py
Comment thread src/transformers/__init__.py
Comment thread src/transformers/__init__.py Outdated
Comment thread src/transformers/__init__.py
Comment thread README_es.md Outdated
Comment thread README_hd.md Outdated
Comment thread README_ja.md Outdated
Comment thread src/transformers/__init__.py Outdated
Comment thread src/transformers/__init__.py Outdated
Comment thread src/transformers/models/auto/configuration_auto.py Outdated
Comment thread src/transformers/models/tvlt/__init__.py
Comment thread src/transformers/models/tvlt/modeling_tvlt.py Outdated
Comment thread src/transformers/models/tvlt/feature_extraction_tvlt.py Outdated
@zinengtang

Copy link
Copy Markdown
Contributor Author

@NielsRogge now I addressed the remaining comments. It makes sense to me that TvltForQuestionAnswering is not needed since it is the same as TvltForAudioVisualClassification.

import unittest

import numpy as np
import torch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current error in the feature extraction tests can be resolved by safely importing torch e.g.

if is_torch_available():
    import torch

e.g. here for Speech T5

@amyeroberts

Copy link
Copy Markdown
Contributor

@zinengtang Thanks for adding these final changes! I've left a comment on test_feature_extraction_tvlt.py on how to resolve one of the current issues on the circleCI run. The failing wav2vec2 tests have been resolved on main - rebasing should resolve them on this branch.

Once all the tests are green I think we're good to merge!

@zinengtang

Copy link
Copy Markdown
Contributor Author

@amyeroberts Sounds great. Btw there seems to be a fail from other models
FAILED tests/models/hubert/test_modeling_tf_hubert.py::TFHubertRobustModelTest::test_dataset_conversion
Do you think it comes from this branch or main branch?

@amyeroberts

Copy link
Copy Markdown
Contributor

@zinengtang It's coming from main, not this branch :) I've just pushed a change on main - #21643 - which should resolve this temporarily for now. Could you rebase to add it to this branch?

@zinengtang

Copy link
Copy Markdown
Contributor Author

@amyeroberts Now it passed the tests! Thanks so much for the help/suggestions all the way. :)

@amyeroberts amyeroberts merged commit a0e69a9 into huggingface:main Feb 15, 2023
@amyeroberts

Copy link
Copy Markdown
Contributor

@zinengtang Thanks for all your work on adding this model, it's great to have it added to the repo! Make sure to spread the word about it being available :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants