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.

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

Make the policy functions pluggable #28558

Merged
merged 7 commits into from
Feb 12, 2023

Conversation

ashb
Copy link
Member

@ashb ashb commented Dec 23, 2022

Previously it was only possible to set "policy" functions via
airflow_local_settings.py which is fine for "small clusters" but being
able to control some of these policies from installed
plugins/distributions is helpful in a few circumstances: it lets
"platforms" (either of the SaaS variety, or internal platform teams)
specify some common policies, but still let local Ariflow teams define
other policies using airflow_local_settings

This uses Pluggy[1] for plugin management (that we are already using
for the listener interface), and out of the box that
gives us the ability to have multiple implementations of the policy
functions where it makes sense

  • on all the "mutation" hooks all implementations will be called. (i.e.
    for the policies that don't return a value)
  • for any policy function that returns a value (such as
    get_dagbag_import_timeout) then the plugins will be tried in LIFO
    order until a non-None value is returned. (No code needed for us --
    Pluggy handles this out of the box with the firstresult=True
    property on the hookspec decorator).

Todo:

  • Document the way of adding plugins (setuptools entrypoints) and mark it experimental
  • Document how multiple plugins hooking the same function interact.
  • Mention that the argument names must match
  • Issue deprecation warning when arg names don't match
  • ???

@potiuk
Copy link
Member

potiuk commented Dec 23, 2022

Nice. Super useful for multiple policies and to be able to write multiple smaller policies. I wondered initially if the args passed by pluggy are mutable but the example has it explained https://pluggy.readthedocs.io/en/stable/#a-complete-example

Indeed steak sauce does not goo well with eggs, but I wish they also deleted mint sauce and picked walnuts (?). I love mushy peas though.

Copy link
Member

@kaxil kaxil 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, haven't tested it out -- but code looks sensible :)

airflow/policies.py Outdated Show resolved Hide resolved
airflow/policies.py Outdated Show resolved Hide resolved
airflow/policies.py Outdated Show resolved Hide resolved
airflow/policies.py Outdated Show resolved Hide resolved
airflow/policies.py Outdated Show resolved Hide resolved
@ashb ashb force-pushed the airflow-policies-as-plugins branch from 382bb34 to 9728b82 Compare January 6, 2023 22:02
@ashb ashb force-pushed the airflow-policies-as-plugins branch from b4d9089 to 3f6086b Compare February 7, 2023 15:57
@ashb ashb marked this pull request as ready for review February 7, 2023 18:12
@ashb ashb requested a review from potiuk as a code owner February 7, 2023 18:12
ashb and others added 5 commits February 9, 2023 14:41
Previously it was only possible to set "policy" functions via
airflow_local_settings.py which is fine for "small clusters" but being
able to control some of these policies from installed
plugins/distributions is helpful in a few circumstances: it lets
"platforms" (either of the SaaS variety, or internal platform teams)
specify some common policies, but still let local Ariflow teams define
other policies using airflow_local_settings

This uses Pluggy[[1]] for plugin management (that we are already using
for the listener interface), and out of the box that
gives us the ability to have multiple implementations of the policy
functions where it makes sense

- on all the "mutation" hooks all implementations will be called. (i.e.
  for the policies that don't return a value)
- for any policy function that returns a value (such as
  `get_dagbag_import_timeout`) then the plugins will be tried in LIFO
  order until a non-None value is returned. (No code needed for us --
  Pluggy handles this out of the box with the `firstresult=True`
  property on the hookspec decorator).

[1]: https://pluggy.readthedocs.io/en/latest/index.html
Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
And move them back in to cluster-policies.rst -- it feels better where
it was
@ashb ashb force-pushed the airflow-policies-as-plugins branch from c25696a to 28987e8 Compare February 9, 2023 14:41
airflow/policies.py Outdated Show resolved Hide resolved
airflow/policies.py Outdated Show resolved Hide resolved
airflow/policies.py Outdated Show resolved Hide resolved
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@ashb ashb added this to the Airflow 2.6.0 milestone Feb 12, 2023
@ashb ashb merged commit e8ba03c into apache:main Feb 12, 2023
@ashb ashb deleted the airflow-policies-as-plugins branch February 12, 2023 16:03
@pierrejeambrun pierrejeambrun added type:improvement Changelog: Improvements type:new-feature Changelog: New Features and removed type:improvement Changelog: Improvements labels Feb 27, 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.

6 participants