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

Incorporate mypy into lint stage #2858

Merged
merged 2 commits into from
Jun 4, 2021
Merged

Incorporate mypy into lint stage #2858

merged 2 commits into from
Jun 4, 2021

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Jun 3, 2021

Addresses #2550

@kamathhrishi does this PR look ok to you? I thought it would be nice to:

  1. Incorporate mypy into make lint, which we often use to quickly check the codebase. Since we already have a travis stage to make lint, this PR moves mypy into the lint stage in .travis.yml. I've also added mypy to the dev requirements in setup.py
  2. Also check the examples/ and scripts/ directories. Currently examples/ fails so I've commented it out in the make lint command. I guess we can fix examples in a later PR (lower priority than the library fixes you're working on)

note mypy was giving me weird errors for the warn_incomplete_stub changes:

setup.cfg: [mypy-pyro._version.*]: Per-module sections should only specify per-module flags (warn_incomplete_stub)

@fritzo fritzo requested a review from eb8680 June 3, 2021 21:14
Copy link
Contributor

@kamathhrishi kamathhrishi left a comment

Choose a reason for hiding this comment

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

@fritzo Looks good to me. Adding it with the lint stage and making it part of dev requirements. The idea of checking scripts as part of mypy is a good idea for pythonic ones.

Yes in the future annotating type hints for examples would be good. Easy for users to be able to understand various data types that can be accommodated by various modules.

@kamathhrishi
Copy link
Contributor

kamathhrishi commented Jun 4, 2021

Shall I create issues for adding type hints to examples? Further, how about finding a way we mypy throws a warning each time we have ignored a module or used #type ignore somewhere. I wasn't able to find how to do it.

@fritzo
Copy link
Member Author

fritzo commented Jun 4, 2021

Thanks for reviewing @kamathhrishi!

Shall I create issues ...

I think it will be easiest if you create a single general mypy issue with a checklist for all the tasks that remain. I don't know how to give you edit rights to the original issue #2550.

how about finding a way we mypy throws a warning ...

Sure, once we're done annotating most of the codebase we could write a little one-line script using e.g. grep.

Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

LGTM

@eb8680 eb8680 merged commit a9ab3e0 into dev Jun 4, 2021
@eb8680 eb8680 deleted the lint-mypy branch June 4, 2021 16:58
@kamathhrishi
Copy link
Contributor

kamathhrishi commented Jun 6, 2021

Thanks for reviewing @kamathhrishi!

Shall I create issues ...

I think it will be easiest if you create a single general mypy issue with a checklist for all the tasks that remain. I don't know how to give you edit rights to the original issue #2550.

how about finding a way we mypy throws a warning ...

Sure, once we're done annotating most of the codebase we could write a little one-line script using e.g. grep.

@fritzo Since you have writing permission for the repository, you can edit it. I will write a draft here which you can include there. So I thought you can just include a checklist like this:

Add type hints to modules

  • contrib
  • distributions
  • infer
  • nn
  • ops
  • optim
  • params
  • poutine
  • scripts

Additional functionality for type hints include:

  • Give warnings on #type ignore in codebase
  • Give warnings on modules skipped by mypy

A tag of "Good first Issue" will be good.

Grey seems like a good idea for finding #type ignores

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants