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

Switch to mypy part0 #6470

Merged
merged 27 commits into from Oct 19, 2020
Merged

Switch to mypy part0 #6470

merged 27 commits into from Oct 19, 2020

Conversation

m-vdb
Copy link
Collaborator

@m-vdb m-vdb commented Aug 21, 2020

Summary of the first run

Found 812 errors in 136 files (checked 227 source files). Detailed text logs can be found here.

Errors by type

Errors by module:

  • rasa/core/: 511 errors
  • rasa/nlu/: 240 errors
  • rasa/utils/: 50 errors
  • rasa/cli/: 43 errors
  • rasa/train.py: 18 errors
  • rasa/importers/: 14 errors
  • rasa/test.py: 13 errors
  • rasa/server.py: 7 errors
  • rasa/model.py: 7 errors
  • rasa/run.py: 5 errors
  • rasa/validator.py: 2 errors
  • rasa/jupyter.py: 2 errors
  • rasa/constants.py: 1 errors

Files with more that 10 errors

  • rasa/core/training/interactive.py: 39 errors
  • rasa/nlu/classifiers/diet_classifier.py: 34 errors
  • rasa/core/training/structures.py: 31 errors
  • rasa/core/training/generator.py: 28 errors
  • rasa/core/featurizers.py: 26 errors
  • rasa/core/tracker_store.py: 24 errors
  • rasa/cli/train.py: 23 errors
  • rasa/core/actions/forms.py: 20 errors
  • rasa/core/actions/action.py: 19 errors
  • rasa/train.py: 18 errors
  • rasa/nlu/selectors/response_selector.py: 18 errors
  • rasa/core/domain.py: 18 errors
  • rasa/nlu/featurizers/sparse_featurizer/count_vectors_featurizer.py: 17 errors
  • rasa/core/policies/ensemble.py: 16 errors
  • rasa/nlu/training_data/loading.py: 15 errors
  • rasa/core/agent.py: 15 errors
  • rasa/core/slots.py: 14 errors
  • rasa/test.py: 13 errors
  • rasa/core/events/__init__.py: 13 errors
  • rasa/utils/tensorflow/models.py: 12 errors
  • rasa/core/trackers.py: 12 errors
  • rasa/core/test.py: 12 errors
  • rasa/nlu/test.py: 11 errors
  • rasa/nlu/components.py: 11 errors
  • rasa/core/policies/mapping_policy.py: 11 errors
  • rasa/core/policies/ted_policy.py: 10 errors
  • rasa/core/channels/console.py: 10 errors

Strategy

  1. 👉 Find a way to keep mypy and pytype side by side during the transition.
  2. Current files with errors are ignored by mypy in setup.cfg (see 0d35a59). This makes sure we still type check on master when we merge this branch.
  3. We'll work our way through the list of errors and split them in different tasks.
    I've added a ⚠️ in front of the type errors that could yield to immediate errors and I think we should fix them first. Suggested approach:
    i. Group the error types with small numbers of errors together and tackle them into a single issue (<30 errors)
    ii. Treat error types with high number of errors in separate PRs (>30 errors)
    iii. Treat [arg-type] and [assignment] differently: we'll most probably do them in different passes. We might want to split by module in this case.

💡 Note: it doesn't seem to be possible to disable a list of error types in the config, it only works on a line by line basis. It would have been more convenient, but that's OK. Running make types | grep -e "\[error-code\]" does what you want and is really fast.

@m-vdb m-vdb mentioned this pull request Aug 21, 2020
5 tasks
@m-vdb
Copy link
Collaborator Author

m-vdb commented Aug 26, 2020

@tmbo I've done an analysis of the type errors that we have in Rasa and suggested an approach. WDYT? I'd also like to share this to backend engineers to get feedback, they might have ideas (although some of them are off this week)

@tmbo
Copy link
Member

tmbo commented Sep 1, 2020

the approach sounds good 👍 thanks a lot for looking into this, replacing pytype will be a great way to speed up things on the CI but also in local development 🚀

@tmbo
Copy link
Member

tmbo commented Sep 1, 2020

I guess this will allow us to enable an error once we fixed the existing issues to avoid PRs introducing new errors right? (just double checking)

README.md Show resolved Hide resolved
@tmbo
Copy link
Member

tmbo commented Sep 1, 2020

did you already discuss the switch with the backend team to make sure everyone is onboard?

@m-vdb
Copy link
Collaborator Author

m-vdb commented Sep 1, 2020

@tmbo no I haven't, I wanted to get your green light for the plan. Will communicate today

@m-vdb
Copy link
Collaborator Author

m-vdb commented Sep 1, 2020

I guess this will allow us to enable an error once we fixed the existing issues to avoid PRs introducing new errors right? (just double checking)

It's not perfect, as mypy config doesn't let you disable/enable all the error codes. But it lets you ignore files, which is why I included a list of files inside setup.cfg. And if you check the first PR I made to correct some of the type errors, I've updated the setup.cfg as I went: #6511

@wochinge
Copy link
Contributor

wochinge commented Sep 1, 2020

as mypy config doesn't let you disable/enable all the error codes.

this means handling grouped errors by type doesn't work?

I like the iterative approach of doing it on a file per file base a lot. Imo the order of which we tackle them isn't critical as we have managed so far without having any of these resolved (assuming that we not stretch the transition for too long)

@m-vdb
Copy link
Collaborator Author

m-vdb commented Sep 2, 2020

@wochinge it doesn't work at the moment BUT happy news: I just saw that it's available on mypy's master, so I've asked them if we could have a release on pypi in this issue 🚀 That would be awesome if we could use it to disable error codes one by one. That way we can do it iteratively both on the error codes and the files (there are files with a lot of errors, so it will help in these cases). cc @tmbo

@m-vdb m-vdb marked this pull request as ready for review September 4, 2020 12:06
@m-vdb
Copy link
Collaborator Author

m-vdb commented Oct 6, 2020

Quick status update: next mypy release is scheduled for the end of the week. After that, we'll be to enable / disable error codes in a more granular way! 🎉

@m-vdb
Copy link
Collaborator Author

m-vdb commented Oct 12, 2020

aaaaaaand disabling error codes one by one: 85c785d

@m-vdb m-vdb requested review from tmbo and wochinge October 12, 2020 16:29
@m-vdb m-vdb changed the title Switch to mypy Switch to mypy part0 Oct 13, 2020
@m-vdb m-vdb mentioned this pull request Oct 14, 2020
12 tasks
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Excited to have a working type checker! Thanks for driving this!

pyproject.toml Outdated Show resolved Hide resolved
rasa/utils/tensorflow/models.py Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
rasa/nlu/classifiers/diet_classifier.py Outdated Show resolved Hide resolved
rasa/core/policies/ted_policy.py Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
m-vdb and others added 6 commits October 14, 2020 18:28
Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>
Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>
Co-authored-by: Tobias Wochinger <t.wochinger@rasa.com>
@m-vdb m-vdb requested review from tmbo and wochinge October 14, 2020 16:35
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

What is rasa-16-to-rasa-17 for?

pyproject.toml Show resolved Hide resolved
rasa/core/channels/rocketchat.py Outdated Show resolved Hide resolved
@m-vdb m-vdb requested a review from wochinge October 16, 2020 13:00
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

👍

I found 4 more # pytype: disable= comments in ted_policy.py and diet_classifier.py which should also be removed please.

rasa/core/actions/loops.py Outdated Show resolved Hide resolved
rasa/core/channels/botframework.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/diet_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/diet_classifier.py Outdated Show resolved Hide resolved
rasa/shared/core/training_data/structures.py Outdated Show resolved Hide resolved
rasa/shared/core/training_data/structures.py Outdated Show resolved Hide resolved
rasa/shared/nlu/training_data/formats/rasa_yaml.py Outdated Show resolved Hide resolved
rasa/shared/nlu/training_data/message.py Outdated Show resolved Hide resolved
@m-vdb m-vdb removed the request for review from tmbo October 19, 2020 07:58
@rasabot rasabot merged commit 005f8b1 into master Oct 19, 2020
@rasabot rasabot deleted the switch-to-mypy branch October 19, 2020 09:16
@m-vdb m-vdb mentioned this pull request Oct 20, 2020
3 tasks
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.

None yet

4 participants