You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/09 05:45:33 UTC
[GitHub] [airflow] Junnplus opened a new pull request #13585: Add installRequirements for helm chart
Junnplus opened a new pull request #13585:
URL: https://github.com/apache/airflow/pull/13585
we need install extra python packages before container start, I see other implementation of helm chart all supported.
---
**^ 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] potiuk commented on a change in pull request #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554425694
##########
File path: chart/templates/_helpers.yaml
##########
@@ -357,6 +357,10 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s/config/airflow_local_settings.py" .Values.airflowHome) | quote }}
{{- end }}
+{{ define "airflow_install_requirements_path" -}}
Review comment:
This is of course slightly different than the Dockerfile ones (those are embedded in the customized image) but the meaning is the same, so name should also be the same.
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#issuecomment-757114726
@Junnplus Our Docker image is size optimized and therefore not all dependencies can be installed this way. I recommend that you follow the documentation for Docker images that describes how to install additional packages in the image. See: http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow/latest/production-deployment.html#production-container-images
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554425542
##########
File path: chart/templates/_helpers.yaml
##########
@@ -357,6 +357,10 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s/config/airflow_local_settings.py" .Values.airflowHome) | quote }}
{{- end }}
+{{ define "airflow_install_requirements_path" -}}
Review comment:
The name should be change to match the naming we already have.
In our Dockerfile we already have ADDITIONAL_PYTHON_DEPS variable and we are installing additional python dependencies. @mik-laj rightfully noticed that not alll deps can be installed this way (we do not have build-essentials in the image for one) but some can be, and I agree it's nice to have, them, it should be clear however, that those are additional dependencies, and they are not de-facto "install_requirements". The "install_requirements" in python world have a very precise meaning - those are to be found in `setup.py` or `setup.cfg` and they mean "those need to be installed BEFORE you install airflow". What we are talking about here are "additional python dependencies" which are installed AFTER airflow is installed. So let's name it this way.
Relevant snippet from Dockerfile:
```
# Add extra python dependencies
ARG ADDITIONAL_PYTHON_DEPS=""
ENV ADDITIONAL_PYTHON_DEPS=${ADDITIONAL_PYTHON_DEPS}
```
----------------------------------------------------------------
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] Junnplus closed pull request #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
Junnplus closed pull request #13585:
URL: https://github.com/apache/airflow/pull/13585
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554426283
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
+if [[ -f "${AIRFLOW_HOME}/requirements.txt" ]]; then
+ pip install --user -r "${AIRFLOW_HOME}/requirements.txt"
Review comment:
This is not correct way of installing the dependencies. Look here:
https://github.com/apache/airflow/blob/master/scripts/docker/install_additional_dependencies.sh
This script has been extracted yesterdy from the Dockerfile and it shows the right way of installing additional dependencies. It handles eager upgrade case which is not relevant but the "else" part of the if is how the install should be done:
`pip install --user --upgrade --upgrade-strategy only-if-needed`
This way you will automatically upgrade existing dependencies in case any of the existing dependencies require it. This should be likely also configurable (one might want to disable it), but it should be the default IMHO (unless there are good reasons not to do it).
Additionally it would be great to add a "pip check" step afterwards and fail if there are some consistency problems. This will force anyone who adds their requirements this way will not add conflicting requirements.
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554424980
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
Review comment:
Using Airflow AIRFLOW_HOME for extra requirements is not a good idea. It's better to mount the whole folder where you will mount various files, and in this case overriding AIRFLOW_HOME is wrong. Besides AIRFLOW_HOME is read-write where the requirements and related files should be "read-only"
It will be better if you follow the convention that we already have in our "CI" image.
* "/files" directory should be there for any files mounted to to the image
* "/files/requirements" should be the folder where requirements are placed
* "/files/requirements/requirements-3.7.txt" should be the file that you load (depending on the python version you might have different requirements"
Additionally you must print some diagnostics information so that when you see the log files you will see exactly what happening.
This way you will not only be able to use it in production but also test it seamlessly for different python versions in our 'breeze` development envionment.
Relevant snippet from the `run_init_script.sh`:
```
if [ -z "${FILES_DIR+x}" ]; then
export FILES_DIR="/files"
fi
if [ -z "${AIRFLOW_BREEZE_CONFIG_DIR+x}" ]; then
export AIRFLOW_BREEZE_CONFIG_DIR="${FILES_DIR}/airflow-breeze-config"
fi
if [ -z "${INIT_SCRIPT_FILE=}" ]; then
export INIT_SCRIPT_FILE="init.sh"
fi
if [[ -d "${AIRFLOW_BREEZE_CONFIG_DIR}" && \
-f "${AIRFLOW_BREEZE_CONFIG_DIR}/${INIT_SCRIPT_FILE}" ]]; then
pushd "${AIRFLOW_BREEZE_CONFIG_DIR}" >/dev/null 2>&1 || exit 1
echo
echo "Sourcing the initialization script from ${INIT_SCRIPT_FILE} in ${AIRFLOW_BREEZE_CONFIG_DIR}"
echo
# shellcheck disable=1090
source "${INIT_SCRIPT_FILE}"
popd >/dev/null 2>&1 || exit 1
else
echo
echo "You can add ${AIRFLOW_BREEZE_CONFIG_DIR} directory and place ${INIT_SCRIPT_FILE}"
echo "In it to make breeze source an initialization script automatically for you"
echo
fi
```
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
Review comment:
Using Airflow AIRFLOW_HOME for extra requirements is not a good idea. It's better to mount the whole folder where you will mount various files, and in this case overriding AIRFLOW_HOME is wrong. Besides AIRFLOW_HOME is read-write where the requirements and related files should be "read-only"
It will be better if you follow the convention that we already have in our "CI" image.
* "/files" directory should be there for any files mounted to to the image
* "/files/requirements" should be the folder where requirements are placed
* "/files/requirements/requirements-3.7.txt" should be the file that you load (depending on the python version you might have different requirements)
Additionally you must print some diagnostics information so that when you see the log files you will see exactly what happening.
This way you will not only be able to use it in production but also test it seamlessly for different python versions in our 'breeze` development envionment.
Relevant snippet from the `run_init_script.sh`:
```
if [ -z "${FILES_DIR+x}" ]; then
export FILES_DIR="/files"
fi
if [ -z "${AIRFLOW_BREEZE_CONFIG_DIR+x}" ]; then
export AIRFLOW_BREEZE_CONFIG_DIR="${FILES_DIR}/airflow-breeze-config"
fi
if [ -z "${INIT_SCRIPT_FILE=}" ]; then
export INIT_SCRIPT_FILE="init.sh"
fi
if [[ -d "${AIRFLOW_BREEZE_CONFIG_DIR}" && \
-f "${AIRFLOW_BREEZE_CONFIG_DIR}/${INIT_SCRIPT_FILE}" ]]; then
pushd "${AIRFLOW_BREEZE_CONFIG_DIR}" >/dev/null 2>&1 || exit 1
echo
echo "Sourcing the initialization script from ${INIT_SCRIPT_FILE} in ${AIRFLOW_BREEZE_CONFIG_DIR}"
echo
# shellcheck disable=1090
source "${INIT_SCRIPT_FILE}"
popd >/dev/null 2>&1 || exit 1
else
echo
echo "You can add ${AIRFLOW_BREEZE_CONFIG_DIR} directory and place ${INIT_SCRIPT_FILE}"
echo "In it to make breeze source an initialization script automatically for you"
echo
fi
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -357,6 +357,10 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s/config/airflow_local_settings.py" .Values.airflowHome) | quote }}
{{- end }}
+{{ define "airflow_install_requirements_path" -}}
Review comment:
The name should be change to match the naming we already have.
In our Dockerfile we already have ADDITIONAL_PYTHON_DEPS variable and we are installing additional python dependencies. @mik-laj rightfully noticed that not alll deps can be installed this way (we do not have build-essentials in the image for one) but some can be, and I agree it's nice to have, them, it should be clear however, that those are additional dependencies, and they are not de-facto "install_requirements". The "install_requirements" in python world have a very precise meaning - those are to be found in `setup.py` or `setup.cfg` and they mean "those need to be installed BEFORE you install airflow". What we are talking about here are "additional python dependencies" which are installed AFTER airflow is installed. So let's name it this way.
##########
File path: chart/templates/_helpers.yaml
##########
@@ -357,6 +357,10 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s/config/airflow_local_settings.py" .Values.airflowHome) | quote }}
{{- end }}
+{{ define "airflow_install_requirements_path" -}}
Review comment:
The name should be change to match the naming we already have.
In our Dockerfile we already have ADDITIONAL_PYTHON_DEPS variable and we are installing additional python dependencies. @mik-laj rightfully noticed that not alll deps can be installed this way (we do not have build-essentials in the image for one) but some can be, and I agree it's nice to have, them, it should be clear however, that those are additional dependencies, and they are not de-facto "install_requirements". The "install_requirements" in python world have a very precise meaning - those are to be found in `setup.py` or `setup.cfg` and they mean "those need to be installed BEFORE you install airflow". What we are talking about here are "additional python dependencies" which are installed AFTER airflow is installed. So let's name it this way.
Relevant snippet from Dockerfile:
```
# Add extra python dependencies
ARG ADDITIONAL_PYTHON_DEPS=""
ENV ADDITIONAL_PYTHON_DEPS=${ADDITIONAL_PYTHON_DEPS}
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -357,6 +357,10 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s/config/airflow_local_settings.py" .Values.airflowHome) | quote }}
{{- end }}
+{{ define "airflow_install_requirements_path" -}}
Review comment:
This is of course slightly different than the Dockerfile ones (those are embedded in the customized image) but the meaning is the same, so name should also be the same.
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
+if [[ -f "${AIRFLOW_HOME}/requirements.txt" ]]; then
+ pip install --user -r "${AIRFLOW_HOME}/requirements.txt"
Review comment:
This is not correct way of installing the dependencies. Look here:
https://github.com/apache/airflow/blob/master/scripts/docker/install_additional_dependencies.sh
This script has been extracted yesterdy from the Dockerfile and it shows the right way of installing additional dependencies. It handles eager upgrade case which is not relevant but the "else" part of the if is how the install should be done:
`pip install --user --upgrade --upgrade-strategy only-if-needed`
This way you will automatically upgrade existing dependencies in case any of the existing dependencies require it. This should be likely also configurable (one might want to disable it), but it should be the default IMHO (unless there are good reasons not to do it).
Additionally it would be great to add a "pip check" step afterwards and fail if there are some consistency problems. This will force anyone who adds their requirements this way will not add conflicting requirements.
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
+if [[ -f "${AIRFLOW_HOME}/requirements.txt" ]]; then
+ pip install --user -r "${AIRFLOW_HOME}/requirements.txt"
Review comment:
Relevant code snippet:
```
echo
echo Installing additional dependencies upgrading only if needed
echo
pip install ${AIRFLOW_INSTALL_USER_FLAG} \
--upgrade --upgrade-strategy only-if-needed \
${ADDITIONAL_PYTHON_DEPS}
# make sure correct PIP version is used
pip install ${AIRFLOW_INSTALL_USER_FLAG} --upgrade "pip==${AIRFLOW_PIP_VERSION}"
pip check || ${CONTINUE_ON_PIP_CHECK_FAILURE}
```
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#issuecomment-757103414
[The Workflow run](https://github.com/apache/airflow/actions/runs/473588644) 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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554424980
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
Review comment:
Using Airflow AIRFLOW_HOME for extra requirements is not a good idea. It's better to mount the whole folder where you will mount various files, and in this case overriding AIRFLOW_HOME is wrong. Besides AIRFLOW_HOME is read-write where the requirements and related files should be "read-only"
It will be better if you follow the convention that we already have in our "CI" image.
* "/files" directory should be there for any files mounted to to the image
* "/files/requirements" should be the folder where requirements are placed
* "/files/requirements/requirements-3.7.txt" should be the file that you load (depending on the python version you might have different requirements"
Additionally you must print some diagnostics information so that when you see the log files you will see exactly what happening.
This way you will not only be able to use it in production but also test it seamlessly for different python versions in our 'breeze` development envionment.
Relevant snippet from the `run_init_script.sh`:
```
if [ -z "${FILES_DIR+x}" ]; then
export FILES_DIR="/files"
fi
if [ -z "${AIRFLOW_BREEZE_CONFIG_DIR+x}" ]; then
export AIRFLOW_BREEZE_CONFIG_DIR="${FILES_DIR}/airflow-breeze-config"
fi
if [ -z "${INIT_SCRIPT_FILE=}" ]; then
export INIT_SCRIPT_FILE="init.sh"
fi
if [[ -d "${AIRFLOW_BREEZE_CONFIG_DIR}" && \
-f "${AIRFLOW_BREEZE_CONFIG_DIR}/${INIT_SCRIPT_FILE}" ]]; then
pushd "${AIRFLOW_BREEZE_CONFIG_DIR}" >/dev/null 2>&1 || exit 1
echo
echo "Sourcing the initialization script from ${INIT_SCRIPT_FILE} in ${AIRFLOW_BREEZE_CONFIG_DIR}"
echo
# shellcheck disable=1090
source "${INIT_SCRIPT_FILE}"
popd >/dev/null 2>&1 || exit 1
else
echo
echo "You can add ${AIRFLOW_BREEZE_CONFIG_DIR} directory and place ${INIT_SCRIPT_FILE}"
echo "In it to make breeze source an initialization script automatically for you"
echo
fi
```
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554424980
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
Review comment:
Using Airflow AIRFLOW_HOME for extra requirements is not a good idea. It's better to mount the whole folder where you will mount various files, and in this case overriding AIRFLOW_HOME is wrong. Besides AIRFLOW_HOME is read-write where the requirements and related files should be "read-only"
It will be better if you follow the convention that we already have in our "CI" image.
* "/files" directory should be there for any files mounted to to the image
* "/files/requirements" should be the folder where requirements are placed
* "/files/requirements/requirements-3.7.txt" should be the file that you load (depending on the python version you might have different requirements)
Additionally you must print some diagnostics information so that when you see the log files you will see exactly what happening.
This way you will not only be able to use it in production but also test it seamlessly for different python versions in our 'breeze` development envionment.
Relevant snippet from the `run_init_script.sh`:
```
if [ -z "${FILES_DIR+x}" ]; then
export FILES_DIR="/files"
fi
if [ -z "${AIRFLOW_BREEZE_CONFIG_DIR+x}" ]; then
export AIRFLOW_BREEZE_CONFIG_DIR="${FILES_DIR}/airflow-breeze-config"
fi
if [ -z "${INIT_SCRIPT_FILE=}" ]; then
export INIT_SCRIPT_FILE="init.sh"
fi
if [[ -d "${AIRFLOW_BREEZE_CONFIG_DIR}" && \
-f "${AIRFLOW_BREEZE_CONFIG_DIR}/${INIT_SCRIPT_FILE}" ]]; then
pushd "${AIRFLOW_BREEZE_CONFIG_DIR}" >/dev/null 2>&1 || exit 1
echo
echo "Sourcing the initialization script from ${INIT_SCRIPT_FILE} in ${AIRFLOW_BREEZE_CONFIG_DIR}"
echo
# shellcheck disable=1090
source "${INIT_SCRIPT_FILE}"
popd >/dev/null 2>&1 || exit 1
else
echo
echo "You can add ${AIRFLOW_BREEZE_CONFIG_DIR} directory and place ${INIT_SCRIPT_FILE}"
echo "In it to make breeze source an initialization script automatically for you"
echo
fi
```
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#issuecomment-757114726
@Junnplus Our Docker image is size optimized and therefore not all dependencies can be installed this way. I recommend that you follow the documentation for Docker images that describes how to install additional packages in the image. See: http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow/latest/production-deployment.html#production-container-images
----------------------------------------------------------------
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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554425542
##########
File path: chart/templates/_helpers.yaml
##########
@@ -357,6 +357,10 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s/config/airflow_local_settings.py" .Values.airflowHome) | quote }}
{{- end }}
+{{ define "airflow_install_requirements_path" -}}
Review comment:
The name should be change to match the naming we already have.
In our Dockerfile we already have ADDITIONAL_PYTHON_DEPS variable and we are installing additional python dependencies. @mik-laj rightfully noticed that not alll deps can be installed this way (we do not have build-essentials in the image for one) but some can be, and I agree it's nice to have, them, it should be clear however, that those are additional dependencies, and they are not de-facto "install_requirements". The "install_requirements" in python world have a very precise meaning - those are to be found in `setup.py` or `setup.cfg` and they mean "those need to be installed BEFORE you install airflow". What we are talking about here are "additional python dependencies" which are installed AFTER airflow is installed. So let's name it 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] Junnplus commented on pull request #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#issuecomment-757123078
@mik-laj Customizable, scalable production container image looks awesome, but provide a simple way to install python packages can avoid maintaining redundant 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] Junnplus commented on pull request #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#issuecomment-757598754
@potiuk Thanks for the detailed explanation, I will try to build own custom 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 #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#discussion_r554426360
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
export HOME="${AIRFLOW_USER_HOME_DIR}"
fi
+# Install extra python packages if requirements.txt is present.
+if [[ -f "${AIRFLOW_HOME}/requirements.txt" ]]; then
+ pip install --user -r "${AIRFLOW_HOME}/requirements.txt"
Review comment:
Relevant code snippet:
```
echo
echo Installing additional dependencies upgrading only if needed
echo
pip install ${AIRFLOW_INSTALL_USER_FLAG} \
--upgrade --upgrade-strategy only-if-needed \
${ADDITIONAL_PYTHON_DEPS}
# make sure correct PIP version is used
pip install ${AIRFLOW_INSTALL_USER_FLAG} --upgrade "pip==${AIRFLOW_PIP_VERSION}"
pip check || ${CONTINUE_ON_PIP_CHECK_FAILURE}
```
----------------------------------------------------------------
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] Junnplus commented on pull request #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#issuecomment-757123078
@mik-laj Customizable, scalable production container image looks awesome, but provide a simple way to install python packages can avoid maintaining redundant 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] github-actions[bot] commented on pull request #13585: Add installRequirements for helm chart
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13585:
URL: https://github.com/apache/airflow/pull/13585#issuecomment-757103414
[The Workflow run](https://github.com/apache/airflow/actions/runs/473588644) 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