Uh oh!
There was an error while loading. Please reload this page.
feat: adding new feature to mark task group as success or failed - #58102
feat: adding new feature to mark task group as success or failed#58102itayweb wants to merge 11 commits into
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
guan404ming
commented
Nov 12, 2025
Could you help resolve static check error and attatch some screenshot or demo for your changes? Thanks! |
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the PR!
The unit tests are also required in airflow-core/tests/unit/api_fastapi/core_api/services/public/test_task_instances.py for the corresponding changes, thanks!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
itayweb
commented
Nov 12, 2025
What do you mean by static check error? Regarding the screenshots I'll add |
potiuk
commented
Nov 12, 2025
See the errors in the CI outputs - you need to have |
| """Bulk update, and delete task instances.""" | ||
| return BulkTaskInstanceService( | ||
| session=session, request=request, dag_id=dag_id, dag_run_id=dag_run_id, dag_bag=dag_bag, user=user | ||
| session=session, request=request, dag_id=dag_id, dag_run_id=dag_run_id, dag_bag=dag_bag, user=user, commit=True, |
There was a problem hiding this comment.
I've removed the duplicate dry run function and used the new parameter passed (commit) to reuse the existing function
Uh oh!
There was an error while loading. Please reload this page.
5228041 to
0f9013fCompareitayweb
commented
Nov 22, 2025
I've took a look into the unit tests that are already exist and noticed that there is already for bulk task instances but in the routes instead of in the services, so is it suitable for this? looks good to me, also there is not for the new added dry run, should I added another for the dry run? |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Cool PR.
A few suggestions before we can merge 🎉
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.
ddf30db to
80172f6Compareitayweb
commented
Dec 6, 2025
Yes 😅 |
There was a problem hiding this comment.
I gave it some thoughts and it appears that the bulk api doesn't really fit well here.
Maybe we should instead use the "patch/update" TI endpoint and add support for TaskGroup there.
There already is a dry run endpoint for this too. And in addition it returns a TI collection already because it handles the "multiple mapped indexes" use case.
10b0b1a to
ce03f38Comparepierrejeambrun
commented
Jan 9, 2026
Feel free to resolve irrelevant/addressed comments. Let us know when the PR is ready for another round of review, it looks like there are unrelated change added that need to be cleaned. |
itayweb
commented
Jan 9, 2026
Currently working on your last comment of using the existing patch endpoint |
ce03f38 to
c491700Compareitayweb
commented
Jan 9, 2026
I've created a new endpoint for patching task and task group to not interfere with existing routes/tests, let me know what do you all think about my new approach 😄 |
c491700 to
777b701Compare…n clear multiple tasks
777b701 to
461e109Compare| ], | ||
| operation_id="post_patch_task_instances", | ||
| ) | ||
| def post_patch_task_instances( |
There was a problem hiding this comment.
Some cleaning like that post_patch
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/HTTPValidationError' | ||
| /api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/dry_run: |
There was a problem hiding this comment.
We shouldn't use the bulk task instance endpoint.
itayweb
commented
Jan 30, 2026
Sorry about that. I think you could take your time on finding another issue and trying to fix it. Meanwhile, you could still read the review from reviewer here and learn how to improve your next pr. Thanks for your contribution and looking forward to your next try! |
closes: #56103
related: #50443
Adding new feature for Apache Airflow 3.x to mark TaskGroup tasks as Success/Failed, using existing bulk api