Skip to content

API Add consuming and producing asset filters on list DagRuns - #56022

Closed
valentinDruzhinin wants to merge 1 commit into
apache:mainfrom
valentinDruzhinin:feature/53046-dag-view-runs-filters
Closed

API Add consuming and producing asset filters on list DagRuns#56022
valentinDruzhinin wants to merge 1 commit into
apache:mainfrom
valentinDruzhinin:feature/53046-dag-view-runs-filters

Conversation

@valentinDruzhinin

@valentinDruzhininvalentinDruzhinin commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

Relates to: #53046

The new consumed_assets and produced_assets fields were added.
image


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

@RoyLee1224

Copy link
Copy Markdown
Contributor

Hi @valentinDruzhinin , thanks for your work on this!

It looks like we might have been working on a similar issue in parallel. My PR #55735 also works for Dag run view tab, which seems to have a significant overlap with your work here.

Perhaps the maintainers could advise on the best way to combine or move forward with these changes. Thanks!

@vincbeckvincbeck added the allow translation change This label should be set if we want to bypass translation freeze and change english translations. label Sep 24, 2025
@valentinDruzhinin
valentinDruzhininforce-pushed the feature/53046-dag-view-runs-filters branch from d301012 to 00679e4CompareSeptember 24, 2025 14:51
@valentinDruzhinin

Copy link
Copy Markdown
ContributorAuthor

Hi @valentinDruzhinin , thanks for your work on this!

It looks like we might have been working on a similar issue in parallel. My PR #55735 also works for Dag run view tab, which seems to have a significant overlap with your work here.

Perhaps the maintainers could advise on the best way to combine or move forward with these changes. Thanks!

Hey @RoyLee1224 . Thanks for letting me know. It's sad to see that. I hope we can combine our work together.

@bbovenzi

Copy link
Copy Markdown
Contributor

Yes, let's definitely move forward with @RoyLee1224's UX changes. But perhaps we could have this PR focus just on backend changes?

I'm also hesitant about eagerly adding indexes.

Comment threadairflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py Outdated
@valentinDruzhinin

Copy link
Copy Markdown
ContributorAuthor

Yes, let's definitely move forward with @RoyLee1224's UX changes. But perhaps we could have this PR focus just on backend changes?

I'm also hesitant about eagerly adding indexes.

Sounds good. Let me revert the UI changes then. I'll only add the backend changes.

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@valentinDruzhinin#55735 PR is very close to be in a mergeable state.

I'm sorry but this PR will most probably not be merged in its current state, in favor of #55735. As Brent suggested we can focus this PR on the backend change that will allow filtering on the consuming assets and producing assets. This is a big enough change to be in a separate PR.

@RoyLee1224, please make sure to communicate when you start working on an issue that already has an assignee, just to avoid doing the work twice. 🙂

@RoyLee1224

Copy link
Copy Markdown
Contributor

Apologies for the confusion—I was assigned to #53043 and didn't see the overlap with #53046.

@valentinDruzhinin, sorry for stepping on your toes, I'll be more careful about checking for related issues in the future.

@valentinDruzhinin

Copy link
Copy Markdown
ContributorAuthor

Hey @pierrejeambrun can you reopen the PR please? I've added the changes on backend what you asked for consuming assets and producing assets. These changes haven't been added at #55735 .
Thank you

@pierrejeambrun

Copy link
Copy Markdown
Member

Hey @pierrejeambrun can you reopen the PR please? I've added the changes on backend what you asked for consuming assets and producing assets. These changes haven't been added at #55735 .
Thank you

Cool, let me just update the PR description then

@pierrejeambrunpierrejeambrun changed the title Add DAG view runs filtersAPI Add consuming and producing asset filters on list DagRunsOct 1, 2025
@valentinDruzhinin
valentinDruzhininforce-pushed the feature/53046-dag-view-runs-filters branch 8 times, most recently from 7c98c55 to baef3dcCompareOctober 3, 2025 20:07
@valentinDruzhinin

Copy link
Copy Markdown
ContributorAuthor

@bbovenzi@pierrejeambrun could you please take a look ? :)

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! API side looks good! I would appreciate another view from the UI side

@valentinDruzhinin

Copy link
Copy Markdown
ContributorAuthor

Thanks for the PR! API side looks good! I would appreciate another view from the UI side

Thanks, I can add UI filters after this PR

@bbovenzi

Copy link
Copy Markdown
Contributor

Yes, UI changes can be done in another PR but I don't see the AssetSummary showing up in the DagRunResponse in typescript. Could you take a look?

@bbovenzibbovenzi added this to the Airflow 3.2.0 milestone Oct 9, 2025
@bbovenzibbovenzi removed the allow translation change This label should be set if we want to bypass translation freeze and change english translations. label Oct 9, 2025
@bbovenzi

Copy link
Copy Markdown
Contributor

Also, can we please update the PR description and screenshots to be reflect the latest changes?

@valentinDruzhinin
valentinDruzhininforce-pushed the feature/53046-dag-view-runs-filters branch from baef3dc to d19356eCompareOctober 10, 2025 20:05
@valentinDruzhinin

Copy link
Copy Markdown
ContributorAuthor

@bbovenzi Thank you! Fixed the model to show the new fields in response.
And updated the PR description as well!
Please take a look :)

@valentinDruzhinin

Copy link
Copy Markdown
ContributorAuthor

@bbovenzi@pierrejeambrun please take a look

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

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.

)


class AssetSummary(PydanticBaseModel):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May I ask why do we need PydanticBaseModel instead of BaseModel here ?

total_entries=total_entries,
)
dag_runs = list(session.scalars(dag_runs_select))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to modularize the post processing logic for binding dr.consumed_assets and dr.produced_assets in airflow-core/src/airflow/api_fastapi/core_api/services/public/dag_run.py


def _iter_dags() -> Iterable[DAG | SerializedDAG]:
dagbag = DagBag(read_dags_from_db=True) # type: ignore[call-arg]
dagbag = DagBag(read_dags_from_db=True) # type: ignore[misc, call-arg]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems to be not related change.

Comment on lines +1193 to +1197
assert any(item.get("id") == prod_model.id for item in prod_list)

cons_list = runs_by_id[DAG1_RUN2_ID].get("consumed_assets", [])
assert isinstance(cons_list, list)
assert any(item.get("id") == cons_model.id for item in cons_list)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to validate what the exact dict looks like for AssetSummary here?

Somehow like

assertconsumed_assets== [
{
"id": ...,
"name": ...,
...

which might be more readable for the consumed_assets and produced_assets response.

Comment on lines +374 to +377
query = select(DagRun).options(
selectinload(DagRun.consumed_asset_events).selectinload(AssetEvent.asset),
joinedload(DagRun.dag_model),
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems we don't need these selectinload or joinedload statement here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we do need the first part selectinload(DagRun.consumed_asset_events) for the serialization to not emit N lazy db queries at serialization time.

The second part I believe can be moved inside the parameter logic because they seem to be the one using it, and there is no need to always do that eager loading if there is no filtering on consuming or producing assets


for dr in dag_runs:
consumed_list: list[AssetSummary] = []
for ev in getattr(dr, "consumed_asset_events", []) or []:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
forevingetattr(dr, "consumed_asset_events", [])or []:
forevingetattr(dr, "consumed_asset_events", []):

for ev in getattr(dr, "consumed_asset_events", []) or []:
consumed_list.append(AssetSummary(id=ev.asset_id, name=ev.name, uri=ev.uri, group=ev.group))
dr.consumed_assets = consumed_list
produced_list = produced_map.get((dr.dag_id, dr.run_id)) or []

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
produced_list=produced_map.get((dr.dag_id, dr.run_id)) or []
produced_list=produced_map.get((dr.dag_id, dr.run_id), [])

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, a few small things to address on top of jason comments and we should be good to move forward.

Comment on lines +374 to +377
query = select(DagRun).options(
selectinload(DagRun.consumed_asset_events).selectinload(AssetEvent.asset),
joinedload(DagRun.dag_model),
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we do need the first part selectinload(DagRun.consumed_asset_events) for the serialization to not emit N lazy db queries at serialization time.

The second part I believe can be moved inside the parameter logic because they seem to be the one using it, and there is no need to always do that eager loading if there is no filtering on consuming or producing assets

dag_runs = list(session.scalars(dag_runs_select))

source_keys = {(dr.dag_id, dr.run_id) for dr in dag_runs}
produced_map: dict[tuple[str, str], list[AssetSummary]] = {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can use a default dict, that will avoid the 'setdefault' call for each item below.

return select.where(exists_clause)


QueryDagRunConsumingAssetFilter = Annotated[

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similarly to other 'search' operating on ilike Pattern suffix should be in the name, take a look at other search params in the same file.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 20, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:APIAirflow's REST/HTTP APIarea:db-migrationsPRs with DB migrationarea:translationsarea:UIRelated to UI/UX. For Frontend Developers.kind:documentationstaleStale PRs per the .github/workflows/stale.yml policy filetranslation:default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@valentinDruzhinin@RoyLee1224@bbovenzi@pierrejeambrun@bugraoz93@jason810496@vincbeck