[airflow] Add lint rule to show error for removed context variables in airflow #15144

sunank200 · 2024-12-26T08:14:27Z

Summary

Airflow 3.0 removes following deprecated Airflow context variables:

conf
execution_date
next_ds
next_ds_nodash
next_execution_date
prev_ds
prev_ds_nodash
prev_execution_date
prev_execution_date_success
tomorrow_ds
yesterday_ds
yesterday_ds_nodash

They have been deprecated in 2.x, but the removal causes incompatibilities that we want to detect.

related: #44409, #41641

Test Plan

A test fixture is included in the PR.

github-actions · 2024-12-26T08:20:32Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs

uranusjr · 2024-12-26T08:51:16Z

crates/ruff_linter/resources/test/fixtures/airflow/AIR302_context.py

+from airflow.decorators import task
+
+@task
+def print_config(**context):


There are some more things we should check

Named keyword arguments (e.g. def print_config(execution_date))

Getting the context with get_current_context instead of function arguments.

context in an operator’s execute.

These can be added in separate PRs instead after this is merged.

I have added logic for other ways to access context value as well. It is part of tests.

For Named keyword arguments (e.g. def print_config(execution_date)) I can create a separate PR

kaxil · 2024-12-26T08:52:33Z

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs

+            if id.as_str() == "context" {
+                if let Some(key) = extract_name_from_slice(slice) {
+                    const REMOVED_CONTEXT_KEYS: [&str; 11] = [
+                        "execution_date",


"conf" had also been removed

It's tracked in apache/airflow#45212, but yep, we could do it here as well

If we want to add conf here, it would be awesome if we could include triggering_dataset_events → triggering_asset_events. If not, I can make a separate PR

I have added conf removal check. include triggering_dataset_events → triggering_asset_events can be part of another PR

If conf has been added, could you please update the description as well? Thanks!

...rc/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_context.py.snap

Lee-W · 2024-12-26T08:52:28Z

crates/ruff_linter/resources/test/fixtures/airflow/AIR302_context.py

+@task
+def print_config(**context):
+    # This should not throw an error as logical_date is part of airflow context.
+    logical_date = context["logical_date"]


@sunank200 and I discussed this earlier. What we're trying to check is whether there's a variable named as context in a function (most commonly seen in taskflow and python operator) and whether it's can be accessed like a dict with the keys we want to check. I think it's unlikely users are using something like this out of the airflow context. But would like to know whether there's any concern

@MichaReiser @uranusjr

I have added logic for other ways to access context value as well. It is part of tests.

It’s probably better to detect

Arguments of a function decorated with @task (either ** or simple named arguments). (As a follow-up, any functions called by such a function)

The execute function of a BaseOperator subclass (As a follow-up, any functions called by execute)

The dict returned by get_current_context.

This should be better than detecting with variable name.

What about python_callable?

I don’t think python_callable takes the context though? It only accepts things you provide in self.op_args and self.op_kwargs.

I agree that it'll be useful to guard this check by first verifying that the parameter is coming from a function which is decorated with a @task.

I think this can be done as a pre-check for context variables by using the checker.semantic().current_statements() method to traverse up the AST to find the function definition node and checking whether the function has a @task decorator that originates from the airflow module.

ruff/crates/ruff_python_semantic/src/model.rs

Lines 1232 to 1239 in 9fd4eb8

/// Returns an [`Iterator`] over the current statement hierarchy, from the current [`Stmt`]

/// through to any parents.

pub fn current_statements(&self) -> impl Iterator<Item = &'a Stmt> + '_ {

let id = self.node_id.expect("No current node");

self.nodes

.ancestor_ids(id)

.filter_map(move |id| self.nodes[id].as_statement())

}

I don’t think python_callable takes the context though? It only accepts things you provide in self.op_args and self.op_kwargs.

I though we can still get it in the python_callable? https://airflow.apache.org/docs/apache-airflow/stable/howto/operator/python.html#pythonoperator

Hmm OK I didn’t even realise you can do that… yeah in that case it’s probably a good idea to also detect python_callable arguments.

crates/ruff_linter/resources/test/fixtures/airflow/AIR302_context.py

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs

Lee-W · 2024-12-27T05:47:16Z

crates/ruff_linter/resources/test/fixtures/airflow/AIR302_context.py

+@task
+def print_config(**context):
+    # This should not throw an error as logical_date is part of airflow context.
+    logical_date = context["logical_date"]


What about python_callable?

crates/ruff_linter/resources/test/fixtures/airflow/AIR302_context.py

dhruvmanila · 2024-12-30T08:13:37Z

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs

+
+    if let Expr::Subscript(ExprSubscript { value, slice, .. }) = expr {
+        if let Expr::Name(ExprName { id, .. }) = &**value {
+            if id.as_str() == "context" {


I think we'd also need to check where the context variable is originating from otherwise, I think, this will raise a violation on all variables that's named "context" and is using a similar access pattern.

i.e., we need to make sure that the definition of context variable is the function parameter that's decorated with @task.

add lint rule to show error for removed context variables in airflow

jscheffl · 2025-01-02T08:54:08Z

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs

+pub(crate) fn removed_context_variable(checker: &mut Checker, expr: &Expr) {
+    const REMOVED_CONTEXT_KEYS: [&str; 12] = [
+        "conf",
+        "execution_date",


For execution_date there is actually a replacement - in the docs: https://airflow.apache.org/docs/apache-airflow/stable/templates-ref.html#deprecated-variables - can you add this?

(Same for next_execution_date, prev_execution_date)

sunank200 force-pushed the deprecated_context_variable_airflow branch from 89ed2ce to c3fb996 Compare December 26, 2024 08:27

sunank200 mentioned this pull request Dec 26, 2024

Check whether context[key]has variables that are no longer available apache/airflow#44409

Open

2 tasks

uranusjr reviewed Dec 26, 2024

View reviewed changes

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs Outdated Show resolved Hide resolved

uranusjr reviewed Dec 26, 2024

View reviewed changes

kaxil reviewed Dec 26, 2024

View reviewed changes

Lee-W reviewed Dec 26, 2024

View reviewed changes

sunank200 force-pushed the deprecated_context_variable_airflow branch from 5c96f89 to 5103ef7 Compare December 27, 2024 04:52

sunank200 requested review from Lee-W and uranusjr December 27, 2024 04:53

uranusjr reviewed Dec 27, 2024

View reviewed changes

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs Outdated Show resolved Hide resolved

uranusjr reviewed Dec 27, 2024

View reviewed changes

crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs Outdated Show resolved Hide resolved

Lee-W reviewed Dec 27, 2024

View reviewed changes

Lee-W mentioned this pull request Dec 30, 2024

Airflow 2 to 3 auto migration rules - ruff (~44080) apache/airflow#44556

Open

dhruvmanila reviewed Dec 30, 2024

View reviewed changes

dhruvmanila added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 30, 2024

This was referenced Jan 2, 2025

Airflow 2 to 3 auto migration rules - ruff (44080 ~ 45017) apache/airflow#45212

Open

Airflow 2 to 3 auto migration rules apache/airflow#41641

Open

sunank200 force-pushed the deprecated_context_variable_airflow branch 2 times, most recently from 6da5dd9 to d580a4b Compare January 2, 2025 08:00

Add more checks for removed context variables

c0a34d3

add lint rule to show error for removed context variables in airflow

sunank200 force-pushed the deprecated_context_variable_airflow branch from d580a4b to c0a34d3 Compare January 2, 2025 08:03

jscheffl reviewed Jan 2, 2025

View reviewed changes

Lee-W mentioned this pull request Jan 3, 2025

feat(AIR302): check whether removed context key "conf" is access through context.get(...) #15240

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[airflow] Add lint rule to show error for removed context variables in airflow #15144

[airflow] Add lint rule to show error for removed context variables in airflow #15144

sunank200 commented Dec 26, 2024 •

edited

Loading

github-actions bot commented Dec 26, 2024 •

edited

Loading

uranusjr Dec 26, 2024

sunank200 Dec 27, 2024

sunank200 Dec 27, 2024

kaxil Dec 26, 2024

Lee-W Dec 26, 2024

Lee-W Dec 26, 2024

sunank200 Dec 27, 2024

Lee-W Jan 2, 2025

Lee-W Dec 26, 2024

sunank200 Dec 27, 2024

uranusjr Dec 27, 2024

Lee-W Dec 27, 2024

uranusjr Dec 30, 2024

dhruvmanila Dec 30, 2024

Lee-W Dec 30, 2024

uranusjr Jan 2, 2025

Lee-W Dec 27, 2024

dhruvmanila Dec 30, 2024

dhruvmanila Dec 30, 2024

jscheffl Jan 2, 2025

	/// Returns an [`Iterator`] over the current statement hierarchy, from the current [`Stmt`]
	/// through to any parents.
	pub fn current_statements(&self) -> impl Iterator<Item = &'a Stmt> + '_ {
	let id = self.node_id.expect("No current node");
	self.nodes
	.ancestor_ids(id)
	.filter_map(move \|id\| self.nodes[id].as_statement())
	}

[airflow] Add lint rule to show error for removed context variables in airflow #15144

Are you sure you want to change the base?

[airflow] Add lint rule to show error for removed context variables in airflow #15144

Conversation

sunank200 commented Dec 26, 2024 • edited Loading

Summary

Test Plan

github-actions bot commented Dec 26, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

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

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

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

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

sunank200 commented Dec 26, 2024 •

edited

Loading

github-actions bot commented Dec 26, 2024 •

edited

Loading

`ruff-ecosystem` results