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.

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

[CI] Reduce the amount taking to run tests in the CI from 5h to 11min #1297

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

tatiana
Copy link
Collaborator

@tatiana tatiana commented Oct 31, 2024

Context

Closes: #1299

Some CI jobs took an outrageous amount of time to run.

One of our CI test pipeline jobs was taking over five hours to run:

Screenshot 2024-11-05 at 14 52 58

https://github.com/astronomer/astronomer-cosmos/actions/runs/11505558596

More recently, this same job started taking over 6 hours to run and started timing out in the CI, making Cosmos' main branch red for both unit and integration tests for Airflow 2.6. The underlying reason is that it took a long time to resolve dependencies. This seems to have happened since October 29, as seen on the commit 84c5fbd to main.

Example: https://github.com/astronomer/astronomer-cosmos/actions/runs/11594745046/job/32330864858

About this change

This PR solves the original issue by changing where and how we install Airflow dependencies, simplifying the previous setup. We manage Airflow test dependencies in the pre-install-airflow.sh file, not in pyproject.toml, other sh, or the Github action. We are also being strict as we can dependent on the Airflow version. Where possible, we use constraints. Where different providers' dependencies conflict with previous versions of Airflow, we just ensure the expected version remains being used after the installation.

Example of a successful run:

Screenshot 2024-11-05 at 14 52 48

https://github.com/astronomer/astronomer-cosmos/actions/runs/11685652312

Bonus

I realised with this change that users who use K8s and want to define different paths for their dbt projects in Airflow and K8s were facing an issue. This problem was evident when running the k8s example DAG. I've fixed the problem as part of this PR.

Follow-up actions

Since this has been taking a long time to solve and our main branch is still red, I commented out two tasks that were failing tests - and I've logged a follow-up issue for us to address this:

#1304

Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit d471ec2
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/6728e685f83aae0008a14084

The CI is failing for both unit and integraiton tests for Airflow 2.6 because it is taking a long time to try to resolve dependencies.

Example: https://github.com/astronomer/astronomer-cosmos/actions/runs/11594745046/job/32330864858

This PR aims to resolve the issue.
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.84%. Comparing base (342ce3a) to head (43338c3).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1297      +/-   ##
==========================================
+ Coverage   95.73%   95.84%   +0.10%     
==========================================
  Files          67       67              
  Lines        3967     3973       +6     
==========================================
+ Hits         3798     3808      +10     
+ Misses        169      165       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tatiana tatiana marked this pull request as ready for review October 31, 2024 16:26
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. area:ci Related to CI, Github Actions, or other continuous integration tools area:testing Related to testing, like unit tests, integration tests, etc labels Oct 31, 2024
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Oct 31, 2024
@tatiana tatiana marked this pull request as draft October 31, 2024 16:52
Copy link
Contributor

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

These are amazing fixes and great manoeuvring. Certainly this has a massive operational impact & contributors are going to the love the speedy CI now. Kudos to you @tatiana for being so patient with the acting CI, not giving up & taking it to completion 👏🏽

@@ -2,7 +2,7 @@ name: test

on:
push: # Run on pushes to the default branch
branches: [main]
branches: [main, fix-ci-for-tests.py3.8-2.6]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
branches: [main, fix-ci-for-tests.py3.8-2.6]
branches: [main]

gentle reminder comment to remove this before merging

cosmos/converter.py Show resolved Hide resolved
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 5, 2024
@tatiana tatiana marked this pull request as ready for review November 5, 2024 14:49
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Nov 5, 2024
@dosubot dosubot bot added the area:performance Related to performance, like memory usage, CPU usage, speed, etc label Nov 5, 2024
@tatiana tatiana changed the title Fix unit and integration tests for Python 3.8 & Airflow 2.6 [CI] Reduce the amount taking to run tests in the CI from 5h to 11min Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ci Related to CI, Github Actions, or other continuous integration tools area:performance Related to performance, like memory usage, CPU usage, speed, etc area:testing Related to testing, like unit tests, integration tests, etc lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Reduce the amount taking to run tests in the CI
2 participants