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

[Community effort] Type Hints #287

Closed
patrickvonplaten opened this issue Aug 31, 2022 · 21 comments · Fixed by #288
Closed

[Community effort] Type Hints #287

patrickvonplaten opened this issue Aug 31, 2022 · 21 comments · Fixed by #288
Labels
good first issue Good for newcomers

Comments

@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Aug 31, 2022

Is your feature request related to a problem? Please describe.

Many important functions and classes are missing type hints

Describe the solution you'd like

It would be great if the following functions could have better type hints to understand what input is expected.

Models

Schedulers

Pipelines

@patrickvonplaten patrickvonplaten added the good first issue Good for newcomers label Aug 31, 2022
@patrickvonplaten
Copy link
Contributor Author

patrickvonplaten commented Aug 31, 2022

Lots of beginner good first issues here for a first contribution. The way it works is easy! Pick some numbers by posting them here as a message here, open a PR to add type hints, ping @anton-l @patil-suraj @pcuenca or @patrickvonplaten on the PR to merge it and we'll tick your numbers of :-)

Example:

I'm taking numbers 39. and 40. and will attach a PR below

Note: Don't forget to run make style after you've finished your PR 🙂

@patrickvonplaten patrickvonplaten linked a pull request Aug 31, 2022 that will close this issue
@patrickvonplaten patrickvonplaten pinned this issue Aug 31, 2022
@patil-suraj patil-suraj reopened this Aug 31, 2022
@leszekhanusz
Copy link
Contributor

Maybe that would also be a good idea to make a GitHub action to check that mypy is happy in all the PRs.

@patrickvonplaten
Copy link
Contributor Author

Usually we're happy with just black, isort and flake8 - are there any advantages of mypy over flake8 ? mypy is a bit too restrictive to me personally

@leszekhanusz
Copy link
Contributor

Well it catches typing errors... That would be a good idea when you add type hints.

@patrickvonplaten
Copy link
Contributor Author

patrickvonplaten commented Aug 31, 2022

What do you think here @anton-l @patil-suraj @pcuenca - note that we don't have it in Transformers and might require some more setting up . Not sure if it's worth it from my point of view, but happy to go for it if others think it's a good idea.

Also cc @LysandreJik @sgugger would love to hear your opinion on mypy

@sgugger
Copy link
Contributor

sgugger commented Aug 31, 2022

I do have some opinions on mypy ;-) It usually ends up requiring the maintainer to write some horrible type hints that no one reading it will understand.

IMO you should focus on having type hints which make your code better documented and are readable and understandable, but enabling mypy is a step ahead as it will send errors if you use one function with a type that doesn't strictly match the type hint (and Python is dynamically typed so this tends to happen a lot for good reasons. For instance in Transformers, we had some issues AutoModel).

@santiviquez
Copy link
Contributor

Hey, if there is no problem. I'm taking numbers 37. and 38. and will attach a PR below.

@daspartho
Copy link
Contributor

I'll be taking numbers 35. and 36. and will attach a PR below.

This was referenced Sep 1, 2022
@sidthekidder
Copy link
Contributor

Hi! I'll take numbers 1 and 2, attaching a PR below.

@santiviquez
Copy link
Contributor

I'll be taking 33 and 34. Will attach a PR below.

@daspartho
Copy link
Contributor

I'll work on numbers 24, 25, and 26. and will attach a PR below.

@santiviquez
Copy link
Contributor

I'll take 12, 13 and 14 and will attach a PR below

@daspartho
Copy link
Contributor

I'll work on 3, 4, 7, 10, and 11 and will attach a PR below

@sidthekidder
Copy link
Contributor

I'll pick up 30, 31 and 32, attaching a PR below

@realsama
Copy link
Contributor

realsama commented Sep 4, 2022

Hello,

I will be taking 5, 6. Will attach a PR.

@daspartho
Copy link
Contributor

I'll work on 15, 16, and 17 and will attach a PR below.

@santiviquez
Copy link
Contributor

I'll take 18, 19 and 20 and will attach a PR below

@santiviquez
Copy link
Contributor

I'll take 21, 22 and 23 and will attach a PR below

@jcmc00
Copy link

jcmc00 commented Sep 5, 2022

I'll take 27, 28 and 29 and will attach a PR

@daspartho
Copy link
Contributor

I'll take on 8 and will attach a PR

@patrickvonplaten
Copy link
Contributor Author

Amazing work everyone here - we can close this issue 🥳 Our IDEs should be happy now

@patrickvonplaten patrickvonplaten unpinned this issue Sep 8, 2022
PhaneeshB pushed a commit to nod-ai/diffusers that referenced this issue Mar 1, 2023
currently setup for tf bert/resnet50
going to refactor test class to avoid having to
add an argument to 50+ files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants