[AIRFLOW-2973] Add Python 3.6 to Supported Prog Langs

[kaxil]

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-2973
  • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.

Description

• Here are some details about my PR, including screenshots of any UI changes:
  I remember Fokko and Joy had tested Airflow on Python 3.6. Also, DigitalOcean has also been using Airflow with 3.6 as mentioned in the mailing list: https://lists.apache.org/thread.html/4b3ab976ad268f3da96c5317290d11120c26c7e0f92886eef5b8b50f@%3Cdev.airflow.apache.org%3E

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

• Passes git diff upstream/master -u -- "*.py" | flake8 --diff

[kaxil]

cc @bolkedebruin @Fokko

[ashb]

Is it worth adding/changing Py 3.5 to Py 3.6 in Travis? (I don't think we want to have both py 3.5 and py 3.6 tests as not that much has changed and we don't want more runtime) but...?

[bolkedebruin]

Agree with @ashb

[codecov-io]

Codecov Report

Merging #3815 into master will increase coverage by 0.66%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3815      +/-   ##
==========================================
+ Coverage   76.67%   77.34%   +0.66%     
==========================================
  Files         199      203       +4     
  Lines       16186    15817     -369     
==========================================
- Hits        12410    12233     -177     
+ Misses       3776     3584     -192

Impacted Files	Coverage Δ
airflow/sensors/s3_prefix_sensor.py	0% <0%> (-100%)	⬇️
airflow/operators/slack_operator.py	0% <0%> (-97.37%)	⬇️
airflow/sensors/s3_key_sensor.py	30.3% <0%> (-69.7%)	⬇️
airflow/example_dags/example_python_operator.py	78.94% <0%> (-15.79%)	⬇️
airflow/utils/helpers.py	71.16% <0%> (-13.21%)	⬇️
airflow/hooks/mysql_hook.py	78% <0%> (-12%)	⬇️
airflow/sensors/sql_sensor.py	90.47% <0%> (-9.53%)	⬇️
airflow/configuration.py	83.33% <0%> (-6.09%)	⬇️
airflow/models.py	88.74% <0%> (-3.31%)	⬇️
airflow/utils/state.py	93.33% <0%> (-3.1%)	⬇️
... and 73 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e703d6b...a20a648. Read the comment docs.

[kaxil]

@bolkedebruin @ashb Do you guys suggest to add Python 3.4 and Python 3.6 both to Travis? As currently just have py27 and py35 on Travis.

[Fokko]

This would give too much load on Travis. Apache infra paying for cpu hours.

[kaxil]

@Fokko Agree with that, so do you like to keep it as it is or changing py3.5 to py3.6 and keep py2.7 and py3.6 ?

[bolkedebruin]

2.7 and 3.6 (and maybe 3.7 instead)

[tedmiston]

@kaxil @Fokko I also put together a quick complementary PR #3816 adding / upgrading to 3.6 in all of the places. CI is still running so I may expect some test failures but hopefully nothing major. I would be happy to merge this into Kaxil's current PR or as a follow-on PR.

Personally I would prefer Travis to run against 3.6 over 3.5. It's been out for a while now, and AFAIK we don't have a compelling reason to prefer 3.5 today.

That said it would be nice if we could have some sort of nightly-ish build to run on all supported subversions for a very thorough test.

[kaxil]

@tedmiston I have merged the changes to this PR but as a follow-up to this PR, I suppose it would be great if you can test Python 3.7 and as Bolke mentioned, we can then replace 3.6 with 3.7 :)

[kaxil]

The way CI works now with Docker, we will have to change bits on our Docker CI Image as well. I have pinged @gerardo on the Dockerised PR to help the case where we would have to run tests for multiple python versions. I am hence just keeping the scope of this PR to add 3.6 to setup.py, or we can close this PR entirely and have first sort out our CI pipeline and can add these changes over there?

cc @ashb @Fokko

[gerardo]

(posting again in here)

@kaxil give we're using tox for this, it should be a matter of just adding/updating the runtimes inside the tox file here and here and then pass the desired python version through the Travis script as we do now.

[Fokko]

@gerardo This does not make sense to me. Why are we installing both 2 and 3 in the ci image? https://github.com/apache/incubator-airflow-ci/blob/master/Dockerfile

Ideally we would have a Python 2 and Python 3 image, but this can become complicated quickly.

[gerardo]

@Fokko The only reason 2 versions are being installed is that I wanted to follow the CI process as close as it was before merging the docker-ci changes. At the time, the tox target and the python version to be used were being matched (run a python3 tox target and run tox under python3), although I never understood the reason. We only use the system python to install tox.

I only remember coming across a single issue with a PythonVirtualOperator test failing because of a version mismatch (tox running python3 and PythonVirtualOperator trying to use the system python, which could be python2), but if that happens, it should be simple to fix.

[kaxil]

@gerado I already tried changing that, it says python3.6 interpreter not found.

Here is the link to the Travis job:

https://travis-ci.org/apache/incubator-airflow/builds/421714589

[gerardo]

@kaxil mmm, it seems it needs to be installed in the system to be used: https://tox.readthedocs.io/en/latest/example/general.html#basepython-defaults-overriding

And it seems the only python3 version available in Ubuntu Xenial is 3.5.1: https://packages.ubuntu.com/search?keywords=python3-dev&searchon=names&suite=xenial&section=all

It might be a matter of having a separate image (like @Fokko suggested) using a more recent Ubuntu (Artful or Bionic). The official python docker images are also based in Debian, that's another option.

[kaxil]

Would you be able to do that if you have got time?

[gerardo]

@kaxil sure! I have time this week to work on this.

[Fokko]

@gerardo On the incubator-airflow-ci we can create two branches, one for Python 2 and one for Python 3. I can configure the docker-hub to publish two images incubator-airflow-ci:2 and incubator-airflow-ci:3 for example

[tedmiston]

@kaxil Sure. Btw, I've started a 3.7 test branch currently living here - https://github.com/astronomerio/incubator-airflow/tree/python-3.7-experimental.

It looks like the merge of #3816 into here yesterday was reverted. It sounds like this PR may be on hold until the Docker image is reworked to support multiple subversions of Python 3. Is that correct?

@gerardo Can you tag me in that PR ^ when you're done? I'd like to make a similar change in my follow up PR to support Python 3.7 (perhaps with an optional flag for now so we don't burn Apache Infra CI time).

[kaxil]

That's awesome @gerardo . Looking forward for it.

@tedmiston Great. Would love to see Py3.7 PR, I think #3816 can be converted to be a 3.7 Upgrade PR. So once 3.6 is fixed, you can probably overwrite it with 3.7.

[gerardo]

@kaxil sure! I have time this week to work on this.

BTW, I hugely underestimated the amount of free time I had available 😂

Here's the PR that creates separate images for python2 and python3: apache/airflow-ci#4

@tedmiston Regarding python3.7, it's still not available to any Ubuntu version, not even through a PPA. Even cosmic cuttlefish, which hasn't been released yet, supports only 3.6.

[Fokko]

@gerardo When updating the CI, we run into problems since we need a to point the Python3 tests to the Python 3 image: https://travis-ci.org/Fokko/incubator-airflow

I think we need to get rid of the python3 and python3.6 command and generalize everything to just python, and pick the correct base images if we run on either python2 or python3.

I think updating to Python 3.7 doesn't make sense since a lot of companies are still on 3.6 (or even lower :-)

[gerardo]

@Fokko @kaxil @tedmiston
I've done some more work on apache/airflow-ci#4, but I've found a few issues with the Python3.6 upgrade, plus other issues with some assumptions that the test suite makes:

• tests.operators.test_virtualenv_operator.test_python_3 fails when run with the python2 image (logs). Using skipIf to check what python version is present should be enough. And maybe we should do the same with test_python_2 and test_python_2.7
• tests.operators.test_virtualenv_operator.test_string_args is a weird test. It uses a method called _invert_python_major_version() to use the opposite version to the one available, which is causing problems with python2 tests. Do you guys understand this test?
• There was a change between python3.5 and python3.6 in the error message emitted when an object is not serializable (logs). It's just a matter of changing the test to check for the new message format.
• One of the tz tests is failing as well, more specifically, test_following_previous_schedule_daily_dag_CEST_to_CET (logs). I'm not sure where to start. It seems to happen only with python3.6.

[Fokko]

Give me some time to look into this.

[bryantbiggs]

It looks like this line should be dropped since 3.4 is not tested in the CI process

[bryantbiggs]

disregard - this has been fixed in #4259

[holdenk]

Hi @gerardo I did some looking into tests.operators.test_virtualenv_operator.test_string_args as well. Looking at the Python operator description string args is the only supported method of allowing configuration when the Python versions don't match between airflow and operator -- which would be why it does the test through these string arguments.

It looks like if we want to make that test workable in CI then the Python 2 image needs Python 3 installed as well and vice versa just different default Pythons configured.

[Fokko]

Already the case https://github.com/apache/airflow/blob/master/setup.py#L429

👍