You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/09 07:18:12 UTC
[GitHub] [airflow] xinbinhuang opened a new pull request #15301: Remove python2 related handlings and dependencies
xinbinhuang opened a new pull request #15301:
URL: https://github.com/apache/airflow/pull/15301
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611068654
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611136031
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
Better indeed 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611243227
##########
File path: tests/core/test_logging_config.py
##########
@@ -143,15 +143,6 @@ def settings_context(content, directory=None, name='LOGGING_CONFIG'):
# Create the directory structure
dir_path = os.path.join(settings_root, directory)
pathlib.Path(dir_path).mkdir(parents=True, exist_ok=True)
-
- # Add the __init__ for the directories
- # This is required for Python 2.7
- basedir = settings_root
- for part in directory.split('/'):
- open(os.path.join(basedir, '__init__.py'), 'w').close()
Review comment:
Agreed after reading your comment. I can always learn new things by reading your comments!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#issuecomment-817252829
[The Workflow run](https://github.com/apache/airflow/actions/runs/737503796) is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r610931798
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
@uranusjr what do you think is the workaround here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611068654
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review 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 python2 related tests and dependencies but if anything goes wrong, that's user's problem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r610716447
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
I don't think it's really workaround here. The current test case requires the `on_failure_callback` to change the value of `data['called']` and it doesn't require nonlocal to do that.
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
I don't think it's really a workaround here. The current test case requires the `on_failure_callback` to change the value of `data['called']` and it doesn't require nonlocal to do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611183793
##########
File path: tests/operators/test_python.py
##########
@@ -830,19 +830,6 @@ def f():
with pytest.raises(CalledProcessError):
self._run_as_operator(f)
- def test_python_2(self):
- def f():
- {}.iteritems() # pylint: disable=no-member
-
- self._run_as_operator(f, python_version=2, requirements=['dill'])
-
- def test_python_2_7(self):
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611183211
##########
File path: tests/core/test_logging_config.py
##########
@@ -143,15 +143,6 @@ def settings_context(content, directory=None, name='LOGGING_CONFIG'):
# Create the directory structure
dir_path = os.path.join(settings_root, directory)
pathlib.Path(dir_path).mkdir(parents=True, exist_ok=True)
-
- # Add the __init__ for the directories
- # This is required for Python 2.7
- basedir = settings_root
- for part in directory.split('/'):
- open(os.path.join(basedir, '__init__.py'), 'w').close()
Review 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: https://github.com/apache/airflow/pull/15317#issuecomment-817300262
I think we should simply remove the comment but leave the `__init__.py` adding
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r610869189
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review comment:
I think a better question for this is: should we still enable user to run python2 virtualenv with the `PythonVirtualenvOperator`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611127711
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
I think it is a workaround in the sense that this is not straightforward at first glance without a comment (why does the variable needs to be a dict, looks like `called = False` would work). So if we're removing the comment, it's probably better to use `nonlocal` since it makes it clearer why the straightforward implementation does not work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r610869189
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review comment:
I think a better question for this is: should we still enable user to run python2 virtualenv with the `PythonVirtualenvOperator`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611243227
##########
File path: tests/core/test_logging_config.py
##########
@@ -143,15 +143,6 @@ def settings_context(content, directory=None, name='LOGGING_CONFIG'):
# Create the directory structure
dir_path = os.path.join(settings_root, directory)
pathlib.Path(dir_path).mkdir(parents=True, exist_ok=True)
-
- # Add the __init__ for the directories
- # This is required for Python 2.7
- basedir = settings_root
- for part in directory.split('/'):
- open(os.path.join(basedir, '__init__.py'), 'w').close()
Review comment:
Agreed after reading your comment. I find myself always learning new things by reading your comments!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r610548032
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
Can we also remove the workaround itself?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611127711
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
I this it is a workaround in the sense that this is not straightforward at first glance without a comment (why does the variable needs to be a dict, looks like `called = False` would work). So if we're removing the comment, it's probably better to use `nonlocal` since it makes it clearer why the straightforward implementation does not work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r610848176
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review comment:
@potiuk do we still need to keep this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#issuecomment-817369454
Feedback addressed. @potiuk PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611133419
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
Ahh, it makes sense. After some thought, I think using `MagicMock` for the callback and then call `assert_called_once` would be a better alternative.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang merged pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang merged pull request #15301:
URL: https://github.com/apache/airflow/pull/15301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r610869189
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review comment:
I think a better question for this is: should we still keep the python2 related tests and dependencies in our codebase and CI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611182835
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
This is nice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611152675
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611238612
##########
File path: tests/operators/test_python.py
##########
@@ -830,19 +830,6 @@ def f():
with pytest.raises(CalledProcessError):
self._run_as_operator(f)
- def test_python_2(self):
- def f():
- {}.iteritems() # pylint: disable=no-member
-
- self._run_as_operator(f, python_version=2, requirements=['dill'])
-
- def test_python_2_7(self):
Review comment:
Thanks for the feedback! Totally makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611183944
##########
File path: tests/www/test_views.py
##########
@@ -336,12 +331,7 @@ def test_import_variables_success(self):
content = (
'{"str_key": "str_value", "int_key": 60, "list_key": [1, 2], "dict_key": {"k_a": 2, "k_b": 3}}'
)
- try:
- # python 3+
- bytes_content = io.BytesIO(bytes(content, encoding='utf-8'))
- except TypeError:
- # python 2.7
- bytes_content = io.BytesIO(bytes(content))
Review comment:
Same here. good cach.
##########
File path: tests/www/test_views.py
##########
@@ -336,12 +331,7 @@ def test_import_variables_success(self):
content = (
'{"str_key": "str_value", "int_key": 60, "list_key": [1, 2], "dict_key": {"k_a": 2, "k_b": 3}}'
)
- try:
- # python 3+
- bytes_content = io.BytesIO(bytes(content, encoding='utf-8'))
- except TypeError:
- # python 2.7
- bytes_content = io.BytesIO(bytes(content))
Review comment:
Same here. good catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611036824
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review comment:
I checked AIP 3 and found it was already discussed:
https://issues.apache.org/jira/browse/AIRFLOW-4828
https://github.com/apache/airflow/pull/5449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611243334
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review comment:
Makes sense! Let's keep it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611133419
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
Ahh, it makes sense. After some thought, I think using `MagicMock` for the callback and then call `assert_called_once` is cleaner and would be a better alternative. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#issuecomment-817370603
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611133419
##########
File path: tests/core/test_core.py
##########
@@ -187,7 +187,6 @@ def test_bash_operator_kill(self):
self.fail("BashOperator's subprocess still running after stopping on timeout!")
def test_on_failure_callback(self):
- # Annoying workaround for nonlocal not existing in python 2
Review comment:
Ahh, it makes sense. After some thought, I think using `MagicMock` for the callback and then call `assert_called_once` is cleaner and would be a better alternative.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611183903
##########
File path: tests/www/test_views.py
##########
@@ -318,12 +318,7 @@ def test_import_variables_failed(self):
set_mock.side_effect = UnicodeEncodeError
assert self.session.query(models.Variable).count() == 0
- try:
- # python 3+
- bytes_content = io.BytesIO(bytes(content, encoding='utf-8'))
- except TypeError:
- # python 2.7
- bytes_content = io.BytesIO(bytes(content))
Review comment:
Ah nice. That can indeed be removed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #15301: Remove python2 related handlings and dependencies
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #15301:
URL: https://github.com/apache/airflow/pull/15301#discussion_r611182623
##########
File path: Dockerfile.ci
##########
@@ -123,7 +121,6 @@ ARG RUNTIME_APT_DEPS="\
krb5-user \
ldap-utils \
less \
- libpython2.7-stdlib \
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org