Skip to content

Fix RTIF override logic when should_store_compiled_sql=False to restore pre-refactor behaviour#1777

Merged
pankajkoti merged 2 commits into
mainfrom
fix-pr-1661
May 27, 2025
Merged

Fix RTIF override logic when should_store_compiled_sql=False to restore pre-refactor behaviour#1777
pankajkoti merged 2 commits into
mainfrom
fix-pr-1661

Conversation

@pankajkoti
Copy link
Copy Markdown
Contributor

PR #1661 introduced a breaking change for standalone Cosmos operators when users set should_store_compiled_sql=False, as detailed in issue #1776.

The issue stems from a refactor in PR #1661. Previously, the RTIF refresh calls to the database were skipped because they occurred within the same function that checked the flag. After the refactor, these operations were moved to a separate _override_rtif function, but the should_store_compiled_sql condition was not checked there. As a result, the operator behaviour broke, as observed in the issue.

To restore the original behaviour, this change adds a condition in the override_rtif function to exit early when should_store_compiled_sql is False, ensuring consistency with the pre-refactor logic.

Closes: #1776

Copilot AI review requested due to automatic review settings May 23, 2025 08:12
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 23, 2025
@netlify
Copy link
Copy Markdown

netlify Bot commented May 23, 2025

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 2c5b6dd
🔍 Latest deploy log https://app.netlify.com/projects/sunny-pastelito-5ecb04/deploys/68304cd6faf6960008ded733

@dosubot dosubot Bot added area:config Related to configuration, like YAML files, environment variables, or executer configuration area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc labels May 23, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR restores the original behavior by skipping RTIF overrides when should_store_compiled_sql=False, matching the pre-refactor logic.

  • Added an early‐exit guard in _override_rtif to respect should_store_compiled_sql
  • Introduced tests for Airflow 2 and Airflow 3 scenarios when storage is disabled

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cosmos/operators/local.py Added an early return in _override_rtif when storage flag is false
tests/operators/test_local.py Added tests verifying no RTIF override for Airflow 2/3 if storage is disabled
Comments suppressed due to low confidence (1)

tests/operators/test_local.py:1943

  • [nitpick] Consider patching operator._override_rtif_airflow_3_x and asserting it is not called, mirroring the Airflow 2 test to fully validate that no override is invoked when storage is disabled.
def test_override_rtif_airflow3_with_should_store_compiled_sql_false():

Comment thread cosmos/operators/local.py
@pankajkoti pankajkoti marked this pull request as draft May 23, 2025 08:15
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 23, 2025

Deploying astronomer-cosmos with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2c5b6dd
Status: ✅  Deploy successful!
Preview URL: https://4905f7df.astronomer-cosmos.pages.dev
Branch Preview URL: https://fix-pr-1661.astronomer-cosmos.pages.dev

View logs

@pankajkoti pankajkoti marked this pull request as ready for review May 23, 2025 12:59
@dosubot dosubot Bot added the execution:local Related to Local execution environment label May 23, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.03%. Comparing base (62e9b40) to head (2c5b6dd).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1777   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files          85       85           
  Lines        5247     5249    +2     
=======================================
+ Hits         5144     5146    +2     
  Misses        103      103           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

Excellent work at supporting the community and fixing this so quickly, @pankajkoti ! Did @tuantran0910 have a chance to confirm it solved the original issue?

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label May 27, 2025
@pankajkoti
Copy link
Copy Markdown
Contributor Author

Did @tuantran0910 have a chance to confirm it solved the original issue?

Yes, @tuantran0910 confirmed that the fix worked for them in https://apache-airflow.slack.com/archives/C059CC42E9W/p1748014917886709?thread_ts=1747031958.683949&cid=C059CC42E9W

@pankajkoti pankajkoti merged commit 2404021 into main May 27, 2025
151 of 181 checks passed
@pankajkoti pankajkoti deleted the fix-pr-1661 branch May 27, 2025 09:53
@pankajkoti pankajkoti added this to the Cosmos 1.10.2 milestone Aug 7, 2025
@tatiana tatiana mentioned this pull request Aug 7, 2025
tatiana pushed a commit that referenced this pull request Aug 7, 2025
…re pre-refactor behaviour (#1777)

PR #1661 introduced a breaking change for standalone Cosmos operators
when users set `should_store_compiled_sql=False`, as detailed in issue
#1776.

The issue stems from a refactor in PR #1661. Previously, the RTIF
refresh calls to the database were skipped because they occurred within
the same function that checked the flag. After the refactor, these
operations were moved to a separate `_override_rtif` function, but the
`should_store_compiled_sql` condition was not checked there. As a
result, the operator behaviour broke, as observed in the issue.

To restore the original behaviour, this change adds a condition in the
override_rtif function to exit early when should_store_compiled_sql is
False, ensuring consistency with the pre-refactor logic.

Closes: #1776
(cherry picked from commit 2404021)
tatiana added a commit that referenced this pull request Aug 8, 2025
**Bug Fixes**

* Fix task instance ``try_number`` attribute for Airflow 3 compatibility
by @pankajkoti in #1781
* Fix rendered template override logic when
``should_store_compiled_sql=False`` to restore pre-refactor behaviour by
@pankajkoti in #1777
* Fix ``ProfileConfig`` in GCP Cloud Run job execution mode by
@ramonvermeulen in #1783
* Fix dbt Docs page height by @1cadumagalhaes in #1793
* Add support to base64 encoded pem in Snowflake profiles by
@brunocmartins in #1801
* Allow to disable owner inheritance from dbt into airflow DAG owners by
@CorsettiS in #1787
* Fix Kubernetes Pod Operator conversion of ``container_resources`` to
``resources`` by @johnhoran in #1821
* Fix ``dbt deps`` with project level variables by @AlexandrKhabarov in
#1822
* Fix source freshness warnings in kubernetes execution mode by
@Pawel-Drabczyk in #1859
* Fix: Harden DbtNode against null config/meta by @pankajkoti in #1877
* Fix cache behaviour when DAG name contains "." by @tatiana in #1908

**Documentation**

* Fix ``contributing.rst`` docs by @tatiana in #1785
* Fix docs rendering in Airflow 3 Compatibility by @pankajastro in #1790
* Fix typo in ``selecting-excluding.rst`` by @msshroff in #1814
* Update testing behavior file with ``ExecutionMode.KUBERNETES`` by
@LuigiCerone in #1813
* Add step to fork repo in contributing guide by @pankajastro in #1808
* Fix ``depends_on`` attribute by @benedikt-buchert in #1837
* Fix character name by @ThePsyjo in #1860
* Update suggested MWAA startup script by @jaklan in #1884
* Make implementation & docs consistent regarding
``use_dataset_airflow3_uri_standard`` by @Anti0ff in #1878

**Others**

* Set retries to 0 in example DAGs by @pankajkoti in #1782
* Fix ``test_async_example_dag_without_setup_task`` tests by
@pankajastro in #1788
* Fix test hash value for Darwin when using Py 3.12.10 by @tatiana in
#1786
* Upgrade Python and Airflow used to run MyPy checks by @tatiana in
#1796
* Assert example DAGs' ``DagRunState`` and fix issues by @pankajkoti and
@tatiana in #1778
* Update the conflict matrix to include AF 2.10, 2.11 & 3.0 and dbt 1.9
& 1.10 by @tatiana in #1820
* Fix broken CI due to Pydantic conflicts by @tatiana in #1809
* Drop Python 3.8 Support by @pankajastro in #1852
* Add Airflow 2.11 to the test matrix by @tatiana in #1807
* Require Authorize for all jobs on pull requests from external
contributors in CI by @pankajkoti in #1861
* Leverage Trusted Publisher Management when publishing PyPI package by
@tatiana in #1862
* CI: Add back accidentally deleted python-version matrix for running
unit tests by @pankajkoti in #1872
* Remove commented code and fix mypy failures by @pankajkoti in #1876
* Add Zizmor analysis GitHub action by @pankajkoti in #1870
* Catch FlushError on Datasets for Airflow 2.11 dags test by @pankajkoti
in #1880
* Add deprecation warning for ``LoadMode.CUSTOM`` parser by
@duongphannamhung in #1885
* CI: Add GitHub CodeQL analysis workflow (codeql.yml) by @pankajkoti in
#1871
* Resolve 'credential persistence through GitHub Actions artifacts'
warnings from Zizmor by @pankajkoti in #1890
* Resolve 'overly broad permissions' warnings from Zizmor by @pankajkoti
in #1889
* Resolve Zizmor error alerts for unpinned image references; mark alert
for pull_request_target ignored by @pankajkoti in #1888
* Fix broken CI ``tests.py3.11-2.8-1.9:test-integration-setup`` by
@tatiana in #1902
* Add dbt-core 1.10 to test matrix by @tatiana in #1767
* Pin package dbt-databricks by @pankajastro in #1909
* Enable matrix test entry for dbt-1.9, python-3.9 and airflow-3.0 tests
in CI by @pankajastro in #1900
* Pre-commit updates: #1779, #1795, #1800, #1857, #1863, #1869, #1892,
#1901
* Dependabot updates: #1904

Co-authored-by: Pankaj Koti <pankajkoti699@gmail.com>
@pankajkoti pankajkoti mentioned this pull request Aug 8, 2025
tatiana added a commit that referenced this pull request Aug 8, 2025
**Bug Fixes**

* Fix task instance ``try_number`` attribute for Airflow 3 compatibility
by @pankajkoti in #1781
* Fix rendered template override logic when
``should_store_compiled_sql=False`` to restore pre-refactor behaviour by
@pankajkoti in #1777
* Fix ``ProfileConfig`` in GCP Cloud Run job execution mode by
@ramonvermeulen in #1783
* Fix dbt Docs page height by @1cadumagalhaes in #1793
* Add support to base64 encoded pem in Snowflake profiles by
@brunocmartins in #1801
* Allow to disable owner inheritance from dbt into airflow DAG owners by
@CorsettiS in #1787
* Fix Kubernetes Pod Operator conversion of ``container_resources`` to
``resources`` by @johnhoran in #1821
* Fix ``dbt deps`` with project level variables by @AlexandrKhabarov in
#1822
* Fix source freshness warnings in kubernetes execution mode by
@Pawel-Drabczyk in #1859
* Fix: Harden DbtNode against null config/meta by @pankajkoti in #1877
* Fix cache behaviour when DAG name contains "." by @tatiana in #1908

**Documentation**

* Fix ``contributing.rst`` docs by @tatiana in #1785
* Fix docs rendering in Airflow 3 Compatibility by @pankajastro in #1790
* Fix typo in ``selecting-excluding.rst`` by @msshroff in #1814
* Update testing behavior file with ``ExecutionMode.KUBERNETES`` by
@LuigiCerone in #1813
* Add step to fork repo in contributing guide by @pankajastro in #1808
* Fix ``depends_on`` attribute by @benedikt-buchert in #1837
* Fix character name by @ThePsyjo in #1860
* Update suggested MWAA startup script by @jaklan in #1884
* Make implementation & docs consistent regarding
``use_dataset_airflow3_uri_standard`` by @Anti0ff in #1878

**Others**

* Set retries to 0 in example DAGs by @pankajkoti in #1782
* Fix ``test_async_example_dag_without_setup_task`` tests by
@pankajastro in #1788
* Fix test hash value for Darwin when using Py 3.12.10 by @tatiana in
#1786
* Upgrade Python and Airflow used to run MyPy checks by @tatiana in
#1796
* Assert example DAGs' ``DagRunState`` and fix issues by @pankajkoti and
@tatiana in #1778
* Update the conflict matrix to include AF 2.10, 2.11 & 3.0 and dbt 1.9
& 1.10 by @tatiana in #1820
* Fix broken CI due to Pydantic conflicts by @tatiana in #1809
* Drop Python 3.8 Support by @pankajastro in #1852
* Add Airflow 2.11 to the test matrix by @tatiana in #1807
* Require Authorize for all jobs on pull requests from external
contributors in CI by @pankajkoti in #1861
* Leverage Trusted Publisher Management when publishing PyPI package by
@tatiana in #1862
* CI: Add back accidentally deleted python-version matrix for running
unit tests by @pankajkoti in #1872
* Remove commented code and fix mypy failures by @pankajkoti in #1876
* Add Zizmor analysis GitHub action by @pankajkoti in #1870
* Catch FlushError on Datasets for Airflow 2.11 dags test by @pankajkoti
in #1880
* Add deprecation warning for ``LoadMode.CUSTOM`` parser by
@duongphannamhung in #1885
* CI: Add GitHub CodeQL analysis workflow (codeql.yml) by @pankajkoti in
#1871
* Resolve 'credential persistence through GitHub Actions artifacts'
warnings from Zizmor by @pankajkoti in #1890
* Resolve 'overly broad permissions' warnings from Zizmor by @pankajkoti
in #1889
* Resolve Zizmor error alerts for unpinned image references; mark alert
for pull_request_target ignored by @pankajkoti in #1888
* Fix broken CI ``tests.py3.11-2.8-1.9:test-integration-setup`` by
@tatiana in #1902
* Add dbt-core 1.10 to test matrix by @tatiana in #1767
* Pin package dbt-databricks by @pankajastro in #1909
* Enable matrix test entry for dbt-1.9, python-3.9 and airflow-3.0 tests
in CI by @pankajastro in #1900
* Pre-commit updates: #1779, #1795, #1800, #1857, #1863, #1869, #1892,
#1901
* Dependabot updates: #1904

Co-authored-by: Tatiana Al-Chueyr <tatiana.alchueyr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:config Related to configuration, like YAML files, environment variables, or executer configuration area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc execution:local Related to Local execution environment lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] AttributeError: '_PythonDecoratedOperator' object has no attribute 'env when using Docs operators

3 participants