-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Remove AIP-44 from airflow/sensors/base.py #44518
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.
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
Remove AIP-44 from airflow/sensors/base.py #44518
Conversation
|
|
||
| def execute(self, context: Context) -> Any: | ||
| @provide_session | ||
| def execute(self, context: Context, session=NEW_SESSION) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding additional arg session is causing lot of mypy failures in providers where BaseSensorOperator is being extended. Error's something like this:
providers/src/airflow/providers/microsoft/azure/sensors/data_factory.py:98: error:
Signature of "execute" incompatible with supertype "BaseSensorOperator"
[override]
def execute(self, context: Context) -> None:
^
providers/src/airflow/providers/microsoft/azure/sensors/data_factory.py:98: note: Superclass:
providers/src/airflow/providers/microsoft/azure/sensors/data_factory.py:98: note: def execute(context: Context, session: Any = ...) -> Any
providers/src/airflow/providers/microsoft/azure/sensors/data_factory.py:98: note: Subclass:
providers/src/airflow/providers/microsoft/azure/sensors/data_factory.py:98: note: def execute(self, context: Context) -> None
Should we just remove the decorator instead of updating execute method signature here?
Failing checks: https://github.com/apache/airflow/actions/runs/12101035549/job/33740515392?pr=44510
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted this in #44510
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uuups, oh was at-sleep while the error was on main. If we revert I assume we should not make this is a batch with another PR - should we not "just" revert the PR from me that caused it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just fixed it and it passed all tests. That should be good? I removed the decorator as well
…pache#44527) This reverts commit de94c06.
related: #44436