-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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 gptneox models #17858
Add type hints for gptneox models #17858
Conversation
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
output_hidden_states=None, | ||
return_dict=None, | ||
): | ||
input_ids: Optional[torch.LongTensor] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the documentation of GPT-NeoX, the parameter input_ids
is not optional for the class GPTNeoXModel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is true, Optional
in Python types has the very specific meaning that the argument can take the value None
. See the typing docs for more info on this.
I agree it's not exactly intuitive, but the annotation here is correct as the argument has a default value of None
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for sharing this information. I'm using this PR as an example to continue addressing the issue 16059. Again, thanks 😃
* feat: Add type hints for GPTNeoxForCausalLM and GPTNeoXModel * fix: removed imported Dict type * fix: Removed unused List import
* feat: Add type hints for GPTNeoxForCausalLM and GPTNeoXModel * fix: removed imported Dict type * fix: Removed unused List import
What does this PR do?
Adding missing type hints for
GPTNeoxForCausalLM
andGPTNeoXModel
as referenced in this issue.(#16059 (comment)).Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community please feel free to review😄
@Rocketknight1