Skip to content

Conversation

@guan404ming
Copy link
Member

Related PR

How

  • add multi-select in ti table
  • minor fix in bulk TI deletion endpoint
Screen.Recording.2025-06-10.at.4.40.43.PM.mov

^ 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 Jun 10, 2025
@guan404ming guan404ming force-pushed the bulk-ti-ui branch 2 times, most recently from 1adc30a to 3376842 Compare June 10, 2025 10:15
@guan404ming guan404ming marked this pull request as draft June 11, 2025 19:02
@guan404ming
Copy link
Member Author

I have updated the translation and try reuse AffectedTasks
Current ui looks like:

Screen.Recording.2025-06-13.at.2.39.31.AM.mov

@guan404ming guan404ming marked this pull request as ready for review June 12, 2025 18:56
@pierrejeambrun pierrejeambrun added this to the Airflow 3.1.0 milestone Jun 13, 2025
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.

For mapped task instance the confirmation table is not correct, cf screenshot, actually 4 TIs are selected (3 mapped TI and 1 non mapped TI), and the modal only show 2 TIs)
Screenshot 2025-06-13 at 15 41 51

@guan404ming
Copy link
Member Author

For mapped task instance the confirmation table is not correct, cf screenshot, actually 4 TIs are selected (3 mapped TI and 1 non mapped TI), and the modal only show 2 TIs)

The api needs some modification for handling ti with map_index as well. Let me mark this as draft.

@guan404ming guan404ming marked this pull request as draft June 13, 2025 15:10
@pierrejeambrun
Copy link
Member

pierrejeambrun commented Jun 16, 2025

The api needs some modification for handling ti with map_index as well. Let me mark this as draft.

Thanks @guan404ming, do not hesitate to create a different PR for the backend part so it's easier to review and merge. (and you can rebase this branch on top of the backend change to test it locally)

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.

This was introduced in #50165 to fix #50104 to try to get feature parity with 2.x.

The goal was to be able to remove both TIs and DagRuns.

It seems like the singular TI deletion is already in our UI and API

Yes, as well as singular dag run. Both in API and UI.

-1. We should never delete TIs and TIH. This shouldn't even be a feature in the API!

@ashb I guess this also goes for dag runs. Should we go ahead and remove all of that from API and UI ?

cc: @bbovenzi

@ashb
Copy link
Member

ashb commented Sep 16, 2025

I guess it's up to users if they want to do it, but we should probably make it 100% clear it will delete all history and audit records of the TI.

I don't personally like it, but now I think about it more I'm not sure that we should stop users from being able to do this if they want to.

Maybe a future change to add a config option to disable deleteing them at the deploy level

@bbovenzi
Copy link
Contributor

I guess it's up to users if they want to do it, but we should probably make it 100% clear it will delete all history and audit records of the TI.

I don't personally like it, but now I think about it more I'm not sure that we should stop users from being able to do this if they want to.

Maybe a future change to add a config option to disable deleteing them at the deploy level

How about we update the copy "This will remove all metadata related" and spell out what will be deleted?

@FrankPortman
Copy link

Not being able to easily sort/search-by-params/delete runs/TIs in bulk is a big con of AF3 for our use case.

@pierrejeambrun
Copy link
Member

@guan404ming any progress on this one ? I think a UI warning could suffice as mentioned by Brent, and unblock this.

@guan404ming
Copy link
Member Author

@guan404ming any progress on this one ? I think a UI warning could suffice as mentioned by Brent, and unblock this.

Sure! I'm also agree with this solution. I've update the warning to a more detailed version, please let me if it could be better. Thanks!

@guan404ming guan404ming marked this pull request as draft October 15, 2025 08:28
@guan404ming guan404ming marked this pull request as ready for review December 10, 2025 10:13
@guan404ming
Copy link
Member Author

I've updated to work with new endpoint. Please take another look, thanks!

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.

On my end, the mark as 'success' 'delete' is not working everytime. Request goes to the backend, and is successful, but it's just not updating the state of the tasks.

Screen.Recording.2025-12-10.at.15.59.45.mov

Also deleting a TI can leave a 'unfound tasks' in the UI, here task 1 was deleted.

Image

I think there are a lot of edge cases to consider, and we need extensive testing / fixes for it to work. This is a complex feature to get right.

Also the crosshair is truncated

Image

Comment on lines +92 to +103
</Dialog.Header>

<Dialog.CloseTrigger />

<Dialog.Body width="full">
<ActionAccordion affectedTasks={affectedTasks} note={note} setNote={setNote} />
<Flex justifyContent="end" mt={3}>
<Button colorPalette="blue" loading={isPending} onClick={() => handlePatch(selectedState)}>
{translate("modal.confirm")}
</Button>
</Flex>
</Dialog.Body>
Copy link
Member

Choose a reason for hiding this comment

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

We are missing the selector,

Image

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's fine for now since we use the 'bulk' endpoint.

Comment on lines +42 to +49
const onSuccess = async (responseData: { delete?: { errors: Array<unknown>; success: Array<string> } }) => {
const queryKeys = [
[useTaskInstanceServiceGetTaskInstancesKey],
dagId === undefined || dagRunId === undefined
? []
: [UseGridServiceGetGridTiSummariesKeyFn({ dagId, runId: dagRunId })],
];

Copy link
Member

@pierrejeambrun pierrejeambrun Dec 10, 2025

Choose a reason for hiding this comment

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

Grid is not updated after a successful delete. We should probably do that too. cf video. (clearing tasks from the same run)

Comment on lines +43 to +46
const queryKeys = [
[useTaskInstanceServiceGetTaskInstancesKey],
dagId === undefined || dagRunId === undefined
? []
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@Lee-W
Copy link
Member

Lee-W commented Jan 9, 2026

@guan404ming would like to follow up whehter wer're still working on this one?

@Lee-W
Copy link
Member

Lee-W commented Jan 9, 2026

I'm now making it a draft. Feel free to mark it as ready

@Lee-W Lee-W marked this pull request as draft January 9, 2026 08:18
@guan404ming
Copy link
Member Author

Apologies for the delay. This PR has become stale, and I noticed there are other active PRs addressing the same issue. To avoid redundancy, I’ll go ahead and close this one. Thank you everyone for your time and reviews.

@guan404ming
Copy link
Member Author

I think splitting this into several PRs is a better approach. I’ll figure out the remaining gaps and open targeted PRs accordingly. I’m also happy to help review other open PRs for this feature to avoid overlapping work.

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.

9 participants