Adjust test for Airflow-2.9 #2149

pankajastro · 2024-05-03T09:36:26Z

pin google-cloud-bigquery
Remove conditional query building for different Airflow version
Adjust dataset and cleanoperator test
clean checkoperator test

pankajastro · 2024-05-03T13:04:12Z

python-sdk/tests/sql/operators/test_cleanup.py

    [
        (SequentialExecutor(), "LocalExecutor", True),
        (LocalExecutor(), "LocalExecutor", False),
-        (None, "LocalExecutor", False),
-        (None, "SequentialExecutor", True),
+        (LocalExecutor(), "LocalExecutor", False),


In case of None default executor getting assigned

I think line 111 and 112 are duplicate.

Do we know which executor was getting used when None was passed?

I looked a bit deeper into the issue and discovered that it's not actually the default value causing the problem, but rather caching.

We eventually call https://github.com/apache/airflow/blob/main/airflow/executors/executor_loader.py#L90 when we are doing job.executor in _get_executor_from_job_id which first time load the value from next call it returns value from https://github.com/apache/airflow/blob/main/airflow/executors/executor_loader.py#L98 in case we have none value for executor_in_job. This behavior leads to the last test consistently failing. For instance,
if test sequence is

(None, "LocalExecutor", False), (None, "SequentialExecutor", True),

(None, "SequentialExecutor", True) fail

for sequence

(None, "SequentialExecutor", True), (None, "LocalExecutor", False),

(None, "LocalExecutor", False) fail

Update the code PTAL commit: e3d3239

pankajastro · 2024-05-07T15:30:49Z

pre-commit failure is looking like because of recent pre-commit hook update will handle that in separate PR

pankajastro · 2024-05-07T15:32:10Z

python-sdk/pyproject.toml

@@ -57,7 +57,8 @@ google = [
    "protobuf",
    "apache-airflow-providers-google>=10.15.0",
    "sqlalchemy-bigquery>=1.3.0",
-    "smart-open[gcs]>=5.2.1,<7.0.0"
+    "smart-open[gcs]>=5.2.1,<7.0.0",
+    "google-cloud-bigquery<3.21.0"


looks like recent optimization in biquery causing 404 error for bigquery job. will create an issue with finding

maybe PR googleapis/python-bigquery#1900 but need bit more debugging

codecov · 2024-05-07T15:36:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.12%. Comparing base (162b0ab) to head (aea0ff6).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2149      +/-   ##
==========================================
+ Coverage   89.75%   91.12%   +1.37%     
==========================================
  Files          75       75              
  Lines        4322     4316       -6     
  Branches      539      538       -1     
==========================================
+ Hits         3879     3933      +54     
+ Misses        343      287      -56     
+ Partials      100       96       -4

Flag	Coverage Δ
PythonSDK	`91.12% <100.00%> (+1.37%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

utkarsharma2 · 2024-05-08T09:31:47Z

python-sdk/tests/sql/operators/test_cleanup.py

@@ -177,8 +177,8 @@ def test_single_worker_mode_backfill_airflow_2_5(executor_in_job, executor_in_cf
    [
        (SequentialExecutor(), "LocalExecutor", True),
        (LocalExecutor(), "LocalExecutor", False),
-        (None, "LocalExecutor", False),
-        (None, "SequentialExecutor", True),
+        (LocalExecutor(), "LocalExecutor", False),


Duplicate tests, line 179 and 180.

utkarsharma2 · 2024-05-08T09:33:54Z

python-sdk/tests_integration/sql/operators/data_validation/test_check_table.py

@@ -22,7 +21,6 @@
        {
            "database": Database.BIGQUERY,
            "file": File(path=str(CWD) + "/../../../data/homes_main.csv"),
-            "table": Table(conn_id="gcp_conn_project"),


Why we don't need it now?

This connection was only utilized in these two tests, which appear to be unnecessary. However, I'm unsure why we included it.

pankajastro · 2024-05-09T11:43:18Z

python-sdk/src/astro/sql/operators/cleanup.py

-        if job.executor_class is None and job.executor:
-            return type(job.executor).__name__


Using job.executor in this context appears incorrect because it will not only retrieve the executor from the job but also consider the executor conf.

I think we should update our test case instead.

kaxil · 2024-05-14T09:11:18Z

Why this in review state since 5 days?

pankajastro · 2024-05-14T11:14:13Z

Why this in review state since 5 days?

Will connect with the codeowners to conclude on this.

pankajastro · 2024-05-16T10:17:11Z

some tests are failing because of Databricks related infra change

pankajastro · 2024-05-16T12:14:44Z

some tests are failing because of Databricks related infra change

Fixed in PR #2161 and #2159

pankajastro force-pushed the fix-af-2.9-test branch 4 times, most recently from 87a8dbb to 650957f Compare May 3, 2024 12:37

pankajastro changed the title ~~Fix AF-2.9 Test~~ Adjust test for Airflow-2.9 May 3, 2024

pankajastro commented May 3, 2024

View reviewed changes

pankajastro force-pushed the fix-af-2.9-test branch from dbf2b57 to d55914d Compare May 3, 2024 14:06

pankajastro mentioned this pull request May 6, 2024

Remove condition import for SQLAlcRow #2146

Merged

pankajastro force-pushed the fix-af-2.9-test branch from d55914d to 75ef5f4 Compare May 6, 2024 20:18

pankajastro marked this pull request as ready for review May 6, 2024 20:18

pankajastro requested review from dimberman, tatiana, utkarsharma2 and sunank200 as code owners May 6, 2024 20:18

pankajastro force-pushed the fix-af-2.9-test branch 4 times, most recently from 58fe573 to 2bf58cc Compare May 7, 2024 08:59

pankajastro commented May 7, 2024

View reviewed changes

This was referenced May 7, 2024

Add Python3.12 in CI config #2154

Closed

Fix pre-commit: UP031 Use format specifiers instead of percent format #2155

Closed

Unpin google-cloud-bigquery in astro-sdk #2156

Open

utkarsharma2 reviewed May 8, 2024

View reviewed changes

pankajastro force-pushed the fix-af-2.9-test branch from 23f9e5d to 2331859 Compare May 8, 2024 19:57

pankajastro commented May 9, 2024

View reviewed changes

pankajastro requested a review from utkarsharma2 May 9, 2024 11:43

pankajastro force-pushed the fix-af-2.9-test branch from 30fd161 to 84c7f7c Compare May 16, 2024 10:45

pankajastro added 8 commits May 16, 2024 17:14

Adjust test for Airflow-2.9

94f2b98

Fix

4b33486

Fix

922283d

Pin bigquery client

f63e99d

Fix tests

6c7fff9

cleanup

47335eb

Fix tests

005937a

Mock _executor_names

aea0ff6

pankajastro force-pushed the fix-af-2.9-test branch from 84c7f7c to aea0ff6 Compare May 16, 2024 11:44

utkarsharma2 approved these changes May 16, 2024

View reviewed changes

pankajastro merged commit 149dd9e into main May 16, 2024
31 of 33 checks passed

pankajastro deleted the fix-af-2.9-test branch May 16, 2024 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust test for Airflow-2.9 #2149

Adjust test for Airflow-2.9 #2149

pankajastro commented May 3, 2024 •

edited

Loading

pankajastro May 3, 2024

utkarsharma2 May 8, 2024

utkarsharma2 May 8, 2024

pankajastro May 8, 2024

pankajastro May 9, 2024

pankajastro commented May 7, 2024

pankajastro May 7, 2024

pankajastro May 7, 2024

codecov bot commented May 7, 2024 •

edited

Loading

utkarsharma2 May 8, 2024

utkarsharma2 May 8, 2024

pankajastro May 8, 2024

pankajastro May 9, 2024

utkarsharma2 May 16, 2024

kaxil commented May 14, 2024

pankajastro commented May 14, 2024

pankajastro commented May 16, 2024

pankajastro commented May 16, 2024

		if job.executor_class is None and job.executor:
		return type(job.executor).__name__

Adjust test for Airflow-2.9 #2149

Adjust test for Airflow-2.9 #2149

Conversation

pankajastro commented May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pankajastro commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxil commented May 14, 2024

pankajastro commented May 14, 2024

pankajastro commented May 16, 2024

pankajastro commented May 16, 2024

pankajastro commented May 3, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading