Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type hints for tf models batch 1 #25853

Conversation

nablabits
Copy link
Contributor

@nablabits nablabits commented Aug 30, 2023

What does this PR do?

Addresses some of the type hints for tf models in #16059:

  1. TFBlipTextModel
  2. DPRModel family:
    1. TFDPRContextEncoder
    2. TFDPRQuestionEncoder
    3. TFDPRReader
  3. LED family
    1. TFLEDForConditionalGeneration
    2. TFLEDModel
  4. TFLxmertForPreTraining
  5. Marian family
    1. TFMarianMTModel
    2. TFMarianModel
  6. Rag family
    1. TFRagModel
    2. TFRagTokenForGeneration

Who can review?

@Rocketknight1 please

is_decoder=False,
training=None,
):
input_ids: TFModelInputType | None = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen a nice mix of this TFModelInputType and tf.Tensor | None in a few models, so whenever I had to fill input_ids I defaulted to the former as the instructions of the issue say. If this is not the case, is there a way to tell apart when each one should be used?

decoder_attention_mask=None,
head_mask=None,
decoder_head_mask=None,
encoder_outputs: Optional[Union[Tuple, TFLEDEncoderBaseModelOutput]] = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced this to the | format for consistency. There are other places where there's a mix of approaches that I didn't fix as all the type hints were present. Let me know if you want me to give them some consistency

Copy link
Member

Choose a reason for hiding this comment

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

I think the | format is preferred! The main reason is that it looks nicer in the automatically generated documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, great, I've then added a commit to make them consistent within the method

@nablabits
Copy link
Contributor Author

It seems that some tests failed because there was a connection error to huggingface.co:

image

And

image

I don't seem to have permissions to retrigger them so anyone who has can do so. Alternatively we can wait until some comment needs to be addressed (pretty likely)

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.

This looks great! Good catches with the copy-pasted docstrings from torch as well. Is this ready to merge, or do you want to add anything else?

@@ -1571,7 +1571,7 @@ class TFLEDSeq2SeqLMOutput(ModelOutput):
- 0 for tokens that are **masked**.

[What are attention masks?](../glossary#attention-mask)
decoder_input_ids (`tf.LongTensor` of shape `(batch_size, target_sequence_length)`, *optional*):
decoder_input_ids (`tf.Tensor` of shape `(batch_size, target_sequence_length)`, *optional*):
Copy link
Member

Choose a reason for hiding this comment

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

Great catch!

decoder_attention_mask=None,
head_mask=None,
decoder_head_mask=None,
encoder_outputs: Optional[Union[Tuple, TFLEDEncoderBaseModelOutput]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

I think the | format is preferred! The main reason is that it looks nicer in the automatically generated documentation

@HuggingFaceDocBuilderDev

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

@nablabits
Copy link
Contributor Author

This looks great! Good catches with the copy-pasted docstrings from torch as well. Is this ready to merge, or do you want to add anything else?

Now with the consistency tweaks I'm happy for you to merge, thanks 🤗

@Rocketknight1 Rocketknight1 merged commit eaf5e98 into huggingface:main Aug 31, 2023
@nablabits nablabits deleted the feature/add-type-hints-for-tf-models-batch-1 branch September 14, 2023 07:06
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
* Add type hints to `TFBlipTextModel`

* Add missing type hints to DPR family models

* Add type hints to `TFLEDModel`

* Add type hints to `TFLxmertForPreTraining`

* Add missing type hints to `TFMarianMTModel` and `TFMarianModel`

* Add missing type hints to `TFRagModel` & `TFRagTokenForGeneration`

* Make type hints annotations consistent
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
* Add type hints to `TFBlipTextModel`

* Add missing type hints to DPR family models

* Add type hints to `TFLEDModel`

* Add type hints to `TFLxmertForPreTraining`

* Add missing type hints to `TFMarianMTModel` and `TFMarianModel`

* Add missing type hints to `TFRagModel` & `TFRagTokenForGeneration`

* Make type hints annotations consistent
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
* Add type hints to `TFBlipTextModel`

* Add missing type hints to DPR family models

* Add type hints to `TFLEDModel`

* Add type hints to `TFLxmertForPreTraining`

* Add missing type hints to `TFMarianMTModel` and `TFMarianModel`

* Add missing type hints to `TFRagModel` & `TFRagTokenForGeneration`

* Make type hints annotations consistent
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.

3 participants