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

Experimental: Support custom weight_rule implementation to calculate the TI priority_weight #38222

Merged

Conversation

hussein-awala
Copy link
Member

This PR adds an experimental feature that allows users to implement custom weight rules (weight strategies) to calculate the priority weight of the task instance in runtime.

This is a lighter alternative for #36029 but without serializing the class instance parameters, which removes the need to add a new column to the task table and handles many cases to keep the new functionality b/c .

@hussein-awala
Copy link
Member Author

Some of the dags/plugin examples tests will fail, I will fix them tomorrow before merging.

@hussein-awala hussein-awala added this to the Airflow 2.9.0 milestone Mar 17, 2024
@hussein-awala hussein-awala added type:new-feature Changelog: New Features area:core labels Mar 17, 2024
@hussein-awala hussein-awala force-pushed the experimental/priority_weight_strategy branch 3 times, most recently from 29a44e8 to e65e6df Compare March 17, 2024 02:20
@hussein-awala hussein-awala force-pushed the experimental/priority_weight_strategy branch 3 times, most recently from a9b00e2 to 6581f7b Compare March 17, 2024 16:53
@hussein-awala hussein-awala force-pushed the experimental/priority_weight_strategy branch from 6581f7b to 56cbc49 Compare March 17, 2024 17:21
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM!
Very cool

airflow/models/abstractoperator.py Outdated Show resolved Hide resolved
@hussein-awala hussein-awala force-pushed the experimental/priority_weight_strategy branch from 56cbc49 to 6a4d1c9 Compare March 17, 2024 20:28
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Tested and like it!

Besides the comment two things that I'd really like before merge but acknowledge this might not be blocking:

  • The example is removed - because obviously w/o default plugin deployed will not work. But w/o reading the code it might be hard to make it working in a frst attempt for a user. So if example is missing, then might be good to add some example DAG code in RST docs or commented example in the plugin code.
  • Small UI Glitch: As the priority weight is calculated dynamically, it might change display on UI after execution. In the decreasing example the displayed weight on UI changes after task was successfully executed. Because the try_number of the data at the task is a moving target. So we need to remember that the UI is a display of the current state and might have been different at time of execution depending on rule logic. I don't see it as problem and of course it is caused by the fast that we do not persist all values (not only priority weight but also other task execution evidence data).

Thanks again for the rework and besides small comment LGTM!

@hussein-awala
Copy link
Member Author

then might be good to add some example DAG code in RST docs or commented example in the plugin code.

I agree that it is not obvious for users to use it, I will add an example to the documentation.

@hussein-awala
Copy link
Member Author

So we need to remember that the UI is a display of the current state and might have been different at the time of execution depending on rule logic.

I will check if there is a way to improve this, but since it displays the priority weight used to schedule the current attempt, IMHO it should be fine. However, it would be better to add something that tells the user what we will have in the next attempt or update it when the task passes from running to any other state. We can discuss and implement this before 2.10.0

@hussein-awala hussein-awala force-pushed the experimental/priority_weight_strategy branch from 6a4d1c9 to 5ab7a40 Compare March 17, 2024 23:44
@hussein-awala hussein-awala merged commit 9855515 into apache:main Mar 18, 2024
45 of 47 checks passed
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:CLI area:core area:Executors-core LocalExecutor & SequentialExecutor area:plugins area:serialization type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants