AIP-84 Migrate /object/grid_data from views to FastAPI #44332

bugraoz93 · 2024-11-25T03:39:31Z

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

pierrejeambrun

Nice. A few early comments, but overall looking good.

I'll do a more in depth review when the PR is ready :)

airflow/api_fastapi/core_api/routes/ui/grid.py

airflow/api_fastapi/common/parameters.py

bugraoz93 · 2024-11-27T02:10:52Z

I addressed the comments. Thanks for the review! Some parts turned out to be more complex than I imagined. It still needs small touches and should be ready with unit tests soon. 😅

bugraoz93 · 2024-12-01T10:25:30Z

@pierrejeambrun this is ready for review. I removed the WIP earlier but forgot to ping you again 😅 Please when you have time, thanks!

pierrejeambrun

Great.

Looking good, just a few suggestions / improvement on the code, but nothing blocking.

I'll also let Brent double check that the interface corresponds to the UI specifications.

airflow/api_fastapi/core_api/routes/ui/grid.py

tests/api_fastapi/core_api/routes/ui/test_grid.py

bbovenzi

Great work! This will power so much of the new UI!

In the old UI we also included the note for both dag runs and task instances. We should still include it for the new UI.

airflow/ui/openapi-gen/requests/types.gen.ts

airflow/api_fastapi/core_api/routes/ui/grid.py

bugraoz93 · 2024-12-03T16:34:41Z

Great.

Looking good, just a few suggestions / improvement on the code, but nothing blocking.

I'll also let Brent double check that the interface corresponds to the UI specifications.

That sounds amazing! Let me know if I have missed anything, and we can fix it at an early stage.
I will review the comments and adjust the code accordingly in the coming hours. Many thanks for the review and the comments!

bugraoz93 · 2024-12-03T16:36:16Z

Great work! This will power so much of the new UI!

In the old UI we also included the note for both dag runs and task instances. We should still include it for the new UI.

I was writing my message and saw your review now 🙂 Thanks for the review, Brent!

pierrejeambrun

Nice

airflow/api_fastapi/core_api/routes/ui/service/grid.py

airflow/api_fastapi/common/parameters.py

pierrejeambrun · 2024-12-05T10:59:57Z

Can you also plug the new common filters include_upstream and include_downstream in structure_data (structure.py)

bugraoz93 · 2024-12-08T21:25:20Z

Can you also plug the new common filters include_upstream and include_downstream in structure_data (structure.py)

I already included this in the previous commit, updating that endpoint params according to changes on those command upstream|downstream variables :)

airflow/api_fastapi/core_api/routes/ui/grid.py

bugraoz93 · 2024-12-17T16:13:55Z

Amazing, thanks both of you for the open-minded approach and constructive feedback! :)
I really appreciate the great work done here. Handling recursive logic like this isn’t easy. This work would take much longer without that logic already in place. I am focusing on adding the necessary tests and checking the areas you’ve highlighted. The PR will be updated soon.

bugraoz93 · 2024-12-20T22:53:08Z

Can you check that limit is actually being respected? In my manual testing, I was trying to only fetch 14 runs but I always got the default of 100. @pierrejeambrun would you have any ideas here?

I have checked and it seems like it is working fine. I have included a test case for it.

Can we add a test case for a triply nested dag group? Because it looks like we're only bubbling a failed state up twice.

Indeed, this needs to be recalculated since the entire parent state is updated accordingly. I have included that small calculation to cascade this to all recursive parent groups. I have also included test cases for nested loops. I have tested locally with a case that is exactly similar to the picture you sent with 3 depth.

As long as we have tests covering those edge cases we should be fine.

I have included multiple test cases. I have also updated the code according to the comments. I will resolve them accordingly.

Only one specific thing to mention in general is I have included the SortParam OR method. Since the value won't be null, the only thing to compare is the generated way which falls to the primary key. This made me eliminate the unnecessary method and make or with order_by | SortParam(...., <first_element_ordering>). If you think this isn't generic enough to put on SortParam, I can move the OR logic to the grid method.

Sorry for the delay! Many thanks for the tests and reviews!

bugraoz93 · 2024-12-21T05:14:40Z

I double-checked the parameters created with filter_param_factory, and they weren’t working as expected. 😕 To address this, I needed to adjust the run_types and run_states parameter names to run_type and state, respectively. It seems that filter_param_factory requires the parameter names to match the column names to function correctly.

Additionally, I have included the remaining two filter tests to ensure full coverage. With this update, all filters have been tested.

Thanks!

Stale

airflow/api_fastapi/common/parameters.py

…s comments

…test

…se SortParam rather than order_expressions, remove unnecessary version check, include additional checks for MappedTaskGroups, Move include upstream and downstream to parameters.py, remove unrelated comment and include task and dagrun notes

…t_param, remove unreachable statement from parameters.py::_transform_ti_states, make include upstream/downstream Annotated optional and include new test for upstream/downstream

…ix loop order for calculating overall_state, fix sorting and include user to pass it as parameter and include new test for order_by

… query

…, change types of Pydantic models

…hild_states to prevent confusion

…ty, decide the order_by with first element of timetable.run_ordering, add __or__ method to SortParam, calculate overall_ti_state after proper additions in child_states, add more test cases around order_by, limit, filtering such as logical_date_gte/lte, make test 3 depth nested task group

…maining filter in unit test, make tests use params and parametrize test methods

bugraoz93 requested review from bbovenzi, pierrejeambrun, ryanahamilton, jscheffl, ashb and ephraimbuddy as code owners November 25, 2024 03:39

boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Nov 25, 2024

bugraoz93 added the legacy ui Whether legacy UI change should be allowed in PR label Nov 25, 2024

pierrejeambrun reviewed Nov 25, 2024

View reviewed changes

bugraoz93 force-pushed the feat/42595/grid-fastapi branch 2 times, most recently from 597fed2 to 41e6dca Compare November 27, 2024 02:07

bugraoz93 force-pushed the feat/42595/grid-fastapi branch from 41e6dca to dc6466d Compare November 28, 2024 00:07

bugraoz93 changed the title ~~[WIP] AIP-84 Migrate /object/grid_data from views to FastAPI~~ AIP-84 Migrate /object/grid_data from views to FastAPI Nov 28, 2024

bugraoz93 force-pushed the feat/42595/grid-fastapi branch 2 times, most recently from 0642476 to a65e089 Compare December 1, 2024 10:24

pierrejeambrun reviewed Dec 3, 2024

View reviewed changes

bbovenzi reviewed Dec 3, 2024

View reviewed changes

bugraoz93 force-pushed the feat/42595/grid-fastapi branch 2 times, most recently from aca5d83 to 46e7fb4 Compare December 5, 2024 00:08

tirkarthi mentioned this pull request Dec 5, 2024

AIP-38 | Add Grid view #44671

Open

pierrejeambrun reviewed Dec 5, 2024

View reviewed changes

bugraoz93 force-pushed the feat/42595/grid-fastapi branch from 46e7fb4 to fdf2fa9 Compare December 8, 2024 21:22

bbovenzi reviewed Dec 9, 2024

View reviewed changes

airflow/api_fastapi/core_api/routes/ui/grid.py Outdated Show resolved Hide resolved

bugraoz93 force-pushed the feat/42595/grid-fastapi branch 2 times, most recently from 561073d to 0de62d1 Compare December 20, 2024 22:41

bugraoz93 force-pushed the feat/42595/grid-fastapi branch from 0de62d1 to 69a0e73 Compare December 21, 2024 05:18

pierrejeambrun reviewed Dec 21, 2024

View reviewed changes

airflow/api_fastapi/common/parameters.py Outdated Show resolved Hide resolved

bugraoz93 added 20 commits December 22, 2024 23:48

Include grid endpoint to FastAPI

1b85793

Finalise the endpoint other than dynamically created tasks and addres…

fee35b2

…s comments

Finalise the endpoint with dynamically mapped tasks and include unit …

1645b82

…test

Use SessionDep for session param

0b50ea0

Fix unit test according to adjustments

f855b9d

Include missing MappedTaskGroup parent check to MappedOperator

bec8f75

Remove remaining version check

02f3f00

Move service/ to services/ui/, make sort_param unique for dag_run_sor…

0c241ff

…t_param, remove unreachable statement from parameters.py::_transform_ti_states, make include upstream/downstream Annotated optional and include new test for upstream/downstream

Fix SortParam creation

ec7c668

Revert changes in QueryLastDagRunStateFilter

f53e17d

include missing task_count to parent_node for recursive taskgroups, f…

eaad640

…ix loop order for calculating overall_state, fix sorting and include user to pass it as parameter and include new test for order_by

Rename num_runs to limit for consistency, make base_date filter range…

fb06957

… query

Fix task_count and states for nested task_groups, add alias to run_id…

cd2f6e0

…, change types of Pydantic models

Rebase and rerun pre-commit

5aabc09

Change GridTaskInstanceSummary state to TaskInstanceState object

7fdac87

Fix setting state in GridTaskInstanceSummary, change name states to c…

6c85456

…hild_states to prevent confusion

Fix run_type and state param not working due to naming and include re…

871a73f

…maining filter in unit test, make tests use params and parametrize test methods

Move SortParam not provided comparison logic to view

8cd110d

bugraoz93 force-pushed the feat/42595/grid-fastapi branch from 69a0e73 to 8cd110d Compare December 22, 2024 22:51

Remove forgotten code piece

e4f55fa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AIP-84 Migrate /object/grid_data from views to FastAPI #44332

AIP-84 Migrate /object/grid_data from views to FastAPI #44332

bugraoz93 commented Nov 25, 2024 •

edited

Loading

pierrejeambrun left a comment

bugraoz93 commented Nov 27, 2024 •

edited

Loading

bugraoz93 commented Dec 1, 2024

pierrejeambrun left a comment •

edited

Loading

bbovenzi left a comment

bugraoz93 commented Dec 3, 2024

bugraoz93 commented Dec 3, 2024

pierrejeambrun left a comment

pierrejeambrun commented Dec 5, 2024

bugraoz93 commented Dec 8, 2024

bugraoz93 commented Dec 17, 2024

bugraoz93 commented Dec 20, 2024

bugraoz93 commented Dec 21, 2024

AIP-84 Migrate /object/grid_data from views to FastAPI #44332

Are you sure you want to change the base?

AIP-84 Migrate /object/grid_data from views to FastAPI #44332

Conversation

bugraoz93 commented Nov 25, 2024 • edited Loading

pierrejeambrun left a comment

Choose a reason for hiding this comment

bugraoz93 commented Nov 27, 2024 • edited Loading

bugraoz93 commented Dec 1, 2024

pierrejeambrun left a comment • edited Loading

Choose a reason for hiding this comment

bbovenzi left a comment

Choose a reason for hiding this comment

bugraoz93 commented Dec 3, 2024

bugraoz93 commented Dec 3, 2024

pierrejeambrun left a comment

Choose a reason for hiding this comment

pierrejeambrun commented Dec 5, 2024

bugraoz93 commented Dec 8, 2024

bugraoz93 commented Dec 17, 2024

bugraoz93 commented Dec 20, 2024

bugraoz93 commented Dec 21, 2024

bugraoz93 commented Nov 25, 2024 •

edited

Loading

bugraoz93 commented Nov 27, 2024 •

edited

Loading

pierrejeambrun left a comment •

edited

Loading