Skip to content

Conversation

@guan404ming
Copy link
Member

Related Issue

#46730

How

  • alias the dag_display_name for GridRunsResponse
  • update tests to validate dag_display_name

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

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Aug 22, 2025
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the PR and LGTM overall.

I just have a question about the query statement.

Comment on lines 220 to 222
# Retrieve, sort the previous DAG Runs
base_query = select(
DagRun.dag_id,
DagRun.run_id,
DagRun.queued_at,
DagRun.start_date,
DagRun.end_date,
DagRun.run_after,
DagRun.state,
DagRun.run_type,
).where(DagRun.dag_id == dag_id)
base_query = select(DagRun).options(joinedload(DagRun.dag_model)).where(DagRun.dag_id == dag_id)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if selecting all fields of DagRun model will introduce long loading time for Gride View or not?

As Grid view optimization #51805 only select necessary fields for this route.
Maybe there isn't large difference, but it would be nice to have a small benchmark to find out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on my test on my local machine, it does increase the load time 0.1s in average. Not sure if this is ok in here.

before (avg 3.95s in 10 time tests)

Screenshot 2025-08-22 at 9 13 02 PM

after (avg 4.05 in 10 time tests)

Screenshot 2025-08-22 at 9 07 46 PM

Copy link
Member

@jason810496 jason810496 Aug 22, 2025

Choose a reason for hiding this comment

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

Thanks for the update!
How about taking Dag Example from Grid view optimization #51805 as baseline?

Or is this the same Dag as #51805? The dag name seems similiar.

Copy link
Member Author

@guan404ming guan404ming Aug 22, 2025

Choose a reason for hiding this comment

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

My computer is too old to run the original PR example smoothly 🥲, so I simplified it before testing. The simplified dag I used is based on the same idea, but not exactly identical to the one in #51805. The only difference is that the number of iterations is reduced to 5.

from __future__ import annotations

import datetime

import pendulum

from airflow.providers.standard.operators.empty import EmptyOperator
from airflow.sdk import DAG, TaskGroup, chain

with DAG(
    dag_id="big_hello",
    schedule="0 0 * * *",
    start_date=pendulum.datetime(2021, 1, 1, tz="UTC"),
    catchup=False,
    dagrun_timeout=datetime.timedelta(minutes=60),
    params={"example_key": "example_value"},
) as dag:
    for i in range(5):
        with TaskGroup(f"group_{i}"):
            EmptyOperator(task_id="hello")
            with TaskGroup(f"group_{i}2"):
                chain([EmptyOperator(task_id=f"empty_{j}") for j in range(5)])
                chain([EmptyOperator(task_id=f"empty2_{j}") for j in range(5)])
                # EmptyOperator.partial(task_id=f"hello2").expand(doc=list(range(5)))
            with TaskGroup(f"group_{i}3"):
                chain([EmptyOperator(task_id=f"empty_{j}") for j in range(5)])
                chain([EmptyOperator(task_id=f"empty2_{j}") for j in range(5)])
                # EmptyOperator.partial(task_id=f"hello2").expand(doc=list(range(5)))
            with TaskGroup(f"group_{i}4"):
                chain([EmptyOperator(task_id=f"empty_{j}") for j in range(5)])
                chain([EmptyOperator(task_id=f"empty2_{j}") for j in range(5)])
                # EmptyOperator.partial(task_id=f"hello2").expand(doc=list(range(5)))

@guan404ming guan404ming force-pushed the alias-grid-runs-response branch from cef984a to df68d4a Compare August 22, 2025 13:19
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.

Maybe we shouldn't do that for GridRunResponse. If we are fetching this endpoint, most cases are we are in the context of the Grid. And the Structure of the dag or a dag related endpoint has been called beforehand, the dag_display_name should be there and UI can use it, no need to add it to each GridRunsResponse. (So badically add the display name to the DAG related endpoint of the Grid, and not a all runs level)

@guan404ming
Copy link
Member Author

Maybe we shouldn't do that for GridRunResponse. If we are fetching this endpoint, most cases are we are in the context of the Grid. And the Structure of the dag or a dag related endpoint has been called beforehand, the dag_display_name should be there and UI can use it, no need to add it to each GridRunsResponse. (So badically add the display name to the DAG related endpoint of the Grid, and not a all runs level)

Make sense to me. I've checked frontend and confirmed that it's no needed to use this field. Let me close this one. Thanks!

@guan404ming guan404ming deleted the alias-grid-runs-response branch August 22, 2025 15:03
@guan404ming
Copy link
Member Author

Also confirmed and updated the remaining todos in parent issue.

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:UI Related to UI/UX. For Frontend Developers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants