Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[airflow] Add fix to remove deprecated keyword arguments (AIR302) #14887

Merged
merged 7 commits into from
Dec 10, 2024
+207 −56
Merged

[airflow] Add fix to remove deprecated keyword arguments (AIR302) #14887

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aec2ce2
Add unsafe fixes based on suggestions.
tirkarthi Dec 9, 2024
9de09c8
Fix formatting.
tirkarthi Dec 10, 2024
d8d8c71
Fix clippy errors.
tirkarthi Dec 10, 2024
c6bb96b
Refactor help and message string.
tirkarthi Dec 10, 2024
d9b9a3b
Fix regression for sla_miss_callback.
tirkarthi Dec 10, 2024
6a1c167
Update crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs
tirkarthi Dec 10, 2024
4862e4e
Mark the fixes as safe.
tirkarthi Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions crates/ruff_linter/src/rules/airflow/rules/removal_in_3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{name::QualifiedName, Arguments, Expr, ExprAttribute, ExprCall};
use ruff_python_semantic::Modules;
Expand Down Expand Up @@ -43,6 +43,8 @@ pub(crate) struct Airflow3Removal {
}

impl Violation for Airflow3Removal {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let Airflow3Removal {
Expand All @@ -59,6 +61,10 @@ impl Violation for Airflow3Removal {
}
}
}

fn fix_title(&self) -> Option<String> {
Some("Replace deprecated keywords in Airflow 3.0".to_string())
}
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

fn diagnostic_for_argument(
Expand All @@ -67,7 +73,7 @@ fn diagnostic_for_argument(
replacement: Option<&str>,
) -> Option<Diagnostic> {
let keyword = arguments.find_keyword(deprecated)?;
Some(Diagnostic::new(
let mut diagnostic = Some(Diagnostic::new(
Airflow3Removal {
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
deprecated: (*deprecated).to_string(),
replacement: match replacement {
Expand All @@ -79,7 +85,16 @@ fn diagnostic_for_argument(
.arg
.as_ref()
.map_or_else(|| keyword.range(), Ranged::range),
))
));

if let Some(ref mut diagnostic) = diagnostic {
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
replacement?.to_string(),
diagnostic.range,
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
)));
}

Copy link
Member

Choose a reason for hiding this comment

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

We should expand the rule documentation and explain why the fix is unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fixes are drop in replacements for keywords. I had them unsafe by default for review. Is there a convention over how to classify if a fix as safe or unsafe?

Copy link
Member

Choose a reason for hiding this comment

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

What Micha is referring to is to add a section like https://docs.astral.sh/ruff/rules/unused-import/#fix-safety that describes why the fix is marked as unsafe. You can use existing documentation as a reference, search for "Fix safety" at https://docs.astral.sh/ruff/rules.

I think the reason this is unsafe is because the user would still need to update the references to the argument in the function body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the link @dhruvmanila , I feel the fixes are safer in this case since the values to keyword arguments or the attributes are not really referenced anywhere. I have marked the fixes as safe.

Copy link
Member

Choose a reason for hiding this comment

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

keyword arguments or the attributes are not really referenced anywhere

I'm not sure I understand this, can you expand? The argument most likely is being used in the function body, I'm referring to those references.

Copy link
Contributor Author

@tirkarthi tirkarthi Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The keyword arguments are to construction of a dag. Then the task constructed inside the context manager are automatically attached to the dag. They are not referred by the tasks in the context manager body and are specific to the dag object itself.

Below is a dag called latest_only where there are two tasks latest_only and task1 where task1 depends on latest_only and is denoted by overloading >> operator. Airflow earlier used to have schedule_interval and timetable but then they were merged to schedule keyword argument which is compatible to accept values and is a drop in replacement.

Before

with DAG(
    dag_id="latest_only",
    schedule_interval="daily",
    start_date=datetime.datetime(2021, 1, 1),
    catchup=False,
    tags=["example2", "example3"],
    sla_miss_callback=sla_callback
) as dag:
    latest_only = LatestOnlyOperator(task_id="latest_only")
    task1 = EmptyOperator(task_id="task1")

    latest_only >> task1

After fixes

with DAG(
    dag_id="latest_only",
    schedule="daily",
    start_date=datetime.datetime(2021, 1, 1),
    catchup=False,
    tags=["example2", "example3"],
    sla_miss_callback=sla_callback
) as dag:
    latest_only = LatestOnlyOperator(task_id="latest_only")
    task1 = EmptyOperator(task_id="task1")

    latest_only >> task1

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, sorry I misunderstood. These are function call arguments and not function parameters. Yeah, I think these should be safe to fix.

diagnostic
}

fn removed_argument(checker: &mut Checker, qualname: &QualifiedName, arguments: &Arguments) {
Expand Down
114 changes: 94 additions & 20 deletions ...rc/rules/airflow/snapshots/ruff_linter__rules__airflow__tests__AIR302_AIR302_args.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
source: crates/ruff_linter/src/rules/airflow/mod.rs
snapshot_kind: text
---
AIR302_args.py:15:39: AIR302 `schedule_interval` is removed in Airflow 3.0; use `schedule` instead
AIR302_args.py:15:39: AIR302 [*] `schedule_interval` is removed in Airflow 3.0; use `schedule` instead
|
13 | DAG(dag_id="class_schedule", schedule="@hourly")
14 |
Expand All @@ -11,46 +11,76 @@ AIR302_args.py:15:39: AIR302 `schedule_interval` is removed in Airflow 3.0; use
16 |
17 | DAG(dag_id="class_timetable", timetable=NullTimetable())
|
= help: Replace deprecated keywords in Airflow 3.0

AIR302_args.py:17:31: AIR302 `timetable` is removed in Airflow 3.0; use `schedule` instead
ℹ Unsafe fix
12 12 |
13 13 | DAG(dag_id="class_schedule", schedule="@hourly")
14 14 |
15 |-DAG(dag_id="class_schedule_interval", schedule_interval="@hourly")
15 |+DAG(dag_id="class_schedule_interval", schedule="@hourly")
16 16 |
17 17 | DAG(dag_id="class_timetable", timetable=NullTimetable())
18 18 |

AIR302_args.py:17:31: AIR302 [*] `timetable` is removed in Airflow 3.0; use `schedule` instead
|
15 | DAG(dag_id="class_schedule_interval", schedule_interval="@hourly")
16 |
17 | DAG(dag_id="class_timetable", timetable=NullTimetable())
| ^^^^^^^^^ AIR302
|
= help: Replace deprecated keywords in Airflow 3.0
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

AIR302_args.py:24:34: AIR302 `sla_miss_callback` is removed in Airflow 3.0
|
24 | DAG(dag_id="class_sla_callback", sla_miss_callback=sla_callback)
| ^^^^^^^^^^^^^^^^^ AIR302
|
ℹ Unsafe fix
14 14 |
15 15 | DAG(dag_id="class_schedule_interval", schedule_interval="@hourly")
16 16 |
17 |-DAG(dag_id="class_timetable", timetable=NullTimetable())
17 |+DAG(dag_id="class_timetable", schedule=NullTimetable())
18 18 |
19 19 |
20 20 | def sla_callback(*arg, **kwargs):

AIR302_args.py:32:6: AIR302 `schedule_interval` is removed in Airflow 3.0; use `schedule` instead
AIR302_args.py:32:6: AIR302 [*] `schedule_interval` is removed in Airflow 3.0; use `schedule` instead
|
32 | @dag(schedule_interval="0 * * * *")
| ^^^^^^^^^^^^^^^^^ AIR302
33 | def decorator_schedule_interval():
34 | pass
|
= help: Replace deprecated keywords in Airflow 3.0

AIR302_args.py:37:6: AIR302 `timetable` is removed in Airflow 3.0; use `schedule` instead
ℹ Unsafe fix
29 29 | pass
30 30 |
31 31 |
32 |-@dag(schedule_interval="0 * * * *")
32 |+@dag(schedule="0 * * * *")
33 33 | def decorator_schedule_interval():
34 34 | pass
35 35 |

AIR302_args.py:37:6: AIR302 [*] `timetable` is removed in Airflow 3.0; use `schedule` instead
|
37 | @dag(timetable=NullTimetable())
| ^^^^^^^^^ AIR302
38 | def decorator_timetable():
39 | pass
|
= help: Replace deprecated keywords in Airflow 3.0

AIR302_args.py:42:6: AIR302 `sla_miss_callback` is removed in Airflow 3.0
|
42 | @dag(sla_miss_callback=sla_callback)
| ^^^^^^^^^^^^^^^^^ AIR302
43 | def decorator_sla_callback():
44 | pass
|
ℹ Unsafe fix
34 34 | pass
35 35 |
36 36 |
37 |-@dag(timetable=NullTimetable())
37 |+@dag(schedule=NullTimetable())
38 38 | def decorator_timetable():
39 39 | pass
40 40 |

AIR302_args.py:50:39: AIR302 `execution_date` is removed in Airflow 3.0; use `logical_date` instead
AIR302_args.py:50:39: AIR302 [*] `execution_date` is removed in Airflow 3.0; use `logical_date` instead
|
48 | def decorator_deprecated_operator_args():
49 | trigger_dagrun_op = trigger_dagrun.TriggerDagRunOperator(
Expand All @@ -59,30 +89,74 @@ AIR302_args.py:50:39: AIR302 `execution_date` is removed in Airflow 3.0; use `lo
51 | )
52 | trigger_dagrun_op2 = TriggerDagRunOperator(
|
= help: Replace deprecated keywords in Airflow 3.0

ℹ Unsafe fix
47 47 | @dag()
48 48 | def decorator_deprecated_operator_args():
49 49 | trigger_dagrun_op = trigger_dagrun.TriggerDagRunOperator(
50 |- task_id="trigger_dagrun_op1", execution_date="2024-12-04"
50 |+ task_id="trigger_dagrun_op1", logical_date="2024-12-04"
51 51 | )
52 52 | trigger_dagrun_op2 = TriggerDagRunOperator(
53 53 | task_id="trigger_dagrun_op2", execution_date="2024-12-04"

AIR302_args.py:53:39: AIR302 `execution_date` is removed in Airflow 3.0; use `logical_date` instead
AIR302_args.py:53:39: AIR302 [*] `execution_date` is removed in Airflow 3.0; use `logical_date` instead
|
51 | )
52 | trigger_dagrun_op2 = TriggerDagRunOperator(
53 | task_id="trigger_dagrun_op2", execution_date="2024-12-04"
| ^^^^^^^^^^^^^^ AIR302
54 | )
|
= help: Replace deprecated keywords in Airflow 3.0

ℹ Unsafe fix
50 50 | task_id="trigger_dagrun_op1", execution_date="2024-12-04"
51 51 | )
52 52 | trigger_dagrun_op2 = TriggerDagRunOperator(
53 |- task_id="trigger_dagrun_op2", execution_date="2024-12-04"
53 |+ task_id="trigger_dagrun_op2", logical_date="2024-12-04"
54 54 | )
55 55 |
56 56 | branch_dt_op = datetime.BranchDateTimeOperator(

AIR302_args.py:57:33: AIR302 `use_task_execution_day` is removed in Airflow 3.0; use `use_task_logical_date` instead
AIR302_args.py:57:33: AIR302 [*] `use_task_execution_day` is removed in Airflow 3.0; use `use_task_logical_date` instead
|
56 | branch_dt_op = datetime.BranchDateTimeOperator(
57 | task_id="branch_dt_op", use_task_execution_day=True
| ^^^^^^^^^^^^^^^^^^^^^^ AIR302
58 | )
59 | branch_dt_op2 = BranchDateTimeOperator(
|
= help: Replace deprecated keywords in Airflow 3.0

AIR302_args.py:60:34: AIR302 `use_task_execution_day` is removed in Airflow 3.0; use `use_task_logical_date` instead
ℹ Unsafe fix
54 54 | )
55 55 |
56 56 | branch_dt_op = datetime.BranchDateTimeOperator(
57 |- task_id="branch_dt_op", use_task_execution_day=True
57 |+ task_id="branch_dt_op", use_task_logical_date=True
58 58 | )
59 59 | branch_dt_op2 = BranchDateTimeOperator(
60 60 | task_id="branch_dt_op2", use_task_execution_day=True

AIR302_args.py:60:34: AIR302 [*] `use_task_execution_day` is removed in Airflow 3.0; use `use_task_logical_date` instead
|
58 | )
59 | branch_dt_op2 = BranchDateTimeOperator(
60 | task_id="branch_dt_op2", use_task_execution_day=True
| ^^^^^^^^^^^^^^^^^^^^^^ AIR302
61 | )
|
= help: Replace deprecated keywords in Airflow 3.0

ℹ Unsafe fix
57 57 | task_id="branch_dt_op", use_task_execution_day=True
58 58 | )
59 59 | branch_dt_op2 = BranchDateTimeOperator(
60 |- task_id="branch_dt_op2", use_task_execution_day=True
60 |+ task_id="branch_dt_op2", use_task_logical_date=True
61 61 | )
62 62 |
63 63 | dof_task_sensor = weekday.DayOfWeekSensor(
Loading
Loading