Skip to content

Conversation

@robsmith155
Copy link
Contributor

What does this PR do?

I added type hints for the BERTGenerationEncoder and BERTGenerationDecoder classes as requested in #16059 and demonstrated in #16074.

I wasn't completely sure on what to use for the past_key_values argument. I set it to Optional[Tuple[Tuple[torch.FloatTensor]]], but let me know if this is wrong. Also, not sure if I should also add type hints for the BertGenerationConfig class?

@Rocketknight1

Added type hints for the BERTGenerationEncoder and BERTGenerationDecoder
classes.
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 2, 2022

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

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.

Looks good to me, thank you!

@Rocketknight1
Copy link
Member

I wasn't completely sure on what to use for the past_key_values argument. I set it to Optional[Tuple[Tuple[torch.FloatTensor]]], but let me know if this is wrong. Also, not sure if I should also add type hints for the BertGenerationConfig class?

What you put here is fine, and type hints for BertGenerationConfig are nice but optional - if you want to do them you can, but the main thing we're interested in is the core model classes. Let me know either way - if you don't want to do it, this is ready to merge now!

@robsmith155
Copy link
Contributor Author

Okay great, you can go ahead and merge it then. I'll run your notebook to see what else needs to be done and work on some of those instead. Cheers

@Rocketknight1
Copy link
Member

Got it. Thanks for the PR!

@Rocketknight1 Rocketknight1 merged commit 99289c0 into huggingface:main May 5, 2022
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
Added type hints for the BERTGenerationEncoder and BERTGenerationDecoder
classes.
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