Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Package-Requirements passes along public URL constraints in packaged_requirements.txt #348

Open
wants to merge 8 commits into
base: v2.7.2
Choose a base branch
from
+6 −2
Open

Package-Requirements passes along public URL constraints in packaged_requirements.txt #348

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b96e289
Update entrypoint.sh
url54 Jan 11, 2024
517643a
Update Dockerfile
url54 Jan 11, 2024
6eac8a5
Update entrypoint.sh
url54 Jan 11, 2024
5f41fea
Update README.md
url54 Jan 11, 2024
289efd8
Add constraints.txt to plugins.zip, updated readme
charlielu05 Jan 24, 2024
a0d35f4
Merge pull request #1 from charlielu05/main
url54 Jan 25, 2024
c019a41
changes to reflect PR comments
charlielu05 Apr 23, 2024
aa5bba4
Merge pull request #3 from charlielu05/v2.7.2
url54 Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ For example usage see [Installing Python dependencies using PyPi.org Requirement
- There is a directory at the root of this repository called plugins.
- In this directory, create a file for your new custom plugin.
- Add any Python dependencies to `requirements/requirements.txt`.
- Adds a local `constraints.txt` file to the `plugins/` directory, this is zipped together into the `plugins.zip` artefact.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: artefact -> artifact

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed spelling to artifact

- Creates a new `packaged_requirements.txt` file with the correct configuration for `--find-links` and `--constraint`. This file should be the one you rename to `requirements.txt` and upload to S3 to be used by MWAA.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming isn't strictly necessary since you can configure the MWAA environment to the requirements file regardless of name. This can be left up to the individual user. The second sentence could be removed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed second sentence.


**Note**: this step assumes you have a DAG that corresponds to the custom plugin. For example usage [MWAA Code Examples](https://docs.aws.amazon.com/mwaa/latest/userguide/sample-code.html).

Expand Down
1 change: 1 addition & 0 deletions docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ ARG INDEX_URL=""
ENV AIRFLOW_HOME=${AIRFLOW_USER_HOME}
ENV PATH="$PATH:/usr/local/airflow/.local/bin:/root/.local/bin:/usr/local/airflow/.local/lib/python3.10/site-packages"
ENV PYTHON_VERSION=3.11.6
ENV AIRFLOW_VERSION=2.7.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: ENV AIRFLOW_VERSION=$AIRFLOW_VERSION to reduce any source for error and avoid having the same magic-number defined in two places.

Copy link

@charlielu05 charlielu05 Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using curly brackets to keep consistency with AIRFLOW_HOME environment variable in line 21


COPY script/bootstrap.sh /bootstrap.sh
COPY script/systemlibs.sh /systemlibs.sh
Expand Down
3 changes: 2 additions & 1 deletion docker/script/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ package_requirements() {
if [[ -e "$AIRFLOW_HOME/$REQUIREMENTS_FILE" ]]; then
echo "Packaging requirements.txt into plugins"
pip3 download -r "$AIRFLOW_HOME/$REQUIREMENTS_FILE" -d "$AIRFLOW_HOME/plugins"
wget "https://raw.githubusercontent.com/apache/airflow/constraints-${AIRFLOW_VERSION}/constraints-${PYTHON_VERSION%.*}.txt" -O $AIRFLOW_HOME/plugins/constraints.txt
Copy link
Contributor

@mayushko26 mayushko26 Mar 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you compare the local runner images' constraints file against the one published by Apache Airflow, you will notice there are a few differences for some versions. The source of truth should be the constraints.txt file which exists in this repository. Additionally; users can make changes to this constraints file to fit their use-cases. You can copy it into the plugins directory.

I am also wondering if it's better to place the constraints file in the plugins directory in the package-requirements() function instead of being a step in the startup script. This will ensure that the constraint file will be populated if you delete the plugins directory files before executing the package command.

Copy link

@charlielu05 charlielu05 Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified to fetch the constraints from aws-mwaa-local-runner instead of airflow.
Also for consistency, would it be wise to also switch out the constraint file in requirements/requirements.txt since it is also currently pointing to the apache airflow constraint instead of this repo?
--constraint "https://raw.githubusercontent.com/apache/airflow/constraints-2.7.2/constraints-3.11.txt"

On your last point, do you mean to populate the constraint in that startup script instead of this package-requirements function?

cd "$AIRFLOW_HOME/plugins"
zip "$AIRFLOW_HOME/requirements/plugins.zip" *
printf '%s\n%s\n' "--no-index" "$(cat $AIRFLOW_HOME/$REQUIREMENTS_FILE)" > "$AIRFLOW_HOME/requirements/packaged_requirements.txt"
printf '%s\n%s\n' "--find-links /usr/local/airflow/plugins" "$(cat $AIRFLOW_HOME/requirements/packaged_requirements.txt)" > "$AIRFLOW_HOME/requirements/packaged_requirements.txt"
printf '%s\n%s\n%s\n%s\n' "--find-links /usr/local/airflow/plugins" "--constraint /usr/local/airflow/plugins/constraints.txt" "$(cat $AIRFLOW_HOME/requirements/packaged_requirements.txt | grep -v '^--constraint .https://*')" > "$AIRFLOW_HOME/requirements/packaged_requirements.txt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could loosen the grep to just ^--constraint. There are use-cases where a customer needs to customize the constraints file, so they will use the one present in the config/constraints.txt, instead of the one published by Apache Airflow.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied this change.

fi
}

Expand Down
Loading