-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Switch to mypy part0 #6470
Conversation
@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) |
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 🚀 |
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) |
did you already discuss the switch with the backend team to make sure everyone is onboard? |
@tmbo no I haven't, I wanted to get your green light for the plan. Will communicate today |
It's not perfect, as |
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) |
@wochinge it doesn't work at the moment BUT happy news: I just saw that it's available on |
Quick status update: next |
aaaaaaand disabling error codes one by one: 85c785d |
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.
Excited to have a working type checker! Thanks for driving this!
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
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.
What is rasa-16-to-rasa-17
for?
This reverts commit 7fa00f4.
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.
👍
I found 4 more # pytype: disable=
comments in ted_policy.py
and diet_classifier.py
which should also be removed please.
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
[arg-type]
: 261 errors[assignment]
: 146 errors[var-annotated]
: 59 errors[return-value]
: 53 errors[union-attr]
: 52 errors[override]
: 47 errors[operator]
: 41 errors[attr-defined]
: 39 errors[index]
: 31 errors[misc]
: 21 errors[return]
: 19 errors[call-arg]
: 17 errors[type-var]
: 5 errors[list-item]
: 5 errors[has-type]
: 5 errors[valid-type]
: 4 errors[dict-item]
: 4 errors[no-redef]
: 2 errors[func-returns-value]
: 1 errorsErrors by module:
rasa/core/
: 511 errorsrasa/nlu/
: 240 errorsrasa/utils/
: 50 errorsrasa/cli/
: 43 errorsrasa/train.py
: 18 errorsrasa/importers/
: 14 errorsrasa/test.py
: 13 errorsrasa/server.py
: 7 errorsrasa/model.py
: 7 errorsrasa/run.py
: 5 errorsrasa/validator.py
: 2 errorsrasa/jupyter.py
: 2 errorsrasa/constants.py
: 1 errorsFiles with more that 10 errors
rasa/core/training/interactive.py
: 39 errorsrasa/nlu/classifiers/diet_classifier.py
: 34 errorsrasa/core/training/structures.py
: 31 errorsrasa/core/training/generator.py
: 28 errorsrasa/core/featurizers.py
: 26 errorsrasa/core/tracker_store.py
: 24 errorsrasa/cli/train.py
: 23 errorsrasa/core/actions/forms.py
: 20 errorsrasa/core/actions/action.py
: 19 errorsrasa/train.py
: 18 errorsrasa/nlu/selectors/response_selector.py
: 18 errorsrasa/core/domain.py
: 18 errorsrasa/nlu/featurizers/sparse_featurizer/count_vectors_featurizer.py
: 17 errorsrasa/core/policies/ensemble.py
: 16 errorsrasa/nlu/training_data/loading.py
: 15 errorsrasa/core/agent.py
: 15 errorsrasa/core/slots.py
: 14 errorsrasa/test.py
: 13 errorsrasa/core/events/__init__.py
: 13 errorsrasa/utils/tensorflow/models.py
: 12 errorsrasa/core/trackers.py
: 12 errorsrasa/core/test.py
: 12 errorsrasa/nlu/test.py
: 11 errorsrasa/nlu/components.py
: 11 errorsrasa/core/policies/mapping_policy.py
: 11 errorsrasa/core/policies/ted_policy.py
: 10 errorsrasa/core/channels/console.py
: 10 errorsStrategy
mypy
andpytype
side by side during the transition.setup.cfg
(see 0d35a59). This makes sure we still type check on master when we merge this branch.I've added a
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.