Skip to content

[SegFormer] TensorFlow port#17910

Merged
gante merged 37 commits intohuggingface:mainfrom
sayakpaul:tf-segformer
Jul 21, 2022
Merged

[SegFormer] TensorFlow port#17910
gante merged 37 commits intohuggingface:mainfrom
sayakpaul:tf-segformer

Conversation

@sayakpaul
Copy link
Member

@sayakpaul sayakpaul commented Jun 28, 2022

This PR adds the SegFormer model in TensorFlow (probably the first Transformer-based segmentation model in TensorFlow for which we have PT weights available).

TODOs

  • Write the foundation components
  • Write the image classification layer
  • Write components w.r.t semantic segmentation
  • Write the semantic segmentation layer
  • Add code examples after call() methods where relevant
  • Write tests
  • Modify other related utilities
  • Create Space to allow users to try out the models (preferably with ONNX to reduce the time?)
  • Create a Colab Notebook

The final two points are unrelated to the merging of this PR.

Cc: @deep-diver

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 28, 2022

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

@LysandreJik
Copy link
Member

LysandreJik commented Jun 28, 2022

Hey @sayakpaul! The code quality error comes from a new release from black. Rebasing on main should solve the issue as you'll benefit from #17918.

@sayakpaul sayakpaul marked this pull request as ready for review June 30, 2022 07:44
@sayakpaul sayakpaul changed the title [WIP] [SegFormer] TensorFlow port [SegFormer] TensorFlow port Jun 30, 2022
@sayakpaul
Copy link
Member Author

sayakpaul commented Jun 30, 2022

@Rocketknight1 @sgugger this PR is now ready for your review. Some things to note:

  • This is the first segmentation model on the TF side that has pre-trained segmentation checkpoints available. Hopefully, it serves as a good foundation for devs contributing TF segmentation models in the future.
  • @deep-diver and I will work on creating a Space and Colab notebook (for off-the-shelf inference and fine-tuning) to allow users to take advantage of a state-of-the-art segmentation model like this one in TF via transformers.

@NielsRogge even though the error in the CI is coming from run_tests_pipelines_tf it seems like the PT test is what is originating the error. Do you mind taking a look?

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Very clean!
The error you mention is caused by the fact that in test_modeling_segformer.py, the config is only imported under an is_torch_available() (which is a mistake). This test file is used by the TF pipeline test to get the small segformer config, hence the errors. Could you just put that import at the top level of this test file? It should resolve the failure.

@sayakpaul sayakpaul requested a review from sgugger July 1, 2022 07:53
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

@Rocketknight1 Could you have a look as well?

@sayakpaul
Copy link
Member Author

Yes @Rocketknight1's comments are needed here as well.

@Rocketknight1
Copy link
Member

Yes, I'm sorry! I went deep on a couple of PRs yesterday and today - one for datasets, the other for XLA in transformers, and haven't had time to review this properly yet. I'll get to it ASAP, though!

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

I just read through the model code, and I'm going to go through the test notes now! Overall, I think the code is very high-quality, and I didn't see any obvious issues anywhere.

If I have one nitpick, it's that I prefer operators like / and > to functions like tf.divide and tf.greater - I think they read more clearly, and they're computationally identical. Still, this is purely an aesthetic preference - if you prefer tf.greater, feel free to keep it that way!

Copy link
Contributor

@gante gante left a comment

Choose a reason for hiding this comment

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

(Approving model_doc/segformer.mdx only, as per @sayakpaul review request) 👍

@sayakpaul
Copy link
Member Author

Thanks, @gante. There were no changes except that. Those were reviewed and approved by @Rocketknight1 and @ydshieh.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

2 tiny naming issues. Thanks @sayakpaul !

src/transformers/models/speech_encoder_decoder/modeling_speech_encoder_decoder.py
src/transformers/models/speech_to_text/modeling_speech_to_text.py
src/transformers/models/speech_to_text_2/modeling_speech_to_text_2.py
src/transformers/models/segformer/modeling_tf_segformer.py
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the doc example for semantic segmentation doesn't actually test anything.

You only test something if you include an expected output. We can include the expected output shape.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to update the PyTorch one as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Both the semantic segmentation classes don't have the add_code_sample_docstrings decorator. Any suggestion on where to best include the expected output shape?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be added below the code snippet. You can find an example here:

['cat', 'cat', 'couch', 'remote', 'remote']

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see anything in SegformerForSemanticSegmentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did include a comment though:

>>> # logits are of shape (batch_size, num_labels, height, width)

Could you provide an example where the expected shape has been provided in the way you're suggesting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind. Added something. Let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes looks good! Should be added for the PyTorch one as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@NielsRogge NielsRogge left a comment

Choose a reason for hiding this comment

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

Very clean implementation, thanks a lot! Left some minor comments.

@gante gante merged commit 561b9a8 into huggingface:main Jul 21, 2022
muellerzr pushed a commit that referenced this pull request Jul 25, 2022
* add: segformer utils and img. classification.

* add: segmentation layer.

* feat: working implementation of segformer.

* chore: remove unused variable.

* add test, remaining modifications.

* remove: unnecessary files.

* add: rest of the files.

Co-authored-by: matt <rocketknight1@gmail.com>

* chore: remove ModuleList comment.

* chore: apply make style.

* chore: apply make fixup-copies.

* add  to check_repo.py

* add decode head to IGNORE_NON_TESTED

* chore: run make style.

* chore: PR comments.

* chore: minor changes to model doc.

* tests: reduction across samples.

* add a note on the space.

* sort importats.

* fix: reduction in loss computation.

* chore: align loss function with that of NER.

* chore: correct utils/documentation_tests.txt

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

* chore: simplify the interpolation of logits in loss computation.

* chore: return transposed logits when return_dict=False.

* chore: add link to the tf fine-tuning repo.

* address pr comments.

* address niels's comments.

* remove from_pt=True since tf weights are in.

* remove comment from pt model.

* address niels's comments.

Co-authored-by: matt <rocketknight1@gmail.com>
Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>
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