Change type of virtualenv_dir argument from path to string#1721
Change type of virtualenv_dir argument from path to string#1721pankajastro wants to merge 10 commits into
Conversation
✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.
|
Deploying astronomer-cosmos with
|
| Latest commit: |
f7b12fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0c16f483.astronomer-cosmos.pages.dev |
| Branch Preview URL: | https://fix-ve-example.astronomer-cosmos.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1721 +/- ##
==========================================
- Coverage 97.50% 97.48% -0.02%
==========================================
Files 83 83
Lines 5049 5051 +2
==========================================
+ Hits 4923 4924 +1
- Misses 126 127 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pankajkoti
changed the title
| pip_install_options: list[str] | None = None, | ||
| py_system_site_packages: bool = False, | ||
| virtualenv_dir: Path | None = None, | ||
| virtualenv_dir: str | None = None, |
There was a problem hiding this comment.
@pankajastro this could be seen as a breaking change - what motivated it?
There was a problem hiding this comment.
I pushed that change @tatiana in 1f1947c. We had a discussion regarding this in the other PR at #1718 (comment).
Apparently Airflow 3 is evaluating & validating that the Path exists as part of its rending as mentioned by Pankaj in the other PR's description.
There was a problem hiding this comment.
@pankajkoti I just read the thread - would it be possible for this to remain a Path and for us to convert to a string if Airflow 3, as part of the operator implementation? In the worst case, we could support this field being both str and path and handling it in the operator. It would be better if we didn't introduce a breaking change.
There was a problem hiding this comment.
we need to try out though, when does the rendering evaluate and if we have control over it(meaning if the rendering happens even before the operator initializes/executes and if yes, if there's a method to override the type for it).
There was a problem hiding this comment.
will try it out later today to dig into this.
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.6 → v0.11.7](astral-sh/ruff-pre-commit@v0.11.6...v0.11.7) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
The PR adds the following files to `.gitignore` 1. `mock-venv`: generated when running cosmos unit tests locally, e.g. `hatch run tests.py3.11-3.0-1.9:test-cov` 2. `simple_auth_manager_passwords.json.generated`: Airflow 3 standalone generated password file
…xes (#1719) While the initial x.x.0 versions of dbt adapters may have compatibility issues with certain dependencies, I observed that these incompatibilities are progressively addressed in later patch releases. The issue reported in #1709 appears to stem from dbt-bigquery accessing a protected member `google.cloud.bigquery._helpers._CELLDATA_FROM_JSON`, which was removed in `google-cloud-bigquery 3.31.0`. This access I believe was introduced in [dbt-bigquery#974](dbt-labs/dbt-bigquery#974) and later removed in [dbt-bigquery#1061](dbt-labs/dbt-bigquery#1061), with the fix subsequently back-ported to the 1.7 series via [dbt-bigquery#1074](dbt-labs/dbt-bigquery#1074). Therefore, I believe that relying on the latest patch versions is a better approach to avoid such issues, rather than individually resolving discrepancies. This PR aligns with that strategy. closes: #1709 --------- Co-authored-by: Tatiana Al-Chueyr <tatiana.alchueyr@gmail.com>
During some experimentation with [callbacks](https://astronomer.github.io/astronomer-cosmos/configuration/callbacks.html#example-using-callbacks-with-a-single-operator) I have noticed that we can only use a single callback per time, which can be very limiting if I want to add custom callbacks together with the default ones provided by cosmos. This PR is very simple and aims to allow multiple callbacks to be passed as a list of functions, that will then be executed sequentially.
Cosmos 1.9.0 introduced a change such that `AbstractDbtBase` no longer inherits from airflow `BaseOperator` and I believe this introduced two bugs. - In the past you could give arguments using airflows `default_args` and these would end up being passed to cosmos. So for example we were specifying the `project_dir` as a default arg, and in 1.9.0 these projects were broken because AbstractDbtBase was not longer getting a project_dir specified. So the first change is to check both kwargs and default_args for the values AbstractDbtBase requires. ~I've only made this change for the kubernetes operator, but I'd argue it should be done for the local a docker ones too.~ - The existing code tries to inspect `KubernetesPodOperator` for the args that it requires, but notably this approach misses arguments that are required by `BaseOperator`, e.g. the task_group. - ~I've made an effort to restore the convention in airflow that all arguments must be consumed by the operator. So an argument must either be consumed by `AbstractDbtBase` or it will be passed along to the operator, it may also be consumed by both classes.~
During some experimentation with [callbacks](https://astronomer.github.io/astronomer-cosmos/configuration/callbacks.html#example-using-callbacks-with-a-single-operator) I have noticed that we can only use a single callback per time, which can be very limiting if I want to add custom callbacks together with the default ones provided by cosmos. This PR is very simple and aims to allow multiple callbacks to be passed as a list of functions, that will then be executed sequentially.
|
After rebasing, this PR shows a lot of files changed although the changes are already in main and would make it difficult to review. I created a fresh PR to reflect only the changes being made #1724 Hence, I am closing this PR. |
6 participants
closes: #1707