Sounds reasonable. Like you said, it shouldn't take long (relatively) to get the PRs that are already in the pipe cleared, so this wouldn't be required for long.
________________________________ From: Kaxil Naik <[email protected]> Sent: Friday, January 21, 2022 5:27 AM To: [email protected] Subject: RE: [EXTERNAL] [PROPOSAL] MyPy incremental reintroduction CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. Sounds good to me On Fri, Jan 21, 2022 at 5:17 PM Jarek Potiuk <[email protected]<mailto:[email protected]>> wrote: Hello everyone, We are very quickly approaching the "0 MyPy errors" state. This was a truly collaborative effort of many people :). Fantastic job ! Now - I will shortly (when we reach 0!) re-enable MyPy as "failing" tests, but this might have a rather nasty side effect that some of the PRs approved and not rebased before might introduce new errors which will break main (and other PRs). I would like to mitigate it by introducing - for a couple of days/week a rule that all PRs should be rebased to main before merging. We already have a check for that (but it is enabled only in case of "sqlalchemy migration" changes). But I would like to enable it for any change that involves python code changes. We would run it for a few days and only enable MyPY as "obligatory" then and keep it enabled for a few days after. Also I think - independently - committers should pay attention when migrating older PRs and ask the authors to rebase (but this is not easily visible as you really need to check status of the branch PR is coming from to see if the PR is rebased). Do you think those are good ideas? J. On Wed, Dec 1, 2021 at 9:59 AM Jarek Potiuk <[email protected]<mailto:[email protected]>> wrote: Thanks Khailid (and Josh who commented on the issue) for volunteering to help! And I think we need more helping hands! I have a very kind request to the whole community to help with the effort as we have whooping 990 (!) issues to fix. Without the help of the community it will take a few weeks at least, but with lots of helping hands we might do it much faster and get much stronger quality protections in place. I merged a change and tested it on a few PRs (https://github.com/apache/airflow/pull/19914, https://github.com/apache/airflow/pull/19912, https://github.com/apache/airflow/pull/19911) to see how this can work on it. We have now a very simple incremental way of fixing mypy that anyone can contribute to - either as separate PRs or as part of their usual contributions. In the issue https://github.com/apache/airflow/issues/19891 we keep an inventory of packages that need fixing (and I will keep it updated). How to help ? VERY EASY: * Checkout latest main. * Run `./breeze build-image` and rebuild the image (you need it to get latest mypy and related libraries included) * `pip install pre-commit` * `pre-commit install That's all that is needed as prerequisites. There are two ways to help: 1) If you make a regular contribution - whenever you commit your code, mypy will fail with the issues that need fixing. It's as simple as that. Just work as normal and make your pre-commit pass. 2) If you want to take on correcting the whole package as a separate PR, just commend in the https://github.com/apache/airflow/issues/19891 that you are working on it and then you can do this: `find <DIRECTORY> -name "*.py" | xargs pre-commit run mypy --files` It will show you what needs to be fixed. One comment from TP who started commenting on the PRs (and I very much agree to) - rather than slapping '# type ignore` everywhere, we should aim to really bring stronger type checking - even if it means slight refactors, or changing the structure of the code. a bit. The aim is not to "pass" mypy, but to improve the quality of our code. Thanks in advance for cooperation! J. J. On Tue, Nov 30, 2021 at 3:20 PM Khalid Mammadov <[email protected]<mailto:[email protected]>> wrote: > > Hi Jarek, > > > Sounds great! > > I can also help to look into these new issues and fix. > > > Regards, > > Khalid > > On 30/11/2021 12:14, Jarek Potiuk wrote: > > Hey all, > > > > I created an issue for that here: > > https://github.com/apache/airflow/issues/19891 but wanted to get a > > lazy consensus on that one (I think it does not really need voting or > > consensus, but If there is anyone who wants to comment/object fill > > free). > > > > For a few weeks MyPy checks have been disabled after the switch to > > Python 3.7 (per https://github.com/apache/airflow/pull/19317). > > > > We should, however, re-enable it back as it is very useful in catching > > a number of mistakes. > > > > I am re-adding the mypy pre-commit now - with mypy bumped to 0.910. > > This version detects far more errors and we should fix them all before > > we switch the CI check back. > > > > The way I am bringing it back in > > https://github.com/apache/airflow/pull/19890: > > > > * mypy will be running for incremental changes in pre-commit, same as > > before. This will enable incremental fixes of the code changed by > > committers who use pre-commits locally > > > > * mypy on CI runs in non-failing mode. When the main pre-commit check > > is run, mypy is disabled, but then it is run as a separate step (which > > does not fail but will show the result of running mypy on all our > > code). This will enable us to track the progress of fixes > > > > We should make a concerted effort now and incrementally fix all the > > mypy incompatibilities - ideally package/by/package to avoid huge code > > reviews. > > > > I'd really appreciate a number of people to contribute, so that we can > > re-enable mypy back fully :). > > > > J.
