Remove python2 related handlings and dependencies#15301
Conversation
boring-cyborg
bot
added
area:dev-tools
area:providers
area:webserver
| krb5-user \ | ||
| ldap-utils \ | ||
| less \ | ||
| libpython2.7-stdlib \ |
There was a problem hiding this comment.
I think a better question for this is: should we still keep the python2 related tests and dependencies in our codebase and CI?
There was a problem hiding this comment.
I checked AIP 3 and found it was already discussed:
https://issues.apache.org/jira/browse/AIRFLOW-4828
#5449
There was a problem hiding this comment.
Hi @eladkal, thanks for pointing me to the PR. Let me explain my reason a bit. I don't mean to remove the python_version param, and I think we should keep them to allow the user to run different versions of python. The question that I want to discuss is since we drop support for python2, should we still keep the python2 related tests and dependencies in our codebase and CI? IMO, similar to @kurtqq 's opinion in that PR, we should remove them, and if anything goes wrong when using python2, that's user's problem
There was a problem hiding this comment.
We need to support this case as this is the recommended migration approach for some users who have code that requires libraries only supported by Python 2.7. Airflow is a task orchestration tool and we dropped Python 2.7 support in the core, but we should still support task orchestrations that use Python 2.7. I think we can rethink this issue when we drop support for Airflow 1.10, because then we can assume that all users have migrated to Airflow 2.0 and all users use Python 3+ as the main version.
There was a problem hiding this comment.
I think we should keep definitely keep it. Otherwise PythonVirtualenv operator will not work with ptyhon2.7. This is one of the ways we still tell our users how to run their python 2.7 code even if airflow does not support 2.7 as such. And I think this is no harm to keep it.
There was a problem hiding this comment.
Makes sense! Let's keep it
|
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
| # This is required for Python 2.7 | ||
| basedir = settings_root | ||
| for part in directory.split('/'): | ||
| open(os.path.join(basedir, '__init__.py'), 'w').close() |
There was a problem hiding this comment.
Are we sure changing those packages into "namespace" packages is a good idea here? I think it is the result of miconception that in Python 3.3 we can simply remove __init__.py everywehere, but it is not really true. See my comment here: #15317 (comment)
I think we should simply remove the comment but leave the __init__.py adding
There was a problem hiding this comment.
Agreed after reading your comment. I find myself always learning new things by reading your comments!
|
|
||
| self._run_as_operator(f, python_version=2, requirements=['dill']) | ||
|
|
||
| def test_python_2_7(self): |
There was a problem hiding this comment.
I do not think we should remove those. We've never decided on dropping 2.7 as a way to exxecute the Python operator. That was always an "escape hatch" for people who wanted to still execute their python 2.7 code even if it is not used/needed by Airflow itself. If we want to drop it, it should be discussed at the devlist because it has serious implications for our users who might still have some python 2.7 code around.
Besides having to install the 2_7 library in the image, It has completely no impact on the code of Airflow, so there is no particular reason to remove it IMHO. It will not impact the way we are developing Airflow, it does not change any tooling support, it is just a way to nicely execute external python 2.7 code from within Airflow codebase.
There was a problem hiding this comment.
Thanks for the feedback! Totally makes sense.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
There are a couple of good catches and things that could be removed.
However, I think we cannot remove external python 2.7 execution. This is a backward incompatible change that might heavily impact our users. And it has no relation whatsoever with Airflow itself supporting only 3.6+. Those are two different things.
And I think there is no particular reason to do so, especially that it has almost zero impact for the development (just one library to install in the image) so it has no negative consequences if we keep it in.
We might consider it for Airflow 3.0 which will be the first one where we can drop features. The "python" operator is one of the "core" operators of Airflow, so I do not think we can drop the ability of running Python 2.7 in a backwards-compatible release.
xinbinhuang
force-pushed
the
remove-python2
branch
from
21c976c to
439b47d
Compare
|
Feedback addressed. @potiuk PTAL. |
|
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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
full tests needed
xinbinhuang
force-pushed
the
remove-python2
branch
from
98da8b3 to
2865ab0
Compare
5 participants
Small cleanup to remove python2 related handling and dependencies
^ 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.