Skip to content

Comments

[airflow] Extend airflow context parameter check for BaseOperator.execute (AIR302)#15713

Merged
dhruvmanila merged 9 commits intoastral-sh:mainfrom
astronomer:extend-airflow-context-check
Jan 27, 2025
Merged

[airflow] Extend airflow context parameter check for BaseOperator.execute (AIR302)#15713
dhruvmanila merged 9 commits intoastral-sh:mainfrom
astronomer:extend-airflow-context-check

Conversation

@Lee-W
Copy link
Contributor

@Lee-W Lee-W commented Jan 24, 2025 •
edited
Loading

Summary

  • feat
    • add is_execute_method_inherits_from_airflow_operator for checking the removed context key in the execute method
  • refactor: rename
    • is_airflow_task as is_airflow_task_function_def
    • in_airflow_task as in_airflow_task_function_def
    • removed_in_3 as airflow_3_removal_expr
    • removed_in_3_function_def as airflow_3_removal_function_def
  • test:
    • reorganize test cases

Test Plan

a test fixture has been updated

@Lee-W Lee-W mentioned this pull request Jan 24, 2025
Merged
@Lee-W Lee-W force-pushed the extend-airflow-context-check branch from cbc7854 to 8952c13 Compare January 24, 2025 10:48
@Lee-W
Copy link
Contributor Author

Lee-W commented Jan 24, 2025

as a heads up, I'll be less active next week as it's the lunar new year in Taiwan. Thanks for helping out!

@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dhruvmanila
Copy link
Member

dhruvmanila commented Jan 24, 2025 •
edited
Loading

refactor: rename

  • is_airflow_task as is_decorated_by_airflow_task
  • check_function_parameters as check_parameters_in_function_def
  • removed_in_3 as removed_expr_in_3
  • removed_in_3_function_def as removed_funciton_def_in_3

Can you provide the motivation for these renames? We usually prefer to have active voice instead of passive (at least regarding the first two renames) (https://github.com/astral-sh/ruff/blob/2b3550c85fcc55d93216c4d4ef64c5f0a5766053/crates/ruff_python_semantic/src/analyze/visibility.rs). I'm a bit torn on the last two renames because it indicates to me that some expression / function definition has been removed but that's not the case. I think we should rename them to airflow_3_removal (similar to numpy_2_0_deprecation) and add suffix _expr / _function_def to indicate the node that's being checked. Happy to hear your thoughts on this.

@dhruvmanila
Copy link
Member

dhruvmanila commented Jan 24, 2025

as a heads up, I'll be less active next week as it's the lunar new year in Taiwan. Thanks for helping out!

Thanks for the heads up. Have a great week! Do you know if there's more work required to complete #14626 ?

dhruvmanila
dhruvmanila approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

I've a minor concern about the renames.

@dhruvmanila dhruvmanila added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 24, 2025
Lee-W and others added 9 commits January 24, 2025 23:12
@Lee-W
test(AIR302): reorganize context handling
fc2dd86
@Lee-W
feat(AIR302): add is_execute_method_inherits_from_airflow_operator fo…
fbfb23b 
…r checking removed context key in execute method
@Lee-W
refactor(AIR302): rename removed_in_3 as removed_expr_in_3 and remove…
c779cbf 
…d_in_3_function_def as removed_funciton_def_in_3
@Lee-W
refactor(AIR302): rename check_function_parameters as check_parameter…
67f65cf 
…s_in_function_def
@Lee-W
refactor(AIR302): rename is_airflow_task as is_decorated_by_airflow_task
d64a952
@Lee-W @dhruvmanila
refactor(AIR302): remove unnecessary if else
b27ec65 
Co-authored-by: Dhruv Manilawala <[email protected]>
@Lee-W
Revert "refactor(AIR302): rename check_function_parameters as check_p…
d11f865 
…arameters_in_function_def"

This reverts commit f742313.
@Lee-W
refactor(AIR302): rename removed_in_3 as airflow_3_removal_expr and r…
4280d11 
…emoved_in_3_function_def as airflow_3_removal_function_def
@Lee-W
refactor(AIR302): rename is_airflow_task as is_airflow_task_function_…
25653cc 
…def and in_airflow_task as in_airflow_task_function_def
@Lee-W Lee-W force-pushed the extend-airflow-context-check branch from 9eb6715 to 25653cc Compare January 24, 2025 15:12
@Lee-W
Copy link
Contributor Author

Lee-W commented Jan 24, 2025

refactor: rename

  • is_airflow_task as is_decorated_by_airflow_task
  • check_function_parameters as check_parameters_in_function_def
  • removed_in_3 as removed_expr_in_3
  • removed_in_3_function_def as removed_funciton_def_in_3

Can you provide the motivation for these renames? We usually prefer to have active voice instead of passive (at least regarding the first two renames) (https://github.com/astral-sh/ruff/blob/2b3550c85fcc55d93216c4d4ef64c5f0a5766053/crates/ruff_python_semantic/src/analyze/visibility.rs). I'm a bit torn on the last two renames because it indicates to me that some expression / function definition has been removed but that's not the case. I think we should rename them to airflow_3_removal (similar to numpy_2_0_deprecation) and add suffix _expr / _function_def to indicate the node that's being checked. Happy to hear your thoughts on this.

The motivation was to make it more accurate. I tried to change it a bit. Please take a look. Thanks!

@Lee-W
Copy link
Contributor Author

Lee-W commented Jan 24, 2025

as a heads up, I'll be less active next week as it's the lunar new year in Taiwan. Thanks for helping out!

Thanks for the heads up. Have a great week! Do you know if there's more work required to complete #14626 ?

There're 3 cases I can think of as of now.

  1. context.get("conf") or context["get"] in BaseOperator.execute
  2. In PythonOperator(..., python_callable=func), the func needs to be checked
  3. templated variable (e.g., params="{{ conf }}"

The first should be easy, the second might need some exploration, and the third one is a bit tricky, and I'm not even sure whether we should check it.
Whether a variable can be templated is somewhat flexible, but on the other hand, it's also uncommon to have a string happen to be in that format

@Lee-W Lee-W requested a review from dhruvmanila January 24, 2025 16:14
@dylwil3 dylwil3 changed the title [airflow] Extend airflow context parameter check in BaseOperator.execute (AIR302) [airflow] Extend airflow context parameter check for BaseOperator.execute (AIR302) Jan 25, 2025
@dhruvmanila dhruvmanila merged commit c161e4f into astral-sh:main Jan 27, 2025
21 checks passed
@dhruvmanila dhruvmanila mentioned this pull request Jan 27, 2025
Merged
dhruvmanila added a commit that referenced this pull request Jan 27, 2025
@dhruvmanila
Rename internal helper functions (#15771)
e994970 
Refer:
#15713 (comment)
dcreager added a commit that referenced this pull request Jan 29, 2025
@dcreager
Merge branch 'main' into dcreager/terminal-visibility
2932bb4 
* main:
  [red-knot] Extend instance-attribute tests (#15808)
  Fix formatter warning message for `flake8-quotes` option (#15788)
  [`flake8-bugbear`] Exempt `NewType` calls where the original type is immutable (`B008`) (#15765)
  Add missing config docstrings (#15803)
  [`refurb`] Do not emit diagnostic when loop variables are used outside loop body (`FURB122`) (#15757)
  [`ruff`] Check for shadowed `map` before suggesting fix (`RUF058`) (#15790)
  [red-knot] Do not use explicit `knot_extensions.Unknown` declaration (#15787)
  Preserve quotes in generated byte strings (#15778)
  [minor] Simplify some `ExprStringLiteral` creation logic (#15775)
  Preserve quote style in generated code (#15726)
  Rename internal helper functions (#15771)
  [`airflow`] Extend airflow context parameter check for `BaseOperator.execute` (`AIR302`) (#15713)
  Implement tab autocomplete for `ruff config` (#15603)
@BrewTestBot BrewTestBot mentioned this pull request Jan 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhruvmanila dhruvmanila dhruvmanila approved these changes

Assignees

No one assigned

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Lee-W @dhruvmanila