Bumps the CI test matrix to the latest supported Airflow and dbt releases, fixes the real compatibility breaks that surfaced along the way, and starts reducing the amount of dbt CLI surface we hardcode by hand. - #182
millin wants to merge 14 commits into
Conversation
f2546c1 to
d0b9651
Compare
FreshnessTask.__init__ started requiring a catalogs argument in dbt-core 1.12, unlike its sibling ConfiguredTask subclasses which default it to None.
Airflow <3.3 (fixed upstream by apache/airflow#68840) schedules the FastAPI execution API app's lifespan startup via asyncio.run_coroutine_threadsafe without waiting for it, so a dag.test() task run can race ahead of app.state.svcs_registry being set and fail with AttributeError. We patch in_process_api_server() to hand out a pre-warmed transport that waits for lifespan startup, mirroring the upstream fix, since InProcessExecutionAPI is an attrs-slotted class we can't otherwise safely monkeypatch.
…ally BaseConfig hardcoded every dbt project-level behavior change flag as a dataclass field, which broke every time dbt added a new one (most recently require_source_and_semantic_model_names_without_spaces in 1.12) since dbt's own Flags object only sets the UPPERCASE attribute for flags absent from our config's vars(), while some dbt code paths read the lowercase one and raise AttributeError. Instead, read the current flag set from dbt.contracts.project.ProjectFlags.project_only_flags at runtime, so new dbt releases work without a change here. Also add an extra_flags escape hatch (mirroring the existing vars field) so any dbt flag or config option we don't otherwise model can still be passed through from the operator.
dbt-core <1.12 pins mashumaro<3.15, which doesn't support Python 3.14 (added only in mashumaro 3.17). Those combinations can't install and would just fail on every run. Also drops a stale beta-era comment now that dbt 1.12 has a stable release.
dbt's own CLI merges --flag/--no-flag into a single boolean option; the separate no_<flag> counterpart we've historically accepted was never something dbt itself needed, only kept for backwards compatibility. Warn callers so we can eventually drop it and shrink this parameter surface.
DagBag dropped the include_examples parameter in Airflow 3.3, breaking every matrix combination using that version. Add AIRFLOW_V_3_3_PLUS and only pass it on older Airflow. Also fixes two mypy errors that surfaced once the previously-broken mypy job condition started actually running mypy again: a possibly None connection type passed where dict.get expects str, and a conditional-import fallback assigning an incompatible type. While at it, point the mypy job condition at Python 3.14, the latest version in the matrix.
Operators previously forwarded their entire __dict__ (vars(self)) to the dbt hook, which happened to work but meant any dbt CLI option not already modeled as a named parameter was silently dropped, and mixed in unrelated Airflow-internal attributes (task_id, dag, retries, ...) that just got filtered out downstream by field-name matching. Add a config_kwargs dict parameter to DbtBaseOperator: every operator now folds its own __init__ parameters into this single dict via a new _update_config_kwargs() helper, and execute() sends this (not vars(self)) to the hook. This is now the sole vehicle for reaching dbt, and named parameters without special handling (Jinja templating, or read elsewhere in the class) no longer get a redundant self.attr of their own. ConfigFactory.create_config() now routes any kwarg that doesn't match one of the target Config's fields into its extra_flags field instead of dropping it, so a dbt parameter we haven't modeled explicitly (e.g. a newer dbt release's addition) still reaches dbt. Updates 14 tests that called get_dbt_task_config(**vars(op)) directly to use **op.config_kwargs instead, matching what execute() now does.
d0b9651 to
d033b46
Compare
|
Hi, @tomasfarias |
|
Hey @millin. Thanks for the PR. I am travelling at the moment, so will take a look tomorrow. Seeing a pretty large diff already. Maybe you could split this one up to make it easy to review? |
|
Hi, @tomasfarias! I hope you had a good trip. You can review commits one by one. Regarding flags, I would like to remove the most of non-mandatories in the future as their list is gradually changing. |
|
Hi @millin. Thank you, I am now back.
The issue is that the longer a PR gets, the more context I have to keep in my head at a time, as I am not approving one commit at a time: I approve (or reject) the whole PR. Breaking up changes means faster reviews and approvals. For example: I could have already approved, merged, and released the more mechanical python 3.14 bump if it were in a separate PR, before even looking at all the other changes which do involve more effort. Now, that's stuck waiting for me to review everything else. Anyways, just keep this in mind for the future please. For now, I'll bite the bullet and get this reviewed now. |
tomasfarias
left a comment
tomasfarias
left a comment
There was a problem hiding this comment.
I appreciate the overall idea of reducing hardcoded arguments, but I have some thoughts about some of the changes.
I may cherry-pick the python 3.14 stuff to a different PR as I really think that's really cool and I have no comments.
| Every `no_<flag>` parameter below is deprecated: dbt's own CLI merges | ||
| `--flag/--no-flag` into a single boolean, so pass `<flag>=False` instead | ||
| of `no_<flag>=True`. They are kept only for backwards compatibility and | ||
| will be removed in a future release. |
There was a problem hiding this comment.
question(blocking): What's the motivation for the deprecation? I am not sure I necessarily agree with it: If the CLI still takes the --no-flag variant, I'd still want to support that to make the transition between CLI to Airlfow seamless.
If we do go forward with it, rather than documenting this in a docstring, we should raise a deprecation warning if any of the no- flags are used.
| Most parameters below are only ever consumed through self.config_kwargs | ||
| (see its own description further down) and no longer get a dedicated | ||
| self.attr of their own; project_dir, profiles_dir, profile, target, | ||
| state, vars, env_vars, dbt_conn_id, profiles_conn_id, project_conn_id, | ||
| and do_xcom_push_artifacts are the exception, kept as real attributes | ||
| since they're either Jinja-templated (see template_fields) or read | ||
| elsewhere in this class (the dbt_hook property, execute()). |
There was a problem hiding this comment.
nit: I don't think there is any need to document what is "no longer" the case:
| Most parameters below are only ever consumed through self.config_kwargs | |
| (see its own description further down) and no longer get a dedicated | |
| self.attr of their own; project_dir, profiles_dir, profile, target, | |
| state, vars, env_vars, dbt_conn_id, profiles_conn_id, project_conn_id, | |
| and do_xcom_push_artifacts are the exception, kept as real attributes | |
| since they're either Jinja-templated (see template_fields) or read | |
| elsewhere in this class (the dbt_hook property, execute()). | |
| Most parameters below are only ever consumed through self.config_kwargs. | |
| project_dir, profiles_dir, profile, target, state, vars, env_vars, | |
| dbt_conn_id, profiles_conn_id, project_conn_id, and | |
| do_xcom_push_artifacts are the exception, kept as real attributes | |
| since they're either Jinja-templated (see template_fields) or read | |
| elsewhere in this class (the dbt_hook property, execute()). |
| # Escape hatch for any dbt flag or config option not modeled above, | ||
| # e.g. a project-level behavior change flag added by a newer dbt. | ||
| extra_flags: Optional[Dict[str, Any]] = None, | ||
| # Escape hatch for ANY dbt parameter, including ones with no named | ||
| # parameter on this operator at all: everything below is folded into | ||
| # this same dict for backwards compatibility, and this is what | ||
| # actually gets forwarded to the dbt hook (see execute()), instead of | ||
| # this operator's full __dict__, which also carries Airflow-internal | ||
| # attributes (task_id, dag, retries, ...) unrelated to dbt. | ||
| config_kwargs: Optional[Dict[str, Any]] = None, |
There was a problem hiding this comment.
nit: This is already documented in the docstring, so I find the comments redundant.
| # Kept as attributes: referenced by template_fields (Jinja rendering | ||
| # needs a real self.attr for each), or read elsewhere in this class | ||
| # (dbt_hook property, execute()). Every other parameter below is | ||
| # only ever consumed through self.config_kwargs, so no longer gets | ||
| # its own attribute -- see _update_config_kwargs(). |
There was a problem hiding this comment.
nit: Another redundant comment, docstring already covers this (mostly).
| **kwargs, | ||
| ) -> None: | ||
| super().__init__(**kwargs) | ||
| self.config_kwargs = dict(config_kwargs or {}) |
There was a problem hiding this comment.
suggestion: I think the intention here is making a copy of config_kwargs, but we should perhaps prefer a deep copy rather than a shallow copy. Just to make sure we don't mutate anything we don't exclusively own.
| with no dedicated field on any Config -- ends up in config_kwargs. | ||
| Parameters absorbed by this __init__'s own **kwargs (Airflow-internal | ||
| attributes like task_id, dag, retries, ...) are never included here, | ||
| since they're never individual entries in `locals()` to begin with. |
There was a problem hiding this comment.
thought: I don't like how fragile this is: if this class or any subclasses adds a new parameter then it's automatically included as a config parameter, even if it was never meant to be one.
I am trying to think of an alternative at the moment.
| @pytest.fixture(scope="session", autouse=True) | ||
| def _fix_in_process_execution_api_lifespan_race(): | ||
| """Work around a lifespan race in Airflow's ``InProcessExecutionAPI``. |
There was a problem hiding this comment.
I am going to trust you on this one and not verifying. Maybe we should make a note to remove this once we drop support for Airflow <3.3
|
I ended up cherry-picking the python 3.14, dbt 1.2, and airflow 3.3 commits, as those are pretty mechanical changes to merge in: #184. I just needed one extra small commit to get around support for a new flag. Again, I agree with the intention of this PR of not having to change the code every time dbt decides to add a new flag, but I think that's a broader discussion to have. In the meantime, I decided to get the minimal changes out, to get support for new versions faster while we discuss it. |
|
I agree. I got a bit carried away and strayed from the main issue. |
2 participants
Matrix updates
3.3.1(latest) and3.2.2(previous minor, kept for backward-compat coverage). Cloud Composer and MWAA both now track the latest release too, so no separate entries are needed for them.1.12.3,1.11.14,1.10.23(latest patch of each supported minor).python-version: '3.14'×dbt-version: '1.11.14'/'1.10.23': dbt-core<1.12pinsmashumaro<3.15, which has no Python 3.14 support (added only inmashumaro 3.17). This combination can't install, regardless of anything on our end — see dbt-labs/dbt-core#12098.mypyjob condition, which referenced Airflow/dbt versions that hadn't existed in the matrix for a while (somypywas silently never running).Compatibility fixes (dbt-core 1.12)
FreshnessTaskrequirescatalogs: dbt-core 1.12 added a requiredcatalogsargument toFreshnessTask.__init__, unlike siblingConfiguredTasksubclasses where it defaults toNone. AddedDBT_INSTALLED_GTE_1_12and passcatalogs=[]for this task specifically.BaseConfighardcoded every dbt behavior change flag (e.g.require_all_warnings_handled_by_warn_error) as a dataclass field. This breaks every time dbt adds a new one — most recentlyrequire_source_and_semantic_model_names_without_spacesin 1.12 — because dbt'sFlagsobject only sets the uppercase attribute for flags absent from our config'svars(), while some dbt code reads the lowercase one and raisesAttributeError. Replaced the hardcoded list with a runtime read ofdbt.contracts.project.ProjectFlags.project_only_flags, confirmed present with the same shape across all three matrix dbt versions. New dbt releases now pick up new flags automatically. Also added anextra_flagsescape hatch (mirroring the existingvarsfield) onBaseConfigand the operator, for anything we still don't model.Test infrastructure fix (Airflow <3.3)
InProcessExecutionAPIlifespan race: before apache/airflow#68840 (fixed in Airflow 3.3+),InProcessExecutionAPI.transportschedules the FastAPI execution-API app's lifespan startup viaasyncio.run_coroutine_threadsafewithout waiting for it, so adag.test()task run can race ahead ofapp.state.svcs_registrybeing set and fail withAttributeError. Added a workaround intests/conftest.pythat pre-warms the transport and waits for lifespan startup, mirroring the upstream fix, sinceInProcessExecutionAPIis anattrs-slotted class that can't otherwise be safely monkeypatched.Deprecations
no_<flag>counterpart of every boolean CLI option (no_introspect,no_defer,no_print, …) has never been something dbt itself needs — dbt's CLI merges--flag/--no-flaginto a single boolean option, so it was always purely our own invention. Left in place for backward compatibility, but now emits aDeprecationWarningpointing at<flag>=<value>instead, so it can eventually be removed and shrink this part of the public API.