Skip to content

Conversation

@Dev-iL
Copy link
Collaborator

@Dev-iL Dev-iL commented Dec 26, 2025

After commit 60b4ed4 ("Remove PriorityWeightStrategy reference in SDK"), SDK operators store weight_rule as a string instead of a PriorityWeightStrategy object. When TaskInstance.refresh_from_task() calls task.weight_rule.get_weight(), it fails silently due to contextlib.suppress(Exception), leaving priority_weight unset.

Fix by wrapping SDK operators with create_scheduler_operator() before passing to TaskInstance, matching the pattern used in core tests.


^ 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 airflow-core/newsfragments.

@uranusjr
Copy link
Member

I’m doing a fix in a more blanket way in #59835, but eventually we should fix all tests to do this instead.

@Dev-iL
Copy link
Collaborator Author

Dev-iL commented Dec 27, 2025

@jscheffl @uranusjr Given your other attempts/reversion, should I close this PR or try to fix it?

@Dev-iL Dev-iL force-pushed the 2512/fix_priority_weight branch from a9a0da2 to f9a9434 Compare December 27, 2025 06:35
@potiuk
Copy link
Member

potiuk commented Dec 27, 2025

@jscheffl @uranusjr Given your other attempts/reversion, should I close this PR or try to fix it?

I think it's good to go as-is - the change in #59835 is yet to be green :)

@Dev-iL Dev-iL force-pushed the 2512/fix_priority_weight branch from f9a9434 to 8654ba1 Compare December 28, 2025 07:33
@Dev-iL Dev-iL force-pushed the 2512/fix_priority_weight branch from 8654ba1 to 3122c97 Compare December 28, 2025 13:04
@Dev-iL
Copy link
Collaborator Author

Dev-iL commented Dec 28, 2025

@potiuk @uranusjr Please see the addition I just included in the PR (a compat test shim for importing create_scheduler_operator). Please lmk if this is the direction we want to go.

@Dev-iL Dev-iL force-pushed the 2512/fix_priority_weight branch from 3122c97 to dcd9918 Compare December 28, 2025 15:53
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

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

Just a nit, otherwise LGTM

After commit 60b4ed4 ("Remove PriorityWeightStrategy reference in
SDK"), SDK operators store weight_rule as a string instead of a
PriorityWeightStrategy object. When TaskInstance.refresh_from_task()
calls task.weight_rule.get_weight(), it fails silently due to
contextlib.suppress(Exception), leaving priority_weight unset.

Fix by wrapping SDK operators with create_scheduler_operator() before
passing to TaskInstance, matching the pattern used in core tests.
@Dev-iL Dev-iL force-pushed the 2512/fix_priority_weight branch from dcd9918 to 08734bc Compare December 29, 2025 07:35
@kaxil
Copy link
Member

kaxil commented Dec 29, 2025

Worth waiting for #59835 to be merged?

@Dev-iL
Copy link
Collaborator Author

Dev-iL commented Dec 29, 2025

Worth waiting for #59835 to be merged?

Ultimately up to @uranusjr. These changes should be complementary.

@Dev-iL
Copy link
Collaborator Author

Dev-iL commented Dec 30, 2025

Closing because unnecessary after #59835.

@Dev-iL Dev-iL closed this Dec 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants