And the Oscar goes to: https://github.com/apache/airflow/pull/21020 :)
On Fri, Jan 21, 2022 at 6:30 PM Ferruzzi, Dennis <ferru...@amazon.com.invalid> wrote: > 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 <kaxiln...@gmail.com> > *Sent:* Friday, January 21, 2022 5:27 AM > *To:* dev@airflow.apache.org > *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 <ja...@potiuk.com> 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 <ja...@potiuk.com> 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 >>> <khalidmammad...@gmail.com> 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. >>> >>
