Skip to content

Apply task group sorting based on webserver config in grid structure response #49418

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 2, 2025
Merged

Apply task group sorting based on webserver config in grid structure response #49418

merged 6 commits into from
May 2, 2025

Conversation

jsjasonseba
Copy link
Contributor

closes: #49374

^ 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.

@jsjasonseba
Copy link
Contributor Author

Hi @pierrejeambrun , need your help reviewing this PR

pierrejeambrun
pierrejeambrun reviewed Apr 17, 2025
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Checking the services/ui/grid.py we have some get_task_group_children_getter there, which I think is supposed to do something similiar. Maybe there's something to fix there, I didn’t take a deep look yet ?

@jsjasonseba
Copy link
Contributor Author

Checking the services/ui/grid.py we have some get_task_group_children_getter there, which I think is supposed to do something similiar. Maybe there's something to fix there, I didn’t take a deep look yet ?

Hi @pierrejeambrun , thanks for pointing this out, I've looked into the code, and it seems like the ordering in frontend relies from StructureDataResponse which are generated from get_combined_structure. Currently, get_combined_structure does not utilize get_task_group_children_getter which sets the proper sorting based on Airflow cfg. So, there is no issue with the get_task_group_children_getter function, it is just not utilized to generate the structure response. Let me utilize this function instead.

@jsjasonseba jsjasonseba force-pushed the fix/task-group-grid-not-sorted-topologically branch from 40b697d to 425bc61 Compare April 18, 2025 16:58
@jsjasonseba jsjasonseba changed the title Apply topological sort on task group grid Apply task group sorting based on webserver config in grid structure response Apr 18, 2025
@jsjasonseba
Copy link
Contributor Author

Hi @pierrejeambrun , the changes are done. Feel free to review at your convenience.

@pierrejeambrun pierrejeambrun added this to the Airflow 3.0.1 milestone Apr 28, 2025
@pierrejeambrun pierrejeambrun added the backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch label Apr 28, 2025
pierrejeambrun
pierrejeambrun approved these changes Apr 28, 2025
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

LGTM thanks, needs rebase and conflict solving.

bugraoz93
bugraoz93 approved these changes Apr 28, 2025
View reviewed changes
Copy link
Contributor

@bugraoz93 bugraoz93 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, thanks! Agree with Pierre on rebase.

@jsjasonseba
Copy link
Contributor Author

Sure, I'll rebase soon

@jsjasonseba jsjasonseba force-pushed the fix/task-group-grid-not-sorted-topologically branch from f0a8a77 to aa2a9c6 Compare April 29, 2025 14:17
@bugraoz93
Copy link
Contributor

One more rebase should fix the K8s tests.

@jsjasonseba
Copy link
Contributor Author

@pierrejeambrun conflict solved

@bbovenzi bbovenzi merged commit d833ecb into apache:main May 2, 2025
97 checks passed
github-actions bot pushed a commit that referenced this pull request May 2, 2025
@jsjasonseba
[v3-0-test] Apply task group sorting based on webserver config in gri…
c24e1b7 
…d structure response (#49418)

* Apply topological sort on task group grid

* Fix task_group sorting by utilizing get_task_group_children_getter to sort based on webserver config

* Adjust test expectation to sort children topologically
(cherry picked from commit d833ecb)

Co-authored-by: Jason <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented May 2, 2025

Backport successfully created: v3-0-test

Status Branch Result
✅ v3-0-test PR Link

potiuk pushed a commit that referenced this pull request May 3, 2025
@github-actions @jsjasonseba
[v3-0-test] Apply task group sorting based on webserver config in gri…
a20a812 
…d structure response (#49418) (#50138)

* Apply topological sort on task group grid

* Fix task_group sorting by utilizing get_task_group_children_getter to sort based on webserver config

* Adjust test expectation to sort children topologically
(cherry picked from commit d833ecb)

Co-authored-by: Jason <[email protected]>
mvfc pushed a commit to mvfc/airflow that referenced this pull request May 6, 2025
@jsjasonseba @mvfc
Apply task group sorting based on webserver config in grid structure …
6c9e2f4 
…response (apache#49418)

* Apply topological sort on task group grid

* Fix task_group sorting by utilizing get_task_group_children_getter to sort based on webserver config

* Adjust test expectation to sort children topologically
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bugraoz93 bugraoz93 bugraoz93 approved these changes

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@rawwar rawwar Awaiting requested review from rawwar rawwar is a code owner

@jason810496 jason810496 Awaiting requested review from jason810496 jason810496 is a code owner

@shubhamraj-git shubhamraj-git Awaiting requested review from shubhamraj-git shubhamraj-git is a code owner

Assignees
No one assigned
Labels
backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch
Projects
None yet
Milestone
Airflow 3.0.1
Development

Successfully merging this pull request may close these issues.

Tasks within Task Groups are not sorted topologically in grid view
4 participants
@jsjasonseba @bugraoz93 @pierrejeambrun @bbovenzi