Skip to content

Conversation

@Lee-W
Copy link
Member

@Lee-W Lee-W commented Jul 7, 2023

As discussed in #32355 (comment), I'm thinking of adding some type annotations to the documentation.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Lee-W Lee-W marked this pull request as ready for review July 7, 2023 09:09
@Lee-W Lee-W requested a review from potiuk as a code owner July 7, 2023 09:09
Copy link
Member

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

we should add the same for exampleDateTimeTrigger too we have 2 serialize and run methods there wdyt?

@Lee-W
Copy link
Member Author

Lee-W commented Jul 7, 2023

we should add the same for exampleDateTimeTrigger too we have 2 serialize and run methods there wdyt?

Sorry, I kinda don't get it. Could you point out where is it?

@pankajastro
Copy link
Member

Sorry, I kinda don't get it. Could you point out where is it?

sure, type annotation for trigger examples also code block https://github.com/apache/airflow/blob/main/docs/apache-airflow/authoring-and-scheduling/deferring.rst?plain=1#L111-L124

@Lee-W
Copy link
Member Author

Lee-W commented Jul 7, 2023

ah yes, I've changed all the code block into that

@Lee-W Lee-W force-pushed the add-type-annotation-to-deferrable-example branch from 4c34d66 to 0ceeac9 Compare July 7, 2023 16:04
* Any Operator can defer; no special marking on its class is needed, and it's not limited to Sensors.
* In order for any changes to a Trigger to be reflected, the *triggerer* needs to be restarted whenever the Trigger is modified.
* If you want add an operator or sensor that supports both deferrable and non-deferrable modes. It's suggested to add ``deferable: bool = conf.getboolean("operators", "default_deferrable", fallback=False)`` to the ``__init__`` method of the operator and use it to decide whether to run the operator in deferrable mode. You'll be able to configure the default value of ``deferrable`` of all the operators and sensors that supports switch between deferrable and non-deferrable mode through ``default_deferrable`` in the ``operator`` section. Here's an example of a sensor that supports both modes.::
* If you want add an operator or sensor that supports both deferrable and non-deferrable modes. It's suggested to add ``deferrable: bool = conf.getboolean("operators", "default_deferrable", fallback=False)`` to the ``__init__`` method of the operator and use it to decide whether to run the operator in deferrable mode. You'll be able to configure the default value of ``deferrable`` of all the operators and sensors that supports switch between deferrable and non-deferrable mode through ``default_deferrable`` in the ``operator`` section. Here's an example of a sensor that supports both modes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are talking about both operators and sensors, but giving an example of deferrable: bool = conf.getboolean("operators", "default_deferrable", fallback=False) only for operators.

Copy link
Member Author

Choose a reason for hiding this comment

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

As the sensor is a kind of operator as well https://github.com/apache/airflow/blob/main/airflow/sensors/base.py#L82, it seems to me this should be fine. But I'm good with making this change as well. We probably should add default_deferrable here. Maybe that should be another PR?

Co-authored-by: Syed Hussain <[email protected]>
Copy link
Contributor

@syedahsn syedahsn left a comment

Choose a reason for hiding this comment

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

LGTM

@potiuk potiuk merged commit 5d8c54e into apache:main Jul 9, 2023
@eladkal eladkal added this to the Airflow 2.6.4 milestone Jul 9, 2023
@eladkal eladkal added the type:doc-only Changelog: Doc Only label Jul 9, 2023
@eladkal eladkal modified the milestones: Airflow 2.6.4, Airflow 2.7.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants