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/09/05 12:45:19 UTC

[GitHub] [airflow] potiuk opened a new pull request #18037: Add Python2 to installed packages

potiuk opened a new pull request #18037:
URL: https://github.com/apache/airflow/pull/18037

As of August 2021, the buster-slim python images, no longer
contain python2 packages. We still support running Python2 via
PythonVirtualenvOperator and our tests started to fail when
we run the tests in `main` - those tests always pull and build
the images using latest-available buster-slim images.

Our system to prevent PR failures in this case has proven to be
useful - the main tests failed to succeed so the base images
we have are still using previous buster-slim images which still
contain Python 2.

This PR adds python2 to installed packages - on both CI images
and PROD images. For CI images it is needed to pass tests, for
PROD images, it is needed for backwards-compatibility.

<!--
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/main/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/main/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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #18037: Add Python2 to installed packages

Posted by GitBox <gi...@apache.org>.

potiuk commented on pull request #18037:
URL: https://github.com/apache/airflow/pull/18037#issuecomment-913150704


   > Maybe we should also add a marker to skip Python 2 tests if `python2` is not available? THis can be dealed separately though.
   
   I think that's not good idea. If we did that before, we would have never known that python slim-buster suddenly without any warning  dropped python2 from their image and we would not know Airflow PROD image suddenly lost the capacity of running Python 2 via PythonVirtualenvOperator. For now this is our official recommendation if you still want to use Python2 when migrating to Python2 (and for some people it is still important). So in a way the test actually tests what we promised.
   
   We should simply remove both test and python2 when we drop this "Python2" fallback (likely Airflow 3).


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #18037: Add Python2 to installed packages

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on pull request #18037:
URL: https://github.com/apache/airflow/pull/18037#issuecomment-913149986


   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.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #18037: Add Python2 to installed packages

Posted by GitBox <gi...@apache.org>.

potiuk commented on pull request #18037:
URL: https://github.com/apache/airflow/pull/18037#issuecomment-913152498

> we would have never known that python slim-buster suddenly without any warning dropped python2 from their image

BTW. This is the perfect example of why I setup our CI in this way with constraints and image cache (including caching the base python image). Our `main` builds (both merges and "scheduled" builds) have the setting to a) use latest released python image as a base and b) rebuild the image from scratch and c) use "eager-upgrade" strategy.

This is to catch exactly those kind of changes where some external dependency break our tests (and likely breaks our promises). The whole "constraints" mechanism for CI is built around that to catch precisely this kind of error in a gentle way - so that when such builds fail in main, we have the time to fix it (like we do now) rather than scramble to diagnose and fix them when all PRs suddenly start failing at once and we do not know why. At the same time, we automatically upgrade all our dependencies to the latest versions including all security fixes applied.

This is the exact case that the complexity of our CI build is justification of. You might think of it as an edge case, but actually it prevents from mysterious errors which are extremely difficult to diagnose (because your base image dependency version changed from 3.6.8 to 3.6.9). This happened already several times in the past that there was "some" change in the base image that caused our tests to fail (and our promises to be broken). I really hate the situation where "someone" makes a change that breaks "our" promises and we are not even aware of 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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #18037: Add Python2 to installed packages

Posted by GitBox <gi...@apache.org>.

potiuk commented on pull request #18037:
URL: https://github.com/apache/airflow/pull/18037#issuecomment-913148747


   fixes build errors in `main` : https://github.com/apache/airflow/runs/3515568433?check_suite_focus=true


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on pull request #18037: Add Python2 to installed packages

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on pull request #18037:
URL: https://github.com/apache/airflow/pull/18037#issuecomment-913152498

> we would have never known that python slim-buster suddenly without any warning dropped python2 from their image

This is to catch exactly those kind of changes where some external dependency break our tests (and likely breaks our promises). The whole "constraints" mechanism for CI is built around that to catch precisely this kind of error in a gentle way - so that when such builds fail in main, we have the time to fix it (like we do now) rather than scramble to diagnose and fix them when all PRs suddenly start failing at once and we do not know why. At the same time, we automatically (and continuously) upgrade all our dependencies to the latest versions including all security fixes applied.

--
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] uranusjr commented on pull request #18037: Add Python2 to installed packages

Posted by GitBox <gi...@apache.org>.

uranusjr commented on pull request #18037:
URL: https://github.com/apache/airflow/pull/18037#issuecomment-913149883


   Maybe we should also add a marker to skip Python 2 tests if `python2` is not available? THis can be dealed separately though.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk merged pull request #18037: Add Python2 to installed packages

Posted by GitBox <gi...@apache.org>.

potiuk merged pull request #18037:
URL: https://github.com/apache/airflow/pull/18037


   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org