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 2020/12/31 16:44:45 UTC
[GitHub] [airflow] potiuk opened a new pull request #13409: Removes provider-imposed requirements from setup.cfg
potiuk opened a new pull request #13409:
URL: https://github.com/apache/airflow/pull/13409
This change removes the provider-imposed requirements from the
airflow's setup.cfg to additional configuration in the
breeze/CI scripts. This does not change constraint apprach
when installing airflow, the constraints to those versions
remain as they were, but airflow package does not have to
have the limits in 'install_requires' section which makes
it much more "standalone.
We can add more requirements there as needed or remove
them when provider's dependencies change.
Also thanks to using --upgrade-to-newer-dependencies flag in
Breeze, the instructions on what to do when there is
a problem with conflicting dependencies are much simpler.
This is a final step of making airflow package fully
independent from the provider's dependencies.
<!--
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553056220
##########
File path: Dockerfile
##########
@@ -172,23 +169,42 @@ RUN if [[ -f /docker-context-files/.pypirc ]]; then \
cp /docker-context-files/.pypirc /root/.pypirc; \
fi
+COPY scripts/docker /scripts/docker
+# fix permission issue in Azure DevOps when running the scripts
+RUN chmod a+x /scripts/docker/*.sh
+
+ARG AIRFLOW_PIP_VERSION
+ENV AIRFLOW_PIP_VERSION=${AIRFLOW_PIP_VERSION}
+
+# Install Airflow ith "--user" flag, so that we can copy the whole .local folder to the final image
+# from the build image and always in non-editable mode
+ENV AIRFLOW_INSTALL_USER_FLAG="--user"
Review comment:
We don't allow overriding this Env Var, do we? If no, do we really need to have this as a separate env var?
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553056289
##########
File path: Dockerfile
##########
@@ -172,23 +169,42 @@ RUN if [[ -f /docker-context-files/.pypirc ]]; then \
cp /docker-context-files/.pypirc /root/.pypirc; \
fi
+COPY scripts/docker /scripts/docker
+# fix permission issue in Azure DevOps when running the scripts
+RUN chmod a+x /scripts/docker/*.sh
+
+ARG AIRFLOW_PIP_VERSION
+ENV AIRFLOW_PIP_VERSION=${AIRFLOW_PIP_VERSION}
+
+# Install Airflow ith "--user" flag, so that we can copy the whole .local folder to the final image
+# from the build image and always in non-editable mode
+ENV AIRFLOW_INSTALL_USER_FLAG="--user"
+ENV AIRFLOW_INSTALL_EDITABLE_FLAG=""
+
+# Upgrade to specific PIP version
RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
# By default we do not use pre-cached packages, but in CI/Breeze environment we override this to speed up
# builds in case setup.py/setup.cfg changed. This is pure optimisation of CI/Breeze builds.
ARG AIRFLOW_PRE_CACHED_PIP_PACKAGES="false"
ENV AIRFLOW_PRE_CACHED_PIP_PACKAGES=${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
+# By default we install providers from PyPI but in case of Breeze build we want to install providers
+# from local sources without the neeed of preparing provider packages upfront. This value is
Review comment:
```suggestion
# from local sources without the need of preparing provider packages upfront. This value is
```
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r551888965
##########
File path: Dockerfile
##########
@@ -179,16 +179,30 @@ RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
ARG AIRFLOW_PRE_CACHED_PIP_PACKAGES="false"
ENV AIRFLOW_PRE_CACHED_PIP_PACKAGES=${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
+# By default we install providers from PyPI but in case of Breze build we want to install providers
Review comment:
```suggestion
# By default we install providers from PyPI but in case of Breeze build we want to install providers
```
----------------------------------------------------------------
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] ashb commented on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756422715
> Hey @ashb - any news about the self-hosted runners modification? I think we are going to get more cross at each-other with each passing day. Is there anything we can help with it?
Nothing from GitHub - so we going to have to carry the patch ourselves for now :( I've started that process today to automate rebasing it (hence my questions in slack) and will work with Rust Lang team (who have similar needs) so we have two teams/projects to keep the patch updated and working
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753006091
cc: @mjpieters -> I think this is the ultimate solution to many of the problems we had with providers messing with the apache-airflow package.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-754639026
@kaxil @mik-laj -> fixed and also extracted the scripts and the dockerfiles look a lot cleaner.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753465399
[The Workflow run](https://github.com/apache/airflow/actions/runs/457608595) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553057515
##########
File path: IMAGES.rst
##########
@@ -473,6 +473,16 @@ The following build arguments (``--build-arg`` in docker build command) can be u
+------------------------------------------+------------------------------------------+------------------------------------------+
| ``AIRFLOW_EXTRAS`` | ``all`` | extras to install |
+------------------------------------------+------------------------------------------+------------------------------------------+
+| ``UPGRADE_TO_NEWER_DEPENDENCIES`` | ``false`` | If set to true, the dependencies are |
+| | | upgraded to newer versions matching |
+| | | setup.py before installation. |
++------------------------------------------+------------------------------------------+------------------------------------------+
+| ``CONTINUE_ON_PIP_CHECK_FAILURE`` | ``false`` | By default the image will fail if pip |
+| | | check fails for it. This is good for |
+| | | interactive building but on CI the |
+| | | image should be build regardless - we |
Review comment:
```suggestion
| | | image should be built regardless - we |
```
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r551851800
##########
File path: scripts/ci/libraries/_initialization.sh
##########
@@ -613,32 +616,50 @@ Verbosity variables:
VERBOSE: ${VERBOSE}
VERBOSE_COMMANDS: ${VERBOSE_COMMANDS}
-Image build variables:
+Common image build variables:
+ INSTALL_AIRFLOW_VERSION=${INSTALL_AIRFLOW_VERSION}
+ INSTALL_AIRFLOW_REFERENCE=${INSTALL_AIRFLOW_REFERENCE}
+ INSTALL_FROM_PYPI: ${INSTALL_FROM_PYPI}
+ AIRFLOW_PRE_CACHED_PIP_PACKAGES: ${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
UPGRADE_TO_NEWER_DEPENDENCIES: ${UPGRADE_TO_NEWER_DEPENDENCIES}
+ CONTINUE_ON_PIP_CHECK_FAILURE: ${CONTINUE_ON_PIP_CHECK_FAILURE}
CHECK_IMAGE_FOR_REBUILD: ${CHECK_IMAGE_FOR_REBUILD}
-
+ AIRFLOW_CONSTRAINTS_LOCATION: ${AIRFLOW_CONSTRAINTS_LOCATION}
+ AIRFLOW_CONSTRAINTS_REFERENCE: ${AIRFLOW_CONSTRAINTS_REFERENCE}
+ INSTALL_PROVIDERS_FROM_SOURCES: ${INSTALL_PROVIDERS_FROM_SOURCES}
+ INSTALL_FROM_DOCKER_CONTEXT_FILES: ${INSTALL_FROM_DOCKER_CONTEXT_FILES}
+ ADDITIONAL_AIRFLOW_EXTRAS: ${ADDITIONAL_AIRFLOW_EXTRAS}
+ ADDITIONAL_PYTHON_DEPS: ${ADDITIONAL_PYTHON_DEPS}
+ DEV_APT_COMMAND: ${DEV_APT_COMMAND}
+ ADDITIONAL_DEV_APT_COMMAND: ${ADDITIONAL_DEV_APT_COMMAND}
+ DEV_APT_DEPS: ${DEV_APT_DEPS}
+ ADDITIONAL_DEV_APT_DEPS: ${ADDITIONAL_DEV_APT_DEPS}
+ RUNTIME_APT_COMMAND: ${RUNTIME_APT_COMMAND}
+ ADDITIONAL_RUNTIME_APT_COMMAND: ${ADDITIONAL_RUNTIME_APT_COMMAND}
+ RUNTIME_APT_DEPS: ${RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_DEPS: ${ADDITIONAL_RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_ENV: ${ADDITIONAL_RUNTIME_APT_ENV}
+
+Production image build variables:
+
+ AIRFLOW_INSTALLATION_METHOD: ${AIRFLOW_INSTALLATION_METHOD}
+ AIRFLOW_INSTALL_VERSION: ${AIRFLOW_INSTALL_VERSION}
+ AIRFLOW_SOURCES_FROM: ${AIRFLOW_SOURCES_FROM}
+ AIRFLOW_SOURCES_TO: ${AIRFLOW_SOURCES_TO}
Detected GitHub environment:
USE_GITHUB_REGISTRY=${USE_GITHUB_REGISTRY}
GITHUB_REGISTRY=${GITHUB_REGISTRY}
- GITHUB_REPOSITORY=${GITHUB_REPOSITORY}
+ GITHUB_REPOSITORY=${GITHUB_REPOSITORY}SETUP.
Review comment:
?
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756421082
Hey @ashb - any news about the self-hosted runners modification? I think we are going to get more cross at each-other with each passing day. Is there anything we can help with 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] potiuk commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552629539
##########
File path: scripts/docker/compile_www_assets.sh
##########
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# shellcheck disable=SC2086
+
Review comment:
:heart: good idea. We have no common inits here where we do such checks. so adding it explictely here is good sanity check
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550640644
##########
File path: Dockerfile
##########
@@ -245,6 +245,10 @@ ENV INSTALL_PROVIDERS_FROM_SOURCES=${INSTALL_PROVIDERS_FROM_SOURCES}
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
ENV UPGRADE_TO_NEWER_DEPENDENCIES=${UPGRADE_TO_NEWER_DEPENDENCIES}
+# Those are additional constraints that are needed for some extras but we do not want to
Review comment:
We need to embed those and use pre-commit because we want to handle the situation when somebody updates it in PR . IN this case the scripts from CI will not be used (only master version) so we need to make sure that Dockerfile's
default value is the same as the ARG passed in this case.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553057824
##########
File path: docs/apache-airflow/production-deployment.rst
##########
@@ -568,6 +568,12 @@ The following build arguments (``--build-arg`` in docker build command) can be u
| | | upgraded to newer versions matching |
| | | setup.py before installation. |
+------------------------------------------+------------------------------------------+------------------------------------------+
+| ``CONTINUE_ON_PIP_CHECK_FAILURE`` | ``false`` | By default the image build fails if pip |
+| | | check fails for it. This is good for |
+| | | interactive building but on CI the |
+| | | image should be build regardless - we |
Review comment:
```suggestion
| | | image should be built regardless - we |
```
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756500400
[The Workflow run](https://github.com/apache/airflow/actions/runs/470474102) is cancelling this PR. Building images for the PR has failed. Follow the 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] potiuk merged pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #13409:
URL: https://github.com/apache/airflow/pull/13409
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552719071
##########
File path: scripts/ci/libraries/_initialization.sh
##########
@@ -613,32 +616,50 @@ Verbosity variables:
VERBOSE: ${VERBOSE}
VERBOSE_COMMANDS: ${VERBOSE_COMMANDS}
-Image build variables:
+Common image build variables:
+ INSTALL_AIRFLOW_VERSION=${INSTALL_AIRFLOW_VERSION}
+ INSTALL_AIRFLOW_REFERENCE=${INSTALL_AIRFLOW_REFERENCE}
+ INSTALL_FROM_PYPI: ${INSTALL_FROM_PYPI}
+ AIRFLOW_PRE_CACHED_PIP_PACKAGES: ${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
UPGRADE_TO_NEWER_DEPENDENCIES: ${UPGRADE_TO_NEWER_DEPENDENCIES}
+ CONTINUE_ON_PIP_CHECK_FAILURE: ${CONTINUE_ON_PIP_CHECK_FAILURE}
CHECK_IMAGE_FOR_REBUILD: ${CHECK_IMAGE_FOR_REBUILD}
-
+ AIRFLOW_CONSTRAINTS_LOCATION: ${AIRFLOW_CONSTRAINTS_LOCATION}
+ AIRFLOW_CONSTRAINTS_REFERENCE: ${AIRFLOW_CONSTRAINTS_REFERENCE}
+ INSTALL_PROVIDERS_FROM_SOURCES: ${INSTALL_PROVIDERS_FROM_SOURCES}
+ INSTALL_FROM_DOCKER_CONTEXT_FILES: ${INSTALL_FROM_DOCKER_CONTEXT_FILES}
+ ADDITIONAL_AIRFLOW_EXTRAS: ${ADDITIONAL_AIRFLOW_EXTRAS}
+ ADDITIONAL_PYTHON_DEPS: ${ADDITIONAL_PYTHON_DEPS}
+ DEV_APT_COMMAND: ${DEV_APT_COMMAND}
+ ADDITIONAL_DEV_APT_COMMAND: ${ADDITIONAL_DEV_APT_COMMAND}
+ DEV_APT_DEPS: ${DEV_APT_DEPS}
+ ADDITIONAL_DEV_APT_DEPS: ${ADDITIONAL_DEV_APT_DEPS}
+ RUNTIME_APT_COMMAND: ${RUNTIME_APT_COMMAND}
+ ADDITIONAL_RUNTIME_APT_COMMAND: ${ADDITIONAL_RUNTIME_APT_COMMAND}
+ RUNTIME_APT_DEPS: ${RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_DEPS: ${ADDITIONAL_RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_ENV: ${ADDITIONAL_RUNTIME_APT_ENV}
+
+Production image build variables:
+
+ AIRFLOW_INSTALLATION_METHOD: ${AIRFLOW_INSTALLATION_METHOD}
+ AIRFLOW_INSTALL_VERSION: ${AIRFLOW_INSTALL_VERSION}
+ AIRFLOW_SOURCES_FROM: ${AIRFLOW_SOURCES_FROM}
+ AIRFLOW_SOURCES_TO: ${AIRFLOW_SOURCES_TO}
Detected GitHub environment:
USE_GITHUB_REGISTRY=${USE_GITHUB_REGISTRY}
GITHUB_REGISTRY=${GITHUB_REGISTRY}
- GITHUB_REPOSITORY=${GITHUB_REPOSITORY}
+ GITHUB_REPOSITORY=${GITHUB_REPOSITORY}.
Review comment:
?
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553058813
##########
File path: scripts/docker/install_additional_dependencies.sh
##########
@@ -0,0 +1,38 @@
+#!/usr/bin/env bash
Review comment:
Where is this file used?
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r551898148
##########
File path: scripts/ci/libraries/_initialization.sh
##########
@@ -613,32 +616,50 @@ Verbosity variables:
VERBOSE: ${VERBOSE}
VERBOSE_COMMANDS: ${VERBOSE_COMMANDS}
-Image build variables:
+Common image build variables:
+ INSTALL_AIRFLOW_VERSION=${INSTALL_AIRFLOW_VERSION}
+ INSTALL_AIRFLOW_REFERENCE=${INSTALL_AIRFLOW_REFERENCE}
+ INSTALL_FROM_PYPI: ${INSTALL_FROM_PYPI}
+ AIRFLOW_PRE_CACHED_PIP_PACKAGES: ${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
UPGRADE_TO_NEWER_DEPENDENCIES: ${UPGRADE_TO_NEWER_DEPENDENCIES}
+ CONTINUE_ON_PIP_CHECK_FAILURE: ${CONTINUE_ON_PIP_CHECK_FAILURE}
CHECK_IMAGE_FOR_REBUILD: ${CHECK_IMAGE_FOR_REBUILD}
-
+ AIRFLOW_CONSTRAINTS_LOCATION: ${AIRFLOW_CONSTRAINTS_LOCATION}
+ AIRFLOW_CONSTRAINTS_REFERENCE: ${AIRFLOW_CONSTRAINTS_REFERENCE}
+ INSTALL_PROVIDERS_FROM_SOURCES: ${INSTALL_PROVIDERS_FROM_SOURCES}
+ INSTALL_FROM_DOCKER_CONTEXT_FILES: ${INSTALL_FROM_DOCKER_CONTEXT_FILES}
+ ADDITIONAL_AIRFLOW_EXTRAS: ${ADDITIONAL_AIRFLOW_EXTRAS}
+ ADDITIONAL_PYTHON_DEPS: ${ADDITIONAL_PYTHON_DEPS}
+ DEV_APT_COMMAND: ${DEV_APT_COMMAND}
+ ADDITIONAL_DEV_APT_COMMAND: ${ADDITIONAL_DEV_APT_COMMAND}
+ DEV_APT_DEPS: ${DEV_APT_DEPS}
+ ADDITIONAL_DEV_APT_DEPS: ${ADDITIONAL_DEV_APT_DEPS}
+ RUNTIME_APT_COMMAND: ${RUNTIME_APT_COMMAND}
+ ADDITIONAL_RUNTIME_APT_COMMAND: ${ADDITIONAL_RUNTIME_APT_COMMAND}
+ RUNTIME_APT_DEPS: ${RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_DEPS: ${ADDITIONAL_RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_ENV: ${ADDITIONAL_RUNTIME_APT_ENV}
+
+Production image build variables:
+
+ AIRFLOW_INSTALLATION_METHOD: ${AIRFLOW_INSTALLATION_METHOD}
+ AIRFLOW_INSTALL_VERSION: ${AIRFLOW_INSTALL_VERSION}
+ AIRFLOW_SOURCES_FROM: ${AIRFLOW_SOURCES_FROM}
+ AIRFLOW_SOURCES_TO: ${AIRFLOW_SOURCES_TO}
Detected GitHub environment:
USE_GITHUB_REGISTRY=${USE_GITHUB_REGISTRY}
GITHUB_REGISTRY=${GITHUB_REGISTRY}
- GITHUB_REPOSITORY=${GITHUB_REPOSITORY}
+ GITHUB_REPOSITORY=${GITHUB_REPOSITORY}SETUP.
Review comment:
copy&Paste
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-754604076
I am also extracting the installation bash commands to separate "install*.sh" files. this will make a lot of code common between Dockerfile and Dockerfile.ci, also it makes it more obvious (and commented) why we are running certain steps in the Dockerfile.
Stay tuned, fixup is coming.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753285136
Actually, this one is even better now - we have a separate set of those "provider-imposed" requirements in Dockerfile.ci and in Dockerfile. This means that even our production docker imaage will not have limitations (like `requests`) imposed by Snowflke for example. It will only require limitations from the providers that are pre-installed (currently there is only the urrlib3 library from boto3/amazon)
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552411091
##########
File path: scripts/docker/compile_www_assets.sh
##########
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# shellcheck disable=SC2086
+
Review comment:
```suggestion
test -v PYTHON_MAJOR_MINOR_VERSION
```
Extra sanity check to spot problems faster.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550769584
##########
File path: Dockerfile
##########
@@ -255,13 +265,16 @@ RUN if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \
if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \
if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \
pip install --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \
+ ${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS} \
--upgrade --upgrade-strategy eager; \
pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
+ pip check || ${CONTINUE_ON_PIP_CHECK_FAILURE}; \
else \
pip install --upgrade --upgrade-strategy only-if-needed \
--user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \
--constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
+ pip check || ${CONTINUE_ON_PIP_CHECK_FAILURE}; \
Review comment:
We fail by default when we build the image locally but we can also disable it and continue building the image if we want to inspect 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] potiuk commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552633523
##########
File path: scripts/docker/install_from_docker_context_files.sh
##########
@@ -0,0 +1,86 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# shellcheck disable=SC2086
+
+# Installs airflow and provider packages from locally present docker context files
+# This is used in CI to install airflow and provider packages in the CI system of ours
+# The packages are prepared from current sources and placed in the 'docker-context-files folder
+# Then both airflow and provider packages are installed using those packages rather than
+# PyPI
Review comment:
Yep. I thoutght about doing it differently, but this is the easiest and more straightforward way in docker file. We could pass them by parameters, but they are not named, so that it is even worse.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753287248
[The Workflow run](https://github.com/apache/airflow/actions/runs/455853669) is cancelling this PR. Building images for the PR has failed. Follow the 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] mik-laj commented on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756364484
It would be great if we could merge these changes, because I have a few changes that are blocked by this.
https://github.com/apache/airflow/pull/13534
https://github.com/apache/airflow/pull/13505
https://github.com/apache/airflow/pull/13347
https://github.com/apache/airflow/pull/13256
https://github.com/apache/airflow/pull/13176
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553055851
##########
File path: Dockerfile
##########
@@ -172,23 +169,42 @@ RUN if [[ -f /docker-context-files/.pypirc ]]; then \
cp /docker-context-files/.pypirc /root/.pypirc; \
fi
+COPY scripts/docker /scripts/docker
+# fix permission issue in Azure DevOps when running the scripts
+RUN chmod a+x /scripts/docker/*.sh
+
+ARG AIRFLOW_PIP_VERSION
+ENV AIRFLOW_PIP_VERSION=${AIRFLOW_PIP_VERSION}
+
+# Install Airflow ith "--user" flag, so that we can copy the whole .local folder to the final image
Review comment:
```suggestion
# Install Airflow with "--user" flag, so that we can copy the whole .local folder to the final image
```
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550770147
##########
File path: setup.py
##########
@@ -779,26 +779,6 @@ def get_provider_package_from_package_id(package_id: str):
return f"apache-airflow-providers-{package_suffix}"
-class AirflowDistribution(Distribution):
Review comment:
We do not need it any more, because with recent setup.py refactor the preinstalled providers were moved back to setup.py.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552412530
##########
File path: scripts/docker/install_airflow_from_latest_master.sh
##########
@@ -0,0 +1,43 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# shellcheck disable=SC2086
+
+# Installs Airflow from latest master. This is pure optimisation. It is done because we do not want
+# to reinstall all dependencies from scratch when setup.py changes. Problem with Docker caching is that
+# when a file is changed, when added to docker context, it invalidates the cache and it causes Docker
+# build to reinstall all dependencies from scratch. This can take a loooooot of time. Therefore we install
+# the dependencies first from master (and uninstall airflow right after) so that we can start installing
+# deps from those pre-installed dependencies. It saves few minutes of build time when setup.py changes.
+#
+# If INSTALL_MYSQL_CLIENT is set to false, mysql extra is removed
+#
+function install_airflow_from_latest_master() {
Review comment:
```suggestion
test -v INSTALL_MYSQL_CLIENT
test -v AIRFLOW_INSTALL_USER_FLAG
test -v AIRFLOW_REPO
test -v AIRFLOW_BRANCH
test -v AIRFLOW_CONSTRAINTS_LOCATION
test -v AIRFLOW_PIP_VERSION
function install_airflow_from_latest_master() {
```
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753460582
[The Workflow run](https://github.com/apache/airflow/actions/runs/457552765) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550729299
##########
File path: scripts/ci/pre_commit/pre_commit_check_eager_constraints.sh
##########
@@ -0,0 +1,60 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+set -euo pipefail
+# shellcheck source=scripts/ci/libraries/_script_init.sh
+. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+
+
+set_eager_upgrade_additional_requirements_variable
+
+PROD_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile | cut -d'"' -f 2)
+CI_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile.ci | cut -d'"' -f 2)
+
+error="false"
+if [[ ${PROD_DOCKER_FILE_EAGER_CONSTRAINTS} != "${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}" ]]; then
+ echo
+ echo """
Review comment:
```suggestion
echo -e """
```
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r551892386
##########
File path: setup.cfg
##########
@@ -86,6 +86,7 @@ install_requires =
# cattrs >= 1.1.0 dropped support for Python 3.6
cattrs>=1.0, <1.1.0;python_version<="3.6"
cattrs~=1.1;python_version>"3.6"
+ chardet<4.0.0
Review comment:
I suggest removing it from here if not required by core even if needed by several providers
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550729182
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -955,50 +979,33 @@ It can mean one of those:
2) You changed some dependencies in setup.py or setup.cfg and they are conflicting.
+
In case 1) - apologies for the trouble.Please let committers know and they will fix it. You might
be asked to rebase to the latest master after the problem is fixed.
In case 2) - Follow the steps below:
-* consult the committers if you are unsure what to do. Just comment in the PR that you need help, if you do,
- but try to follow those instructions first!
-
-* ask the committer to set 'upgrade to newer dependencies'. All dependencies in your PR will be updated
- to latest 'good' versions and you will be able to check if they are not conflicting.
-
-* run locally the image that is failing with Breeze:
-
- ./breeze ${1}--github-image-id ${GITHUB_REGISTRY_PULL_IMAGE_TAG} --backend ${BACKEND="sqlite"} --python ${PYTHON_MAJOR_MINOR_VERSION}
-
-* your setup.py and setup.cfg will be mounted to the container. You will be able to iterate with
- different setup.py versions.
-
-* in container your can run 'pipdeptree' to figure out where the dependency conflict comes from.
-
-* Some useful commands that can help yoy to find out dependencies you have:
-
- * 'pipdeptree | less' (you can then search through the dependencies with vim-like shortcuts)
-
- * 'pipdeptree > /files/pipdeptree.txt' - this will produce a pipdeptree.txt file in your source
- 'files' directory and you can open it in editor of your choice,
-
- * 'pipdeptree | grep YOUR_DEPENDENCY' - to see all the requirements your dependency has as specified
- by other packages
-
-* figure out which dependency limits should be upgraded. Upgrade them in corresponding setup.py extras
- and run pip to upgrade your dependencies accordingly:
-
- pip install '.[all]' --upgrade --upgrade-strategy eager
+* try to build CI and then PROD image locally with breeze, adding --upgrade-to-newer-dependencies flag:
-* run pip check to figure out if the dependencies have been fixed. It should let you know which dependencies
- are conflicting or (hurray!) if there are no conflicts:
+${COLOR_BLUE}
+ for python_version in ${CURRENT_PYTHON_MAJOR_MINOR_VERSIONS[*]}
+ do
+ ./breeze build-image --upgrade-to-newer-dependencies --python "$\{python_version\}"
Review comment:
nit: As a user, I don't like pasting a lot of line bash scripts into the terminalI guess we can just display the 4 commands that the user needs to run.
```
./breeze build-image --upgrade-to-newer-dependencies --python 3.7
./breeze build-image --upgrade-to-newer-dependencies --python 3.7
./breeze build-image --upgrade-to-newer-dependencies --python 3.8
./breeze build-image --upgrade-to-newer-dependencies --python 3.9
```
Or alternatively, as a one-line script.
```
echo ${CURRENT_PYTHON_MAJOR_MINOR_VERSIONS[*]} | xargs -n 1 ./breeze build-image --upgrade-to-newer-dependencies --python
```
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753004254
I believe I managed to improve the "eager constraint update" mechanism in the way that it removes all provider-imposed requirements out of the setup.cfg / install_requires (!). It works exactly as before when it comes to constraints installation and automated constraint upgrade, but this time those requirements are not present in the install_requires section of the `apache-airflow` package.
I simply add the imposed requirements in the images build steps where eager upgrade strategy is used. This seems to keep the same properties as previous solution where those limits were added to "install-requires" but those limits are only used during the image build and airflow package does not have those.
Also with --upgrade-to-newer-dependencies switch of breeze you can run the very same "eager upgrade" locally and it makes it much easier to locally test new constraints without the need to push them as PR. It now becomes rather straightforward how to fix any future similar conflict.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753004570
CC: @r-richmond --> I think this is much better solution of the problem you want to solve in #13333
Happy New Year!
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550739502
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -955,50 +979,33 @@ It can mean one of those:
2) You changed some dependencies in setup.py or setup.cfg and they are conflicting.
+
In case 1) - apologies for the trouble.Please let committers know and they will fix it. You might
be asked to rebase to the latest master after the problem is fixed.
In case 2) - Follow the steps below:
-* consult the committers if you are unsure what to do. Just comment in the PR that you need help, if you do,
- but try to follow those instructions first!
-
-* ask the committer to set 'upgrade to newer dependencies'. All dependencies in your PR will be updated
- to latest 'good' versions and you will be able to check if they are not conflicting.
-
-* run locally the image that is failing with Breeze:
-
- ./breeze ${1}--github-image-id ${GITHUB_REGISTRY_PULL_IMAGE_TAG} --backend ${BACKEND="sqlite"} --python ${PYTHON_MAJOR_MINOR_VERSION}
-
-* your setup.py and setup.cfg will be mounted to the container. You will be able to iterate with
- different setup.py versions.
-
-* in container your can run 'pipdeptree' to figure out where the dependency conflict comes from.
-
-* Some useful commands that can help yoy to find out dependencies you have:
-
- * 'pipdeptree | less' (you can then search through the dependencies with vim-like shortcuts)
-
- * 'pipdeptree > /files/pipdeptree.txt' - this will produce a pipdeptree.txt file in your source
- 'files' directory and you can open it in editor of your choice,
-
- * 'pipdeptree | grep YOUR_DEPENDENCY' - to see all the requirements your dependency has as specified
- by other packages
-
-* figure out which dependency limits should be upgraded. Upgrade them in corresponding setup.py extras
- and run pip to upgrade your dependencies accordingly:
-
- pip install '.[all]' --upgrade --upgrade-strategy eager
+* try to build CI and then PROD image locally with breeze, adding --upgrade-to-newer-dependencies flag:
-* run pip check to figure out if the dependencies have been fixed. It should let you know which dependencies
- are conflicting or (hurray!) if there are no conflicts:
+${COLOR_BLUE}
+ for python_version in ${CURRENT_PYTHON_MAJOR_MINOR_VERSIONS[*]}
+ do
+ ./breeze build-image --upgrade-to-newer-dependencies --python "$\{python_version\}"
Review comment:
I prefer plain for loop. It is much better. Especially that when printed the variable will be interpolated and it will be:
```
for python_version in 3.6 3.7 3.8
do
./breeze build-image --upgrade-to-newer-dependencies --python "$\{python_version\}"
done
```
This is much more straightforward than one-liner you proposed.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550871832
##########
File path: Dockerfile
##########
@@ -179,16 +179,25 @@ RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
ARG AIRFLOW_PRE_CACHED_PIP_PACKAGES="false"
ENV AIRFLOW_PRE_CACHED_PIP_PACKAGES=${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
+# By default we install providers from PyPI but in case of Breze build we want to install providers
+# from local sources without the neeed of preparing provider packages upfront. This value is
+# automatically overridden by Breeze scripts.
+ARG INSTALL_PROVIDERS_FROM_SOURCES="false"
+ENV INSTALL_PROVIDERS_FROM_SOURCES=${INSTALL_PROVIDERS_FROM_SOURCES}
+
# In case of Production build image segment we want to pre-install master version of airflow
# dependencies from GitHub so that we do not have to always reinstall it from the scratch.
+# The Airflow (and providers in case INSTALL_PROVIDERS_FROM_SOURCES is "false")
+# are uninstalled, only depeendencies remain
RUN if [[ ${AIRFLOW_PRE_CACHED_PIP_PACKAGES} == "true" ]]; then \
if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \
AIRFLOW_EXTRAS=${AIRFLOW_EXTRAS/mysql,}; \
fi; \
pip install --user \
"https://github.com/${AIRFLOW_REPO}/archive/${AIRFLOW_BRANCH}.tar.gz#egg=apache-airflow[${AIRFLOW_EXTRAS}]" \
- --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}" \
- && pip uninstall --yes apache-airflow; \
+ --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
+ pip freeze | grep apache-airflow-providers | grep -v '@' | xargs pip uninstall --yes || true ; \
Review comment:
Nope. We were excluding the "@" lines (grep -v ""@") because pip uninstall did not work with those ('apache-airflow@URL).
* `pip uninstall apache-airflow-providers-google==1.0.0` works
* `pip uninstall apache-airflow@http://.....` does not work
However we do not need this grep any-more because I added full 'apache-airlfow-providers grep to remove providers separately and apache-airflow separately in the next line, so the whole grep -v "@" can be removed and we do not need to do any cut .
Removed 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] github-actions[bot] commented on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753323983
[The Workflow run](https://github.com/apache/airflow/actions/runs/456236462) is cancelling this PR. Building images for the PR has failed. Follow the 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] potiuk commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553198440
##########
File path: Dockerfile.ci
##########
@@ -314,56 +327,34 @@ COPY airflow/__init__.py ${AIRFLOW_SOURCES}/airflow/__init__.py
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
ENV UPGRADE_TO_NEWER_DEPENDENCIES=${UPGRADE_TO_NEWER_DEPENDENCIES}
+# Those are additional constraints that are needed for some extras but we do not want to
+# Force them on the main Airflow package. Those limitations are:
+# * chardet,<4: required by snowflake provider https://github.com/snowflakedb/snowflake-connector-python/blob/v2.3.6/setup.py#L201
+# * lazy-object-proxy<1.5.0: required by astroid
+# * pyOpenSSL: Imposed by snowflake provider https://github.com/snowflakedb/snowflake-connector-python/blob/v2.3.6/setup.py#L201
+# * requests>=2.20.0,<2.24.0: required by snowflake provider https://github.com/snowflakedb/snowflake-connector-python/blob/v2.3.6/setup.py#L201
+# * urllib3<1.26: Required to keep boto3 and snowflake happy
+
+ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS="chardet<4 lazy-object-proxy<1.5.0 pyOpenSSL<20.0.0 requests>=2.20.0,<2.24.0 urllib3<1.26"
+
+ARG CONTINUE_ON_PIP_CHECK_FAILURE="false"
+
# The goal of this line is to install the dependencies from the most current setup.py from sources
# This will be usually incremental small set of packages in CI optimized build, so it will be very fast
# In non-CI optimized build this will install all dependencies before installing sources.
# Usually we will install versions based on the dependencies in setup.py and upgraded only if needed.
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \
- if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \
- pip install -e ".[${AIRFLOW_EXTRAS}]" --upgrade --upgrade-strategy eager; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- else \
- pip install -e ".[${AIRFLOW_EXTRAS}]" --upgrade --upgrade-strategy only-if-needed\
- --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- fi; \
+ /scripts/docker/install_airflow.sh; \
fi
# If wheel files are found in /docker-context-files during installation
# they are also installed additionally to whatever is installed from Airflow.
COPY docker-context-files/ /docker-context-files/
-# hadolint ignore=SC2086, SC2010
RUN if [[ ${INSTALL_FROM_DOCKER_CONTEXT_FILES} == "true" ]]; then \
- reinstalling_apache_airflow_package=$(ls /docker-context-files/apache?airflow?[0-9]*.{whl,tar.gz} 2>/dev/null || true); \
- if [[ "${reinstalling_apache_airflow_package}" != "" ]]; then \
- # install airflow with extras \
- reinstalling_apache_airflow_package="${reinstalling_apache_airflow_package}[${AIRFLOW_EXTRAS}]"; \
- fi; \
- reinstalling_apache_airflow_providers_packages=$(ls /docker-context-files/apache?airflow?providers*.{whl,tar.gz} 2>/dev/null || true); \
- if [[ ${reinstalling_apache_airflow_package} != "" || \
- ${reinstalling_apache_airflow_providers_packages} == "" ]]; then \
- if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \
- pip install --force-reinstall --upgrade --upgrade-strategy eager \
- --user ${reinstalling_apache_airflow_package} \
- ${reinstalling_apache_airflow_providers_packages}; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- else \
- pip install --force-reinstall --upgrade --upgrade-strategy only-if-needed \
- --user ${reinstalling_apache_airflow_package} \
- ${reinstalling_apache_airflow_providers_packages} \
- --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- fi; \
- fi ; \
- # All the other packages we want to reinstall as-is, without dependencies \
- reinstalling_other_packages=$(ls /docker-context-files/*.{whl,tar.gz} 2>/dev/null | \
- grep -v apache_airflow | grep -v apache-airflow || true); \
- if [[ "${reinstalling_other_packages}" != "" ]]; then \
- pip install --force-reinstall --user --no-deps ${reinstalling_other_packages}; \
- fi; \
+ /scripts/docker/install_from_docker_contest_files.sh; \
Review comment:
Good point. Fixed
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753319325
Added some explanation and I think this one is also very close to be ready for review and 'green' .
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756430920
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] github-actions[bot] commented on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753473748
[The Workflow run](https://github.com/apache/airflow/actions/runs/457694858) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552413610
##########
File path: setup.cfg
##########
@@ -86,6 +86,7 @@ install_requires =
# cattrs >= 1.1.0 dropped support for Python 3.6
cattrs>=1.0, <1.1.0;python_version<="3.6"
cattrs~=1.1;python_version>"3.6"
+ chardet<4.0.0
Review comment:
This is another problem with snowflake, if I remember correctly. We cannot update requests to the latest version, only the latest version of snowflake supports the latest version of cherdat.
----------------------------------------------------------------
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] ashb edited a comment on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756422715
> Hey @ashb - any news about the self-hosted runners modification? I think we are going to get more cross at each-other with each passing day. Is there anything we can help with it?
Nothing from GitHub - so we going to have to carry the patch ourselves for now :( I've started that process today to automate rebasing it (hence my questions in slack) and will work with Rust Lang team (who have similar needs) so we have two teams/projects to keep the patch updated and working.
I'll let you know if there's anything you can do to help - I'm thinking I'll set up some runners manually and Mike can backfill the automation
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552411658
##########
File path: scripts/docker/install_additional_dependencies.sh
##########
@@ -0,0 +1,38 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# shellcheck disable=SC2086
+
Review comment:
```suggestion
test -v UPGRADE_TO_NEWER_DEPENDENCIES
test -v ADDITIONAL_PYTHON_DEPS
test -v EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS
test -v AIRFLOW_INSTALL_USER_FLAG
test -v AIRFLOW_PIP_VERSION
test -v CONTINUE_ON_PIP_CHECK_FAILURE
```
Extra sanity check.
----------------------------------------------------------------
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 merged pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #13409:
URL: https://github.com/apache/airflow/pull/13409
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753462160
Ok. I hope this one will be good to go and we will have much less limited Airflow 2.0.1
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756271741
[The Workflow run](https://github.com/apache/airflow/actions/runs/469458321) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553196208
##########
File path: Dockerfile.ci
##########
@@ -314,56 +327,34 @@ COPY airflow/__init__.py ${AIRFLOW_SOURCES}/airflow/__init__.py
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
ENV UPGRADE_TO_NEWER_DEPENDENCIES=${UPGRADE_TO_NEWER_DEPENDENCIES}
+# Those are additional constraints that are needed for some extras but we do not want to
+# Force them on the main Airflow package. Those limitations are:
+# * chardet,<4: required by snowflake provider https://github.com/snowflakedb/snowflake-connector-python/blob/v2.3.6/setup.py#L201
+# * lazy-object-proxy<1.5.0: required by astroid
+# * pyOpenSSL: Imposed by snowflake provider https://github.com/snowflakedb/snowflake-connector-python/blob/v2.3.6/setup.py#L201
+# * requests>=2.20.0,<2.24.0: required by snowflake provider https://github.com/snowflakedb/snowflake-connector-python/blob/v2.3.6/setup.py#L201
+# * urllib3<1.26: Required to keep boto3 and snowflake happy
+
+ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS="chardet<4 lazy-object-proxy<1.5.0 pyOpenSSL<20.0.0 requests>=2.20.0,<2.24.0 urllib3<1.26"
+
+ARG CONTINUE_ON_PIP_CHECK_FAILURE="false"
+
# The goal of this line is to install the dependencies from the most current setup.py from sources
# This will be usually incremental small set of packages in CI optimized build, so it will be very fast
# In non-CI optimized build this will install all dependencies before installing sources.
# Usually we will install versions based on the dependencies in setup.py and upgraded only if needed.
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \
- if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \
- pip install -e ".[${AIRFLOW_EXTRAS}]" --upgrade --upgrade-strategy eager; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- else \
- pip install -e ".[${AIRFLOW_EXTRAS}]" --upgrade --upgrade-strategy only-if-needed\
- --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- fi; \
+ /scripts/docker/install_airflow.sh; \
fi
# If wheel files are found in /docker-context-files during installation
# they are also installed additionally to whatever is installed from Airflow.
COPY docker-context-files/ /docker-context-files/
-# hadolint ignore=SC2086, SC2010
RUN if [[ ${INSTALL_FROM_DOCKER_CONTEXT_FILES} == "true" ]]; then \
- reinstalling_apache_airflow_package=$(ls /docker-context-files/apache?airflow?[0-9]*.{whl,tar.gz} 2>/dev/null || true); \
- if [[ "${reinstalling_apache_airflow_package}" != "" ]]; then \
- # install airflow with extras \
- reinstalling_apache_airflow_package="${reinstalling_apache_airflow_package}[${AIRFLOW_EXTRAS}]"; \
- fi; \
- reinstalling_apache_airflow_providers_packages=$(ls /docker-context-files/apache?airflow?providers*.{whl,tar.gz} 2>/dev/null || true); \
- if [[ ${reinstalling_apache_airflow_package} != "" || \
- ${reinstalling_apache_airflow_providers_packages} == "" ]]; then \
- if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \
- pip install --force-reinstall --upgrade --upgrade-strategy eager \
- --user ${reinstalling_apache_airflow_package} \
- ${reinstalling_apache_airflow_providers_packages}; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- else \
- pip install --force-reinstall --upgrade --upgrade-strategy only-if-needed \
- --user ${reinstalling_apache_airflow_package} \
- ${reinstalling_apache_airflow_providers_packages} \
- --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
- pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
- fi; \
- fi ; \
- # All the other packages we want to reinstall as-is, without dependencies \
- reinstalling_other_packages=$(ls /docker-context-files/*.{whl,tar.gz} 2>/dev/null | \
- grep -v apache_airflow | grep -v apache-airflow || true); \
- if [[ "${reinstalling_other_packages}" != "" ]]; then \
- pip install --force-reinstall --user --no-deps ${reinstalling_other_packages}; \
- fi; \
+ /scripts/docker/install_from_docker_contest_files.sh; \
Review comment:
```suggestion
/scripts/docker/install_from_docker_context_files.sh; \
```
?
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550828579
##########
File path: Dockerfile
##########
@@ -179,16 +179,25 @@ RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
ARG AIRFLOW_PRE_CACHED_PIP_PACKAGES="false"
ENV AIRFLOW_PRE_CACHED_PIP_PACKAGES=${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
+# By default we install providers from PyPI but in case of Breze build we want to install providers
+# from local sources without the neeed of preparing provider packages upfront. This value is
+# automatically overridden by Breeze scripts.
+ARG INSTALL_PROVIDERS_FROM_SOURCES="false"
+ENV INSTALL_PROVIDERS_FROM_SOURCES=${INSTALL_PROVIDERS_FROM_SOURCES}
+
# In case of Production build image segment we want to pre-install master version of airflow
# dependencies from GitHub so that we do not have to always reinstall it from the scratch.
+# The Airflow (and providers in case INSTALL_PROVIDERS_FROM_SOURCES is "false")
+# are uninstalled, only depeendencies remain
RUN if [[ ${AIRFLOW_PRE_CACHED_PIP_PACKAGES} == "true" ]]; then \
if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \
AIRFLOW_EXTRAS=${AIRFLOW_EXTRAS/mysql,}; \
fi; \
pip install --user \
"https://github.com/${AIRFLOW_REPO}/archive/${AIRFLOW_BRANCH}.tar.gz#egg=apache-airflow[${AIRFLOW_EXTRAS}]" \
- --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}" \
- && pip uninstall --yes apache-airflow; \
+ --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
+ pip freeze | grep apache-airflow-providers | grep -v '@' | xargs pip uninstall --yes || true ; \
Review comment:
```suggestion
pip freeze | grep apache-airflow-providers | grep -v '@' | cut -d "@" -f 1 | xargs pip uninstall --yes || true ; \
```
Don't we just need to get the package name 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] potiuk commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550640644
##########
File path: Dockerfile
##########
@@ -245,6 +245,10 @@ ENV INSTALL_PROVIDERS_FROM_SOURCES=${INSTALL_PROVIDERS_FROM_SOURCES}
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
ENV UPGRADE_TO_NEWER_DEPENDENCIES=${UPGRADE_TO_NEWER_DEPENDENCIES}
+# Those are additional constraints that are needed for some extras but we do not want to
Review comment:
We need to embed those and use pre-commit because we want to handle the situation when somebody updates it in PR . IN this case the scripts from CI will not be used (only master version) so we need to make sure that Dockerfile's
default value is the same as the ARG passed in this case.
The nice thing is that you can have different requirements for PROD image (less providers that are messing with the dependencies) and different for CI image (there we have all providers).
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550769966
##########
File path: setup.cfg
##########
@@ -113,17 +113,14 @@ install_requires =
pendulum~=2.0
pep562~=1.0;python_version<"3.7"
psutil>=4.2.0, <6.0.0
- # snowflake-connector-python don't support newer version
- # https://github.com/snowflakedb/snowflake-connector-python/blob/v2.3.6/setup.py#L201
- pyOpenSSL<20.0.0
pygments>=2.0.1, <3.0
- # snowflake-connector-python, flask-jwt-extended, msal are not compatible with newest version.
+ # Required for flask-jwt-extended and msal
pyjwt<2
Review comment:
pyjwt should remain here because of flask-jwt and few other providers.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550729336
##########
File path: scripts/ci/pre_commit/pre_commit_check_eager_constraints.sh
##########
@@ -0,0 +1,60 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+set -euo pipefail
+# shellcheck source=scripts/ci/libraries/_script_init.sh
+. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+
+
+set_eager_upgrade_additional_requirements_variable
+
+PROD_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile | cut -d'"' -f 2)
+CI_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile.ci | cut -d'"' -f 2)
+
+error="false"
+if [[ ${PROD_DOCKER_FILE_EAGER_CONSTRAINTS} != "${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}" ]]; then
+ echo
+ echo """
+\e[31m
+The EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS in ${AIRFLOW_SOURCES}/airflow/scripts/ci/libraries/_build_images.sh
+and in Dockerfile are different.${COLOR_RESET}
+
+Dockerfile : ${PROD_DOCKER_FILE_EAGER_CONSTRAINTS}
+_build_images.sh: ${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}
+
+Please synchronize them!
+"""
+ error="true"
+fi
+if [[ ${CI_DOCKER_FILE_EAGER_CONSTRAINTS} != "${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}" ]]; then
+ echo
+ echo """
Review comment:
```suggestion
echo -e """
```
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550739704
##########
File path: scripts/ci/pre_commit/pre_commit_check_eager_constraints.sh
##########
@@ -0,0 +1,60 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+set -euo pipefail
+# shellcheck source=scripts/ci/libraries/_script_init.sh
+. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+
+
+set_eager_upgrade_additional_requirements_variable
+
+PROD_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile | cut -d'"' -f 2)
+CI_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile.ci | cut -d'"' -f 2)
+
+error="false"
+if [[ ${PROD_DOCKER_FILE_EAGER_CONSTRAINTS} != "${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}" ]]; then
+ echo
+ echo """
+\e[31m
+The EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS in ${AIRFLOW_SOURCES}/airflow/scripts/ci/libraries/_build_images.sh
+and in Dockerfile are different.${COLOR_RESET}
+
+Dockerfile : ${PROD_DOCKER_FILE_EAGER_CONSTRAINTS}
+_build_images.sh: ${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}
+
+Please synchronize them!
+"""
+ error="true"
+fi
+if [[ ${CI_DOCKER_FILE_EAGER_CONSTRAINTS} != "${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}" ]]; then
+ echo
+ echo """
Review comment:
Ditto
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550638552
##########
File path: Dockerfile.ci
##########
@@ -314,6 +314,11 @@ COPY airflow/__init__.py ${AIRFLOW_SOURCES}/airflow/__init__.py
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
ENV UPGRADE_TO_NEWER_DEPENDENCIES=${UPGRADE_TO_NEWER_DEPENDENCIES}
+# Those are additional constraints that are needed for some extras but we do not want to
+# Force them on the main Airflow package
+ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS
Review comment:
Yeah. I had to update it still a bit, to handle our image building process in master. Pushing a fix (without those lines)
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553174733
##########
File path: scripts/docker/install_additional_dependencies.sh
##########
@@ -0,0 +1,38 @@
+#!/usr/bin/env bash
Review comment:
Good point. I double-checked and I had a typo in PROD Dockerfile:
```
if [[ -n "${ADDITIONAL_PYTHON_DEPS}" ]]; then \
/scripts/docker/install_addtional_dependencies.sh; \
fi;
```
So it was not used. Fixed the typo :)
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550769871
##########
File path: setup.cfg
##########
@@ -86,6 +86,7 @@ install_requires =
# cattrs >= 1.1.0 dropped support for Python 3.6
cattrs>=1.0, <1.1.0;python_version<="3.6"
cattrs~=1.1;python_version>"3.6"
+ chardet<4.0.0
Review comment:
chardet is a common dependency across many providers and 4.0.0 is conflicting with several of them.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756404110
Sure @mik-laj - my dream is to have more people involved in speeding up the CI. It would be great to let the INFRA knpw that problems are still there, by several members of our community who suffer.
----------------------------------------------------------------
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 edited a comment on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756404110
Sure @mik-laj - my dream is to have more people involved in speeding up the CI. It would be great to let the INFRA know that problems are still there, by several members of our community who suffer (bacause this is the main reason for slowness, not review)
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552889798
##########
File path: scripts/ci/libraries/_initialization.sh
##########
@@ -613,32 +616,50 @@ Verbosity variables:
VERBOSE: ${VERBOSE}
VERBOSE_COMMANDS: ${VERBOSE_COMMANDS}
-Image build variables:
+Common image build variables:
+ INSTALL_AIRFLOW_VERSION=${INSTALL_AIRFLOW_VERSION}
+ INSTALL_AIRFLOW_REFERENCE=${INSTALL_AIRFLOW_REFERENCE}
+ INSTALL_FROM_PYPI: ${INSTALL_FROM_PYPI}
+ AIRFLOW_PRE_CACHED_PIP_PACKAGES: ${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
UPGRADE_TO_NEWER_DEPENDENCIES: ${UPGRADE_TO_NEWER_DEPENDENCIES}
+ CONTINUE_ON_PIP_CHECK_FAILURE: ${CONTINUE_ON_PIP_CHECK_FAILURE}
CHECK_IMAGE_FOR_REBUILD: ${CHECK_IMAGE_FOR_REBUILD}
-
+ AIRFLOW_CONSTRAINTS_LOCATION: ${AIRFLOW_CONSTRAINTS_LOCATION}
+ AIRFLOW_CONSTRAINTS_REFERENCE: ${AIRFLOW_CONSTRAINTS_REFERENCE}
+ INSTALL_PROVIDERS_FROM_SOURCES: ${INSTALL_PROVIDERS_FROM_SOURCES}
+ INSTALL_FROM_DOCKER_CONTEXT_FILES: ${INSTALL_FROM_DOCKER_CONTEXT_FILES}
+ ADDITIONAL_AIRFLOW_EXTRAS: ${ADDITIONAL_AIRFLOW_EXTRAS}
+ ADDITIONAL_PYTHON_DEPS: ${ADDITIONAL_PYTHON_DEPS}
+ DEV_APT_COMMAND: ${DEV_APT_COMMAND}
+ ADDITIONAL_DEV_APT_COMMAND: ${ADDITIONAL_DEV_APT_COMMAND}
+ DEV_APT_DEPS: ${DEV_APT_DEPS}
+ ADDITIONAL_DEV_APT_DEPS: ${ADDITIONAL_DEV_APT_DEPS}
+ RUNTIME_APT_COMMAND: ${RUNTIME_APT_COMMAND}
+ ADDITIONAL_RUNTIME_APT_COMMAND: ${ADDITIONAL_RUNTIME_APT_COMMAND}
+ RUNTIME_APT_DEPS: ${RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_DEPS: ${ADDITIONAL_RUNTIME_APT_DEPS}
+ ADDITIONAL_RUNTIME_APT_ENV: ${ADDITIONAL_RUNTIME_APT_ENV}
+
+Production image build variables:
+
+ AIRFLOW_INSTALLATION_METHOD: ${AIRFLOW_INSTALLATION_METHOD}
+ AIRFLOW_INSTALL_VERSION: ${AIRFLOW_INSTALL_VERSION}
+ AIRFLOW_SOURCES_FROM: ${AIRFLOW_SOURCES_FROM}
+ AIRFLOW_SOURCES_TO: ${AIRFLOW_SOURCES_TO}
Detected GitHub environment:
USE_GITHUB_REGISTRY=${USE_GITHUB_REGISTRY}
GITHUB_REGISTRY=${GITHUB_REGISTRY}
- GITHUB_REPOSITORY=${GITHUB_REPOSITORY}
+ GITHUB_REPOSITORY=${GITHUB_REPOSITORY}.
Review comment:
?
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550770147
##########
File path: setup.py
##########
@@ -779,26 +779,6 @@ def get_provider_package_from_package_id(package_id: str):
return f"apache-airflow-providers-{package_suffix}"
-class AirflowDistribution(Distribution):
Review comment:
We do not need it any more, because with recent setup.py refactor the preinstalled providers were moved back to providers.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r553173045
##########
File path: Dockerfile
##########
@@ -172,23 +169,42 @@ RUN if [[ -f /docker-context-files/.pypirc ]]; then \
cp /docker-context-files/.pypirc /root/.pypirc; \
fi
+COPY scripts/docker /scripts/docker
+# fix permission issue in Azure DevOps when running the scripts
+RUN chmod a+x /scripts/docker/*.sh
+
+ARG AIRFLOW_PIP_VERSION
+ENV AIRFLOW_PIP_VERSION=${AIRFLOW_PIP_VERSION}
+
+# Install Airflow ith "--user" flag, so that we can copy the whole .local folder to the final image
+# from the build image and always in non-editable mode
+ENV AIRFLOW_INSTALL_USER_FLAG="--user"
Review comment:
It's needed to pass it to the script which is shared between PROD and CI docker and this is what we are passing to the script. ENV Variable is the most "readable" way of doing it. We could pass it by parameters of the script, but I think variables are much easier to maintain.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550729182
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -955,50 +979,33 @@ It can mean one of those:
2) You changed some dependencies in setup.py or setup.cfg and they are conflicting.
+
In case 1) - apologies for the trouble.Please let committers know and they will fix it. You might
be asked to rebase to the latest master after the problem is fixed.
In case 2) - Follow the steps below:
-* consult the committers if you are unsure what to do. Just comment in the PR that you need help, if you do,
- but try to follow those instructions first!
-
-* ask the committer to set 'upgrade to newer dependencies'. All dependencies in your PR will be updated
- to latest 'good' versions and you will be able to check if they are not conflicting.
-
-* run locally the image that is failing with Breeze:
-
- ./breeze ${1}--github-image-id ${GITHUB_REGISTRY_PULL_IMAGE_TAG} --backend ${BACKEND="sqlite"} --python ${PYTHON_MAJOR_MINOR_VERSION}
-
-* your setup.py and setup.cfg will be mounted to the container. You will be able to iterate with
- different setup.py versions.
-
-* in container your can run 'pipdeptree' to figure out where the dependency conflict comes from.
-
-* Some useful commands that can help yoy to find out dependencies you have:
-
- * 'pipdeptree | less' (you can then search through the dependencies with vim-like shortcuts)
-
- * 'pipdeptree > /files/pipdeptree.txt' - this will produce a pipdeptree.txt file in your source
- 'files' directory and you can open it in editor of your choice,
-
- * 'pipdeptree | grep YOUR_DEPENDENCY' - to see all the requirements your dependency has as specified
- by other packages
-
-* figure out which dependency limits should be upgraded. Upgrade them in corresponding setup.py extras
- and run pip to upgrade your dependencies accordingly:
-
- pip install '.[all]' --upgrade --upgrade-strategy eager
+* try to build CI and then PROD image locally with breeze, adding --upgrade-to-newer-dependencies flag:
-* run pip check to figure out if the dependencies have been fixed. It should let you know which dependencies
- are conflicting or (hurray!) if there are no conflicts:
+${COLOR_BLUE}
+ for python_version in ${CURRENT_PYTHON_MAJOR_MINOR_VERSIONS[*]}
+ do
+ ./breeze build-image --upgrade-to-newer-dependencies --python "$\{python_version\}"
Review comment:
nit: As a user, I don't like pasting a lot of line bash scripts into the terminalI guess we can just display the 4 commands that the user needs to run.
```bash
./breeze build-image --upgrade-to-newer-dependencies --python 3.7
./breeze build-image --upgrade-to-newer-dependencies --python 3.7
./breeze build-image --upgrade-to-newer-dependencies --python 3.8
./breeze build-image --upgrade-to-newer-dependencies --python 3.9
```
Or alternatively, as a one-line script.
```bash
echo ${CURRENT_PYTHON_MAJOR_MINOR_VERSIONS[*]} | xargs -t -n 1 ./breeze build-image --upgrade-to-newer-dependencies --python
```
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552406898
##########
File path: Dockerfile.ci
##########
@@ -272,17 +260,42 @@ ENV INSTALL_FROM_DOCKER_CONTEXT_FILES=${INSTALL_FROM_DOCKER_CONTEXT_FILES}
ARG INSTALL_FROM_PYPI="true"
ENV INSTALL_FROM_PYPI=${INSTALL_FROM_PYPI}
+ARG AIRFLOW_PIP_VERSION=20.2.4
+ENV AIRFLOW_PIP_VERSION=${AIRFLOW_PIP_VERSION}
+
+# In the CI image we always:
+# * install MySQL
+# * install airflow from current sources, not from PyPI package
+# * install airflow without `--user` flag
+# * install airflow in editable mode
+# * install always current version of airrflow
Review comment:
```suggestion
# * install always current version of airflow
```
----------------------------------------------------------------
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 edited a comment on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756404110
Sure @mik-laj - my dream is to have more people involved in speeding up the CI. It would be great to let the INFRA know that problems are still there, by several members of our community who suffer.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552413610
##########
File path: setup.cfg
##########
@@ -86,6 +86,7 @@ install_requires =
# cattrs >= 1.1.0 dropped support for Python 3.6
cattrs>=1.0, <1.1.0;python_version<="3.6"
cattrs~=1.1;python_version>"3.6"
+ chardet<4.0.0
Review comment:
This is another problem with snowflake, if I remember correctly.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550769457
##########
File path: Dockerfile
##########
@@ -179,16 +179,25 @@ RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
ARG AIRFLOW_PRE_CACHED_PIP_PACKAGES="false"
ENV AIRFLOW_PRE_CACHED_PIP_PACKAGES=${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
+# By default we install providers from PyPI but in case of Breze build we want to install providers
+# from local sources without the neeed of preparing provider packages upfront. This value is
+# automatically overridden by Breeze scripts.
+ARG INSTALL_PROVIDERS_FROM_SOURCES="false"
+ENV INSTALL_PROVIDERS_FROM_SOURCES=${INSTALL_PROVIDERS_FROM_SOURCES}
+
# In case of Production build image segment we want to pre-install master version of airflow
# dependencies from GitHub so that we do not have to always reinstall it from the scratch.
+# The Airflow (and providers in case INSTALL_PROVIDERS_FROM_SOURCES is "false")
+# are uninstalled, only depeendencies remain
RUN if [[ ${AIRFLOW_PRE_CACHED_PIP_PACKAGES} == "true" ]]; then \
if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \
AIRFLOW_EXTRAS=${AIRFLOW_EXTRAS/mysql,}; \
fi; \
pip install --user \
"https://github.com/${AIRFLOW_REPO}/archive/${AIRFLOW_BRANCH}.tar.gz#egg=apache-airflow[${AIRFLOW_EXTRAS}]" \
- --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}" \
- && pip uninstall --yes apache-airflow; \
+ --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \
+ pip freeze | grep apache-airflow-providers | grep -v '@' | xargs pip uninstall --yes || true ; \
Review comment:
For 'cache' installation we only care about dependencies. So removing airflow + providers here makes sense - they will be installed further down.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753411710
This one will get green only after #13422 is merged to master
.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552412153
##########
File path: scripts/docker/install_airflow.sh
##########
@@ -0,0 +1,68 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Install airflow using regular 'pip install' command. This install airflow depending on the arguments:
+# AIRFLOW_INSTALLATION_METHOD - determines where to install airflow form:
+# "." - installs airflow from local sources
+# "apache-airflow" - installs airflow from PyPI 'apache-airflow' package
+# AIRFLOW_INSTALL_VERSION - optionally specify version to install
+# UPGRADE_TO_NEWER_DEPENDENCIES - determines whether eager-upgrade should be performed with the
+# dependencies (with EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS added)
+# In all cases
+# If UPGRADE_TO_NEWER_DEPENDENCIES is set to true,
+# shellcheck disable=SC2086
Review comment:
```suggestion
# shellcheck disable=SC2086
test -v AIRFLOW_INSTALLATION_METHOD
test -v AIRFLOW_INSTALL_EDITABLE_FLAG
test -v AIRFLOW_INSTALL_USER_FLAG
test -v INSTALL_MYSQL_CLIENT
test -v UPGRADE_TO_NEWER_DEPENDENCIES
test -v CONTINUE_ON_PIP_CHECK_FAILURE
test -v AIRFLOW_CONSTRAINTS_LOCATION
```
Extra sanity checks
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550769375
##########
File path: .github/workflows/build-images-workflow-run.yml
##########
@@ -259,6 +259,7 @@ jobs:
PYTHON_MAJOR_MINOR_VERSION: ${{ matrix.python-version }}
GITHUB_REGISTRY_PUSH_IMAGE_TAG: ${{ github.event.workflow_run.id }}
UPGRADE_TO_NEWER_DEPENDENCIES: ${{ needs.build-info.outputs.upgradeToNewerDependencies }}
+ CONTINUE_ON_PIP_CHECK_FAILURE: "true"
Review comment:
We want to continue even if PIP_CHECK fails - so that we can get the image and continue testing regardless. We haave "Verify image" jobs that will show the failure in the CI.yml
----------------------------------------------------------------
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 edited a comment on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753004570
CC: @r-richmond --> I think this is much better solution of the problem you wanted to solve in #13333
Happy New Year!
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756434179
> Prod image build failed
Yeah. Not going to get to sleep before this one gets green!
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550739696
##########
File path: scripts/ci/pre_commit/pre_commit_check_eager_constraints.sh
##########
@@ -0,0 +1,60 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+set -euo pipefail
+# shellcheck source=scripts/ci/libraries/_script_init.sh
+. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+
+
+set_eager_upgrade_additional_requirements_variable
+
+PROD_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile | cut -d'"' -f 2)
+CI_DOCKER_FILE_EAGER_CONSTRAINTS=$(grep "ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS=" Dockerfile.ci | cut -d'"' -f 2)
+
+error="false"
+if [[ ${PROD_DOCKER_FILE_EAGER_CONSTRAINTS} != "${EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS_VARIABLE}" ]]; then
+ echo
+ echo """
Review comment:
We do not need it here. Our variables are already defined in the way that does not need backlash interpretation:
```
COLOR_RESET=$'\e[0m'
```
I am also planning to do in for the in_container variables as I noticed it's not done there this way.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756374096
Yeah. I also wished that the other jiobs do not block the queue, I submitted it this morning and only found out that it fails late afternoon. Until we have self-hosted runnrers up and running, I think there is nothing we can do to speed things up.
I do not see any other options for optimisations. Maybe we should again escalate to infra? I hate doing it again - but maybe if others do it as well, we can get more attention from GitHub about 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] potiuk commented on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-755981788
Hey @kaxil @mik-laj - I think I solved everything. Running some last local tests but I think it's good to go.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552405805
##########
File path: Dockerfile
##########
@@ -172,23 +169,42 @@ RUN if [[ -f /docker-context-files/.pypirc ]]; then \
cp /docker-context-files/.pypirc /root/.pypirc; \
fi
+COPY scripts/docker /scripts/docker
+# fix permission issue in Azure DevOps when running the scripts
+RUN chmod a+x /scripts/docker/*.sh
+
+ARG AIRFLOW_PIP_VERSION
+ENV AIRFLOW_PIP_VERSION=${AIRFLOW_PIP_VERSION}
+
+# Install Airflow ith "--user" flag, so that we can copy the whole .local folder to the final image
+# from the build image and always in non-editable mode
+ENV AIRFLOW_INSTALL_USER_FLAG="--user"
+ENV AIRFLOW_INSTALL_EDITABLE_FLAG=""
+
+# Upgrade to specific PIP version
RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
# By default we do not use pre-cached packages, but in CI/Breeze environment we override this to speed up
# builds in case setup.py/setup.cfg changed. This is pure optimisation of CI/Breeze builds.
ARG AIRFLOW_PRE_CACHED_PIP_PACKAGES="false"
ENV AIRFLOW_PRE_CACHED_PIP_PACKAGES=${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
+# By default we install providers from PyPI but in case of Breeze build we want to install providers
+# from local sources without the neeed of preparing provider packages upfront. This value is
Review comment:
```suggestion
# from local sources without the need of preparing provider packages upfront. This value is
```
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-755982182
I added a few more echos in the scripts to explain what's going on while docker building
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756429756
[The Workflow run](https://github.com/apache/airflow/actions/runs/470115270) is cancelling this PR. Building images for the PR has failed. Follow the 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] mik-laj commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r552413298
##########
File path: scripts/docker/install_from_docker_context_files.sh
##########
@@ -0,0 +1,86 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# shellcheck disable=SC2086
+
+# Installs airflow and provider packages from locally present docker context files
+# This is used in CI to install airflow and provider packages in the CI system of ours
+# The packages are prepared from current sources and placed in the 'docker-context-files folder
+# Then both airflow and provider packages are installed using those packages rather than
+# PyPI
Review comment:
A check would be helpful here as well, because it's very easy to get confused when scripts communicate through environment variables.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-756395040
@potiuk I did not want to put pressure on you, but only to inform other reviewers and the community members that my work is blocked by this change. If reviewers want to make reviews, it is worth doing it a little earlier. We discuss my problems at our meetings, but I don't think these problems appear in the public discussion. This creates a somewhat less open community. For this reason, I try, despite the fact that we talk about something at other meetings, still describe my dreams in public discussions.
----------------------------------------------------------------
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 pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-754495724
@mik-laj -> this change should help in cases like you had with #13224 where setting label caused the PR to succeed but then master was broken after merge. Let's mege it and re-try your data-catalog on top of it.
I removed teh 'upgrade to newer dependencies" label. It should not be needed any more.
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550729182
##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -955,50 +979,33 @@ It can mean one of those:
2) You changed some dependencies in setup.py or setup.cfg and they are conflicting.
+
In case 1) - apologies for the trouble.Please let committers know and they will fix it. You might
be asked to rebase to the latest master after the problem is fixed.
In case 2) - Follow the steps below:
-* consult the committers if you are unsure what to do. Just comment in the PR that you need help, if you do,
- but try to follow those instructions first!
-
-* ask the committer to set 'upgrade to newer dependencies'. All dependencies in your PR will be updated
- to latest 'good' versions and you will be able to check if they are not conflicting.
-
-* run locally the image that is failing with Breeze:
-
- ./breeze ${1}--github-image-id ${GITHUB_REGISTRY_PULL_IMAGE_TAG} --backend ${BACKEND="sqlite"} --python ${PYTHON_MAJOR_MINOR_VERSION}
-
-* your setup.py and setup.cfg will be mounted to the container. You will be able to iterate with
- different setup.py versions.
-
-* in container your can run 'pipdeptree' to figure out where the dependency conflict comes from.
-
-* Some useful commands that can help yoy to find out dependencies you have:
-
- * 'pipdeptree | less' (you can then search through the dependencies with vim-like shortcuts)
-
- * 'pipdeptree > /files/pipdeptree.txt' - this will produce a pipdeptree.txt file in your source
- 'files' directory and you can open it in editor of your choice,
-
- * 'pipdeptree | grep YOUR_DEPENDENCY' - to see all the requirements your dependency has as specified
- by other packages
-
-* figure out which dependency limits should be upgraded. Upgrade them in corresponding setup.py extras
- and run pip to upgrade your dependencies accordingly:
-
- pip install '.[all]' --upgrade --upgrade-strategy eager
+* try to build CI and then PROD image locally with breeze, adding --upgrade-to-newer-dependencies flag:
-* run pip check to figure out if the dependencies have been fixed. It should let you know which dependencies
- are conflicting or (hurray!) if there are no conflicts:
+${COLOR_BLUE}
+ for python_version in ${CURRENT_PYTHON_MAJOR_MINOR_VERSIONS[*]}
+ do
+ ./breeze build-image --upgrade-to-newer-dependencies --python "$\{python_version\}"
Review comment:
nit: As a user, I don't like pasting a lot of line bash scripts into the terminalI guess we can just display the 4 commands that the user needs to run.
```
./breeze build-image --upgrade-to-newer-dependencies --python 3.7
./breeze build-image --upgrade-to-newer-dependencies --python 3.7
./breeze build-image --upgrade-to-newer-dependencies --python 3.8
./breeze build-image --upgrade-to-newer-dependencies --python 3.9
```
Or alternatively, as a one-line script.
```
echo ${CURRENT_PYTHON_MAJOR_MINOR_VERSIONS[*]} | xargs -t -n 1 ./breeze build-image --upgrade-to-newer-dependencies --python
```
----------------------------------------------------------------
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 #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r551898906
##########
File path: setup.cfg
##########
@@ -86,6 +86,7 @@ install_requires =
# cattrs >= 1.1.0 dropped support for Python 3.6
cattrs>=1.0, <1.1.0;python_version<="3.6"
cattrs~=1.1;python_version>"3.6"
+ chardet<4.0.0
Review comment:
Yep. I will build the image and try to analyse it deeper to find out where to add them (in providers)
----------------------------------------------------------------
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] kaxil commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r551889131
##########
File path: Dockerfile
##########
@@ -179,16 +179,30 @@ RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
ARG AIRFLOW_PRE_CACHED_PIP_PACKAGES="false"
ENV AIRFLOW_PRE_CACHED_PIP_PACKAGES=${AIRFLOW_PRE_CACHED_PIP_PACKAGES}
+# By default we install providers from PyPI but in case of Breze build we want to install providers
+# from local sources without the neeed of preparing provider packages upfront. This value is
+# automatically overridden by Breeze scripts.
+ARG INSTALL_PROVIDERS_FROM_SOURCES="false"
+ENV INSTALL_PROVIDERS_FROM_SOURCES=${INSTALL_PROVIDERS_FROM_SOURCES}
+
+# Increase the value here to force reinstalling Apache Airflow pip dependencies
+ARG PIP_DEPENDENCIES_EPOCH_NUMBER="1"
+ENV PIP_DEPENDENCIES_EPOCH_NUMBER=${PIP_DEPENDENCIES_EPOCH_NUMBER}
+
# In case of Production build image segment we want to pre-install master version of airflow
# dependencies from GitHub so that we do not have to always reinstall it from the scratch.
+# The Airflow (and providers in case INSTALL_PROVIDERS_FROM_SOURCES is "false")
+# are uninstalled, only depeendencies remain
Review comment:
```suggestion
# are uninstalled, only dependencies remain
```
----------------------------------------------------------------
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] r-richmond commented on a change in pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
r-richmond commented on a change in pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#discussion_r550635238
##########
File path: Dockerfile.ci
##########
@@ -314,6 +314,11 @@ COPY airflow/__init__.py ${AIRFLOW_SOURCES}/airflow/__init__.py
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
ENV UPGRADE_TO_NEWER_DEPENDENCIES=${UPGRADE_TO_NEWER_DEPENDENCIES}
+# Those are additional constraints that are needed for some extras but we do not want to
+# Force them on the main Airflow package
+ARG EAGER_UPGRADE_ADDITIONAL_REQUIREMENTS
Review comment:
Is this line necessary given the one below 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] potiuk edited a comment on pull request #13409: Removes provider-imposed requirements from setup.cfg
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13409:
URL: https://github.com/apache/airflow/pull/13409#issuecomment-753285136
Actually, this one is even better now - we have a separate set of those "provider-imposed" requirements in Dockerfile.ci and in Dockerfile. This means that even our production docker imaage will not have limitations (like `requests`) imposed by Snowflke for example. It will only require limitations from the providers that are pre-installed (currently there is only the urrlib3 library from boto3/amazon). And they are not "hard" ones embeeded in apache-airflow package - they are only used at installation time.
----------------------------------------------------------------
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