Fix failing pre-commits after making 3.7 default #19334

potiuk · 2021-10-30T17:52:41Z

In order to keep consistency, both mypy and flake were always
using Python 3.6 - simply to make sure that the same errors
are produced for everyone. When default python version was
changed in #19317 the regular PR static checks started to fail
because Python 3.6 image is no longer built for "regular" PRs
(it's only built when "full-tests-needed" is set or when
the build runs in "main" as result of push or schedule).

Unfortunately those "defaults" could not be read from
"default python version" easily, because they are also
executed in pre-commits, which do not have the same
initialization as breeze or other CI scripts so there
are literally 3 more places where Python 3.6 should be
changed to 3.7.

This PR fixes it.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

potiuk · 2021-10-30T17:53:36Z

This should fix the 2nd (and last widespread) failure in regular PRs after chaning 3.7 to be default :).

potiuk · 2021-10-30T18:06:46Z

Hmm. Seems like running current mypy with 3.7 ACTUALLY causes problems:

/usr/local/lib/python3.7/site-packages/git/cmd.py:71: error: INTERNAL ERROR -- Please try using mypy master on Github:

potiuk · 2021-10-30T18:12:01Z

All right - I feel like good opportunity to migrate to latest mypy 😱 .

We had it fixed to 0.770 which was released 11th May 2020. Sounds like we are little behind.

potiuk · 2021-10-30T18:23:30Z

CC: @dstandish -> I am not sure how Many problems we will have now with moving mypy from 0.770 to 0.910, but imagine if every developer had their own version of mypy installation with their own versions of other dependencies in their own version of Python.

This is is why running mypy in the same docker with the same python + the same dependencies is a very good option (even if it is slower on MacOS) :(.

In this case just changing Image from 3.6 to 3.7 to run mypy caused:

/usr/local/lib/python3.7/site-packages/git/cmd.py:71: error: INTERNAL ERROR -- Please try using mypy master on Github:

But I expect quite a number of new errors reported with newer version of Mypy.

potiuk · 2021-10-30T18:29:48Z

BTW. I am waiting (two weeks) for my new McBook Pro (Late 2021 - with notch y'know ) - so I hope I will be able to take a closer look and wit this one stone (it's much heavier and bulkier than previous ones) kill several birds which were kinda` flying loose:

Docker Image for ARM (BTW M1 could be another reason for slow-downs you saw - so far our images are x86 only and on M1 they use emulation - that's why they can be WAY slower)
Speedup some docker operations on MacOS - mypy for one
Finally try some of the new (ish) features in docker stack (mostly BuildKit - which I think will be much easier when it comes to making multi-platform images)

dstandish · 2021-10-30T19:50:34Z

BTW. I am waiting (two weeks) for my new McBook Pro (Late 2021 - with notch y'know )

very exciting. and good luck to you 😅

github-actions · 2021-10-30T19:51:36Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

uranusjr · 2021-10-30T23:09:51Z

Merging into #19333

potiuk · 2021-10-31T09:21:26Z

We still need to fix some resulting mypy errors @uranusjr I am afraid :)

potiuk · 2021-10-31T09:27:02Z

Als this one is really separate from fixing the warnings (I will try to fix the mypy errors today unless someone bits me to it).

potiuk · 2021-11-01T07:56:09Z

I will not be near my PC whole day today - but maybe someone could simply comment out the failing mypy precommits temporarily until we fix it ?

potiuk · 2021-11-01T09:41:46Z

Added PR to remove mypy checks temporarily #19345

After we moved to Python 3.7 as default, it had a ripple effect that MyPy checks started failing. We aim to fix it permanently in apache#19334 but this needs a bit more changes, so for the moment we skip the checks.

After we moved to Python 3.7 as default, it had a ripple effect that MyPy checks started failing. We aim to fix it permanently in #19334 but this needs a bit more changes, so for the moment we skip the checks.

uranusjr

Didn’t read everything but there are some things we can do a bit better

uranusjr · 2021-11-16T07:17:34Z

airflow/api_connexion/endpoints/dag_endpoint.py

-def delete_dag(dag_id: str, session: Session):
+def delete_dag(dag_id: str, session):


I don’t think this is supposed to fail?

uranusjr · 2021-11-16T07:17:59Z

airflow/cli/simple_table.py

@@ -70,13 +70,13 @@ def print_as_plain_table(self, data: List[Dict]):
            self.print("No data found")
            return
        rows = [d.values() for d in data]
-        output = tabulate(rows, tablefmt="plain", headers=data[0].keys())
+        output = tabulate(rows, tablefmt="plain", headers=list(data[0].keys()))


Suggested change

output = tabulate(rows, tablefmt="plain", headers=list(data[0].keys()))

output = tabulate(rows, tablefmt="plain", headers=list(data[0]))

uranusjr · 2021-11-16T07:18:57Z

airflow/config_templates/default_webserver_config.py

-from airflow.www.fab_security.manager import AUTH_DB
+from airflow.www.fab_security.manager import AUTH_DB  # type: ignore

-# from airflow.www.fab_security.manager import AUTH_LDAP
-# from airflow.www.fab_security.manager import AUTH_OAUTH
-# from airflow.www.fab_security.manager import AUTH_OID
-# from airflow.www.fab_security.manager import AUTH_REMOTE_USER
+# from airflow.www.fab_security.manager import AUTH_LDAP  # type: ignore
+# from airflow.www.fab_security.manager import AUTH_OAUTH  # type: ignore
+# from airflow.www.fab_security.manager import AUTH_OID  # type: ignore
+# from airflow.www.fab_security.manager import AUTH_REMOTE_USER  # type: ignore


uranusjr · 2021-11-16T07:23:37Z

airflow/kubernetes/kubernetes_helper_functions.py

+    if not run_id:
+        # TODO: Is this the right way of handling it?
+        raise ValueError("Run Id has to be defined either by `run_id` annotation "
+                         "or derived from `execution_Date")


We can also just do

(run_id,) = ( session.query(TaskInstance.run_id) ... .one() )

and let SQLAlchemy raise when there’s not a matching task instance found (it shouldn’t happen).

potiuk · 2021-11-16T16:06:48Z

Didn’t read everything but there are some things we can do a bit better

Yeah. It's a draft - I want to make all the changes first and split it into a series of "targetted" PRs (otherwise it will be impossible to review) and re-enable mypy after I merge them all.

This PR enables mypy back as pre-commit for local changes after the apache#19317 switched to Python 3.7 but also it separates out mypy to a separate non-failing step in CI. In the CI we will be able to see remaining mypy errors. This will allow us to gradually fix all the mypy errors and enable mypy back when we got all the problems fixed.

In order to keep consistency, both mypy and flake were always using Python 3.6 - simply to make sure that the same errors are produced for everyone. When default python version was changed in apache#19317 the regular PR static checks started to fail because Python 3.6 image is no longer built for "regular" PRs (it's only built when "full-tests-needed" is set or when the build runs in "main" as result of push or schedule). Unfortunately those "defaults" could not be read from "default python version" easily, because they are also executed in pre-commits, which do not have the same initialization as `breeze` or other CI scripts so there are literally 3 more places where Python 3.6 should be changed to 3.7. This PR fixes it.

potiuk · 2021-12-12T18:21:51Z

MyPy effort is now spread among MANY people :)

After we moved to Python 3.7 as default, it had a ripple effect that MyPy checks started failing. We aim to fix it permanently in #19334 but this needs a bit more changes, so for the moment we skip the checks. (cherry picked from commit 1f0db28)

potiuk requested a review from ashb as a code owner October 30, 2021 17:52

boring-cyborg bot added the area:dev-tools label Oct 30, 2021

potiuk requested review from eladkal, kaxil, mik-laj and uranusjr October 30, 2021 17:53

dstandish approved these changes Oct 30, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Oct 30, 2021

uranusjr closed this Oct 30, 2021

potiuk reopened this Oct 31, 2021

potiuk mentioned this pull request Nov 1, 2021

Temporarily remove mypy checks to stop PRs from failing #19345

Merged

kaxil approved these changes Nov 1, 2021

View reviewed changes

potiuk force-pushed the fix-failing-pre-commits-after-making-3.7-default branch from 12fc0cf to 8a66ea0 Compare November 2, 2021 14:42

potiuk marked this pull request as draft November 2, 2021 14:42

potiuk force-pushed the fix-failing-pre-commits-after-making-3.7-default branch from 8a66ea0 to 9f0c641 Compare November 2, 2021 14:44

potiuk force-pushed the fix-failing-pre-commits-after-making-3.7-default branch 4 times, most recently from 2a1bffe to 4fc4ef1 Compare November 4, 2021 18:51

uranusjr reviewed Nov 16, 2021

View reviewed changes

potiuk force-pushed the fix-failing-pre-commits-after-making-3.7-default branch from 4fc4ef1 to db2117a Compare November 16, 2021 15:39

potiuk force-pushed the fix-failing-pre-commits-after-making-3.7-default branch from db2117a to ca641d7 Compare November 30, 2021 11:03

potiuk mentioned this pull request Nov 30, 2021

Re-enable MyPy #19891

Closed

10 tasks

potiuk force-pushed the fix-failing-pre-commits-after-making-3.7-default branch from ca641d7 to ce35e16 Compare November 30, 2021 19:33

potiuk force-pushed the fix-failing-pre-commits-after-making-3.7-default branch from ce35e16 to aa93e28 Compare November 30, 2021 19:38

potiuk closed this Dec 12, 2021

potiuk deleted the fix-failing-pre-commits-after-making-3.7-default branch July 29, 2022 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failing pre-commits after making 3.7 default #19334

Fix failing pre-commits after making 3.7 default #19334

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

dstandish commented Oct 30, 2021

github-actions bot commented Oct 30, 2021

uranusjr commented Oct 30, 2021

potiuk commented Oct 31, 2021

potiuk commented Oct 31, 2021

potiuk commented Nov 1, 2021

potiuk commented Nov 1, 2021

uranusjr left a comment

uranusjr Nov 16, 2021

uranusjr Nov 16, 2021

uranusjr Nov 16, 2021

uranusjr Nov 16, 2021

potiuk commented Nov 16, 2021

potiuk commented Dec 12, 2021

		def delete_dag(dag_id: str, session: Session):
		def delete_dag(dag_id: str, session):

	output = tabulate(rows, tablefmt="plain", headers=list(data[0].keys()))
	output = tabulate(rows, tablefmt="plain", headers=list(data[0]))

Fix failing pre-commits after making 3.7 default #19334

Fix failing pre-commits after making 3.7 default #19334

Conversation

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

potiuk commented Oct 30, 2021

dstandish commented Oct 30, 2021

github-actions bot commented Oct 30, 2021

uranusjr commented Oct 30, 2021

potiuk commented Oct 31, 2021

potiuk commented Oct 31, 2021

potiuk commented Nov 1, 2021

potiuk commented Nov 1, 2021

uranusjr left a comment

Choose a reason for hiding this comment

uranusjr Nov 16, 2021

Choose a reason for hiding this comment

uranusjr Nov 16, 2021

Choose a reason for hiding this comment

uranusjr Nov 16, 2021

Choose a reason for hiding this comment

uranusjr Nov 16, 2021

Choose a reason for hiding this comment

potiuk commented Nov 16, 2021

potiuk commented Dec 12, 2021