Uh oh!
There was an error while loading. Please reload this page.
feat: implement patching of task group instances in API - #62812
Conversation
c03d03f to
60004daCompareThere was a problem hiding this comment.
Thanks for the PR.
Can you add query guards to tests (assert_queries_count) . I'm afraid this will generate N+1 queries problem -> Number of db request will scale linearly with the number of TIs in the group. And we should rework the code not do do that.
Also I find the code overly complicated with numerous duplicated call made at different abstraction level because of function nesting, it makes the whole thing hard to understand and probably sub optimal.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Thanks for the detailed review! I think the points made a lot of sense. Apologies for this oversight on my side. I tried following the implementation of the other PR more, removing the BulkTaskInstanceService usage and solving the N+1 query problem. I still opted to keep them in separate endpoints for clarity. Let me know if you disagree! Also happy to cast this aside and wait for the other PR to be picked up again, if that is more in line with your preference. One question regarding the EDIT - I'm now asserting whether the query count is equal between different size task groups. Should solve the above 👍 |
767a96c to
85e4f72Compare@OscarLigthart Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
85e4f72 to
0bfc970Comparecabb86e to
88add8cCompare1cfe416 to
326b655Compare@OscarLigthart Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
OscarLigthart
commented
Apr 23, 2026
Putting back to ready for review as I'm waiting for maintainer comments in unresolved threads. |
potiuk
commented
Apr 27, 2026
@OscarLigthart — There are 2 unresolved review threads on this PR from @pierrejeambrun. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
potiuk
commented
Apr 27, 2026
Hmmm.. I see that those things are resolved .... Hmm... Probably we should improve the automation. |
potiuk
commented
Apr 27, 2026
@pierrejeambrun -> I will improve the automation here, but this one looks like it's resolved and I marked it as ready for review + invite you to re-review :) |
potiuk
commented
Apr 27, 2026
sorry @OscarLigthart for a bit of spam :( ... we are improving things. |
potiuk
commented
Apr 27, 2026
OscarLigthart
commented
Apr 27, 2026
No worries at all, happy to be a guinea pig. Also very much understand the need for this automation. |
326b655 to
a0c6f89CompareUh oh!
There was an error while loading. Please reload this page.
* feat: implement patching of task group instances in API * feat: improve upon N+1 query problem * fix: revert back to multi calls * feat: implement set_task_group_state with locking mechanism * feat: some cleanup and additional tests * chore: improve function name * Small Adjustments * Small Adjustments * Fix note bug --------- Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
jose-padron-momox
commented
Jun 11, 2026
being able to mark a whole task group as success/failed in the UI is something we really miss from Airflow 2.x. Thank you to everyone working on it! Glad to see the backend support (#62812) merged; looking forward to the UI follow-up landing too. |
pierrejeambrun
commented
Jun 11, 2026
@jose-padron-momox front-end PR was merged here: That will be available in 3.3.0 release coming soon. |
Context
In Airflow 3 the ability to mark a full task group as failed or success is currently missing. In this PR, I try to implement the logic into the API, that can then be called from the frontend to ensure the functionality returns.
I deliberately split the feature in two separate PRs, so the reviewing process can be more targeted to the individually touched components. Should this PR get merged, I will continue to build the remaining requirements into the frontend.
There is an already open PR here: #60161
However, it looks to be stale, and I would love to get this feature into the Airflow 3.2 release.
Implementation
I make use of the BulkTaskInstanceService to perform the state updates with this endpoint. Using it, the implementation should be pretty straightforward :).
Issues
related: #56103
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus following the guidelines
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.