Skip to content

Add Beit3 model - #22289

Closed
raghavanone wants to merge 205 commits into
huggingface:mainfrom
raghavanone:fix_issue_22178
Closed

raghavanone wants to merge 205 commits into
huggingface:mainfrom
raghavanone:fix_issue_22178

Conversation

@raghavanone

Copy link
Copy Markdown
Contributor

Fixes #22178

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@MetaB0y

MetaB0y commented Mar 22, 2023

Copy link
Copy Markdown

I don't know much about the details of the transformers library but isn't it confusing that it refers to the model as microsoft/beit-base-patch16-224-pt22k etc which is the name for beit v1, not beit v3?

@raghavanone

Copy link
Copy Markdown
Contributor Author

Hi @MetaB0y , All I have done is to pull in different modules needed for beit3 into single file. I will start working on cleaning it up.

@atharvakavitkar

Copy link
Copy Markdown

Hi @raghavanone, just wanted to know updates on this PR. If required, I would like to help.

@raghavanone

Copy link
Copy Markdown
Contributor Author

Hi @raghavanone, just wanted to know updates on this PR. If required, I would like to help.

@atharvakavitkar You can for sure contribute, I am out till mid of april, will pick this up if not done by anyone else by that time . Start a new PR, if you wanted to work on this .

@JonathanRayner

Copy link
Copy Markdown

Hey @raghavanone , I was thinking about working on this this weekend and see that you have a WIP already and are probably back now. Would it help if I contributed on this PR or better to leave it to you?

@raghavanone

Copy link
Copy Markdown
Contributor Author

Hey @raghavanone , I was thinking about working on this this weekend and see that you have a WIP already and are probably back now. Would it help if I contributed on this PR or better to leave it to you?

Hey @JonathanRayner , Thanks for asking, I have some changes locally, making changes to MOE layers are bit tricky. We can collaborate in this PR, But we have to find a way to communicate (use HF slack maybe) .

@sgugger

sgugger commented Apr 24, 2023

Copy link
Copy Markdown
Collaborator

cc @alaradirik so you can help when needed.

@alaradirik

alaradirik commented Apr 30, 2023 •

Copy link
Copy Markdown
Contributor

Hey @raghavanone , I was thinking about working on this this weekend and see that you have a WIP already and are probably back now. Would it help if I contributed on this PR or better to leave it to you?

Hey @JonathanRayner , Thanks for asking, I have some changes locally, making changes to MOE layers are bit tricky. We can collaborate in this PR, But we have to find a way to communicate (use HF slack maybe) .

Hi @raghavanone, thanks for working on this! The easiest way to collaborate would be to add @JonathanRayner as a collaborator to your forked transformers repo. I'd also be happy to create a Slack channel and invite you both to make it easier to communicate.

I took a quick look at the PR and it'd be great if you could follow naming conventions in line with Beit such that the class and folder names are Beit3 and beit3 respectively. Other than this, BEiT-3 is a multi-modal model so you'll need to create image_processing_beit3.py, tokenizer_beit3.py and processing_beit3.py scripts, where the latter wraps the text and image preprocessor classes into a single class. BEiT-3 uses the transformers XLMRobertaTokenizer class to preprocess text so you can use that instead of creating tokenizer_beit3.py. You can refer to OWL-ViT to see an example of a multi-modal model that uses an existing tokenizer class (CLIPTokenizer).

In order to check if the PR passes the CI tests, you can run the following commands:

make style
make quality
make repo-consistency

pytest tests/models/beit3/test_image_processor_beit3.py
pytest tests/models/beit3/test_processor_beit3.py
RUN_SLOW=True pytest tests/models/beit3/test_modeling_beit3.py

Hope this helps, I can add invite you to Slack if you send me your email addresses :)

@raghavanone

raghavanone commented May 1, 2023 •

Copy link
Copy Markdown
Contributor Author

Hey @raghavanone , I was thinking about working on this this weekend and see that you have a WIP already and are probably back now. Would it help if I contributed on this PR or better to leave it to you?

Hey @JonathanRayner , Thanks for asking, I have some changes locally, making changes to MOE layers are bit tricky. We can collaborate in this PR, But we have to find a way to communicate (use HF slack maybe) .

Hi @raghavanone, thanks for working on this! The easiest way to collaborate would be to add @JonathanRayner as a collaborator to your forked transformers repo. I'd also be happy to create a Slack channel and invite you both to make it easier to communicate.

I took a quick look at the PR and it'd be great if you could follow naming conventions in line with Beit such that the class and folder names are Beit3 and beit3 respectively. Other than this, BEiT-3 is a multi-modal model so you'll need to create image_processing_beit3.py, tokenizer_beit3.py and processing_beit3.py scripts, where the latter wraps the text and image preprocessor classes into a single class. BEiT-3 uses the transformers XLMRobertaTokenizer class to preprocess text so you can use that instead of creating tokenizer_beit3.py. You can refer to OWL-ViT to see an example of a multi-modal model that uses an existing tokenizer class (CLIPTokenizer).

In order to check if the PR passes the CI tests, you can run the following commands:

make style
make quality
make repo-consistency

pytest tests/models/beit3/test_image_processor_beit3.py
pytest tests/models/beit3/test_processor_beit3.py
RUN_SLOW=True pytest tests/models/beit3/test_modeling_beit3.py

Hope this helps, I can add invite you to Slack if you send me your email addresses :)

Thanks @alaradirik , The PR is almost ready. I am already in HF slack under email oneraghavan@gmail.com / username Raghavan . I have some questions, please ping me in slack.

@raghavanone

Copy link
Copy Markdown
Contributor Author

@alaradirik I am unable to understand the purpose of a processor, the Beit3 model takes in token ids and image as tensor. Please help me understand this more.

@raghavanone raghavanone changed the title <WIP> Early Draft of Beit3 <WIP>Add Beit3 model May 3, 2023
@raghavanone raghavanone changed the title <WIP>Add Beit3 model Add Beit3 model May 3, 2023
@alaradirik

Copy link
Copy Markdown
Contributor

@alaradirik I am unable to understand the purpose of a processor, the Beit3 model takes in token ids and image as tensor. Please help me understand this more.

Hi @raghavanone, the models expect the input images to be preprocessed. Hence, the image_processing_beit3.py script should contain the Beit3ImageProcessor class that takes in the raw input image and preprocesses it to the format expected as input to the model (resizing to a fixed input size, normalization, cropping, etc.).

processing_beit3.py script should contain the Beit3Processor class that wraps this image processor class and tokenizer class into a single instance such that users can use it preprocess text or images or both. Please take a look at other multi-modal model processors such as OWL-ViT and CLIP to see how that works.

@raghavanone

Copy link
Copy Markdown
Contributor Author

@alaradirik I am unable to understand the purpose of a processor, the Beit3 model takes in token ids and image as tensor. Please help me understand this more.

Hi @raghavanone, the models expect the input images to be preprocessed. Hence, the image_processing_beit3.py script should contain the Beit3ImageProcessor class that takes in the raw input image and preprocesses it to the format expected as input to the model (resizing to a fixed input size, normalization, cropping, etc.).

processing_beit3.py script should contain the Beit3Processor class that wraps this image processor class and tokenizer class into a single instance such that users can use it preprocess text or images or both. Please take a look at other multi-modal model processors such as OWL-ViT and CLIP to see how that works.

@alaradirik Thanks, I have added both the class and added tests for them . Requesting you to review the PR.

@alaradirik alaradirik left a comment •

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.

Hi @raghavanone, thanks for working on this again! We are excited to add BEiT-3 to transformers :)

I did an initial pass of the PR and left quite a few comments but most of them are styling and naming changes, and shouldn't take too long to apply. However, I see that there is no conversion script nor checkpoints uploaded to the hub so it's not possible to review the modeling test script or fully review the model classes yet.

The conversion script:

  • Loads the pre-trained original model and randomly initializes the HF implementation with the corresponding configuration
  • Copies the pre-trained parameters (weights and biases) of the original model to the corresponding parameters of the randomly initialized HF model (the conversion step)
  • Forward propagates an arbitrary input (text + image, image pairs, text or image in this case) through both the original model and converted HF model and checks if the outputs match
  • Uploads the converted HF model to the hub
  • If it's not possible / easy to load and perform inference with the original model, you can perform inference with the original model separately using the same inputs and hard-code the expected logits in the conversion script.

Note that each Hub repo is a Git repository that holds the checkpoint specific image preprocessing, tokenizer and model configurations, as well as the weights of that checkpoint. You can take a look at other conversion scripts to see what it looks like.

Could you create a convert_beit3_to_hf.py script and upload the converted checkpoints to the hub? You will need to edit test_modeling_beit3.py script and add tests that check the output of pre-trained models once you do that.

My other comments are:

  • Naming should be consistent, please make sure all model-specific folder names are beit3 and script names end with beit3.
  • Argument and variable names should be as self-descriptive as possible.
  • Commented out / irrelevant sections should be removed from all scripts.
  • Modules of the model should have fixed / pre-defined layers. This ensures readability, re-use of modules in future works that build upon this model and addition of tensorflow version of the model in the future.
  • If the original work preprocesses the raw model outputs to get the final results (e.g. image captioning), post-processing methods should be added to the Beit3Processor class. However, this is not urgent and can be a separate PR.

Comment thread README.md Resolved
Comment thread README.md Resolved
Comment thread README_es.md Resolved
Comment thread README_es.md Resolved
Comment thread README_hd.md Resolved
@raghavanone

Copy link
Copy Markdown
Contributor Author

@alaradirik All the PR feedbacks has been resolved. There are few open and I have put my questions in the comment. On the conversion script I have following questions :

  • There are 22 variations of model checkpoints released, should I test out for each of them ?
  • How to upload the checkpoints to hf ?

Comment thread README.md Resolved
Comment thread docs/source/en/model_doc/beit3.mdx Resolved
Comment thread docs/source/en/model_doc/beit3.mdx Resolved
Comment thread docs/source/en/model_doc/beit3.mdx Resolved
Comment thread docs/source/en/model_doc/beit3.mdx Resolved
@raghavanone

Copy link
Copy Markdown
Contributor Author

@NielsRogge Following are the open questions to be resolved :

Q1. How should be the config uploaded ?
Q2. How should be the checkpoints uploaded ?
Q3. Comment from Alara : "Passing a module to the class is not very optimal. I see that you are initializing and passing various modules in Beit3EncoderLayer and MultiheadAttention to MultiwayNetwork and creating deep copies.

I think it'd make more sense to create separate classes (e.g. Beit3Dense, Beit3FeedForwardNetwork) as variable names such as first and second are confusing and make the code more difficult to be adapted for works that build upon this.

I'm cc'ing @sgugger for his opinion."

For reference look at here https://github.com/huggingface/transformers/blob/1cda50bd12d7d454f56fbdd9f8fe32aee1eae5b3/src/transformers/models/beit3/modeling_beit3.py#L382

@sgugger

sgugger commented May 30, 2023

Copy link
Copy Markdown
Collaborator

cc @amyeroberts

@asartran

Copy link
Copy Markdown
Contributor

Hi @raghavanone,

I'll try to answer your questions as best as possible. For future Q's it's best to ping me rather than @NielsRogge or @sgugger.

  1. When you say 'uploaded' - are you referring to uploading onto the hub e.g. like this for bert? If so, this should be uploaded alongside the model weights. When calling model.push_to_hub(repo_path), both the model's checkpoint and configuration will be uploaded. You can look at some of the conversion scripts to see the weight loading / converting / uploading logic e.g. here. Whilst the PR is still underdevelopment, I suggest having the models under a personal repo. Then, once ready to merge, we can transfer the weights and configs to under the official orgs'.
  2. See above.
  3. What's the question? Are you asking what @alaradirik's comment means, or asking whether this is something that should be done?

@raghavanone

Copy link
Copy Markdown
Contributor Author

Hi @raghavanone,

I'll try to answer your questions as best as possible. For future Q's it's best to ping me rather than @NielsRogge or @sgugger.

  1. When you say 'uploaded' - are you referring to uploading onto the hub e.g. like this for bert? If so, this should be uploaded alongside the model weights. When calling model.push_to_hub(repo_path), both the model's checkpoint and configuration will be uploaded. You can look at some of the conversion scripts to see the weight loading / converting / uploading logic e.g. here. Whilst the PR is still underdevelopment, I suggest having the models under a personal repo. Then, once ready to merge, we can transfer the weights and configs to under the official orgs'.
  2. See above.
  3. What's the question? Are you asking what @alaradirik's comment means, or asking whether this is something that should be done?

Thanks for the answers, for Q3 , I am not sure how to incorporate the feedback, I need some support on what needs to be done .

@github-actions

Copy link
Copy Markdown
Contributor

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 are likely to be ignored.

@github-actions github-actions Bot closed this Mar 20, 2024
@jhillhouse92

Copy link
Copy Markdown

@raghavanone Is there other work with getting this merged in?

@NielsRogge

Copy link
Copy Markdown
Collaborator

This PR was actually 95% ready to get merged in. I can take it over and make him a co-author

@geiche735

Copy link
Copy Markdown

Just curious. Was this completed?

@jaycecd

jaycecd commented Dec 4, 2024

Copy link
Copy Markdown

Just curious. Was this completed?

It appears that it hasn't been merged into the main branch yet.

@Leon0402 Leon0402 mentioned this pull request Jul 20, 2025
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.

Add BEiTv3