Restore ability to mark task groups as success/failed in UI #60161
Conversation
arjav1528
requested review from
bbovenzi,
guan404ming,
pierrejeambrun,
ryanahamilton,
shubhamraj-git and
vatsrahul1001
as code owners
|
@arjav1528 Thank you for working on that! 👍 |
arjav1528
force-pushed
the
main
branch
2 times, most recently
from
5db3cd8 to
c9d177e
Compare
is the UI ok or do you want me to refactor that |
bbovenzi
left a comment
bbovenzi
left a comment
There was a problem hiding this comment.
Thanks for taking this on! Do you mind doing more manually testing to find where MarkAs isn't quite working? I think there are some issues with the API that we need to tackle before we can merge any UI changes.
...low-core/src/airflow/ui/src/components/MarkAs/TaskInstance/MarkGroupTaskInstanceAsDialog.tsx
Outdated
Show resolved
Hide resolved
...low-core/src/airflow/ui/src/components/MarkAs/TaskInstance/MarkGroupTaskInstanceAsDialog.tsx
Outdated
Show resolved
Hide resolved
arjav1528
requested review from
bugraoz93,
ephraimbuddy,
jason810496,
potiuk and
rawwar
as code owners
providers/snowflake/tests/unit/snowflake/hooks/test_snowflake_sql_api.py
Outdated
Show resolved
Hide resolved
arjav1528
force-pushed
the
main
branch
2 times, most recently
from
b068b63 to
da03292
Compare
bbovenzi
left a comment
bbovenzi
left a comment
There was a problem hiding this comment.
I tried using this locally and still had issues. Could you both manually test this out and also add fastapi test for dry runs?
Sorry about the trouble. I’ve tested this locally and couldn’t reproduce the issue. I have reproduced the issue, solved it, you can run it locally now |
arjav1528
force-pushed
the
main
branch
3 times, most recently
from
d24fdae to
34153d8
Compare
…ooks and update request body validation
…o the service layer
…ate update logic and removing listener triggers
…ion for improved clarity and reuse in task instance patching
… ID, including dry run functionality and improved state management
…sk_instances.py Co-authored-by: Jason(Zhe-You) Liu <68415893+jason810496@users.noreply.github.com>
… affected instances and streamline updates for both single task instances and task groups
… instances in patching process
…points, enhancing dry run functionality and validation logic
…sk_instances.py Co-authored-by: Jason(Zhe-You) Liu <68415893+jason810496@users.noreply.github.com>
…llection of unique task instances in patching logic
… based on test group and downgrade SQLAlchemy flag
…tances to ensure correct handling of failed states
…r improved clarity and consistency
…up_id over identifier, improving error handling and validation flow
|
@jason810496 could you please review the PR and merge if good to go, |
jason810496
left a comment
jason810496
left a comment
There was a problem hiding this comment.
Hi @arjav1528, thank you for your patience and for the update.
I recommend reviewing the earlier comments, as some of the same issues have not yet been fully resolved. Thanks!
| # Default is 60 minutes; Min SQLAlchemy providers DB runs can exceed that slightly. | ||
| TOTAL_TEST_TIMEOUT: "${{ (inputs.test-group == 'providers' && inputs.downgrade-sqlalchemy == 'true') && '3900' || '3600' }}" | ||
| if: inputs.test-group == 'core' || inputs.skip-providers-tests != 'true' |
There was a problem hiding this comment.
I thinks this is a bad rebase. The change doesn't seem to be related to the PR.
There was a problem hiding this comment.
Still exist, needs another rebase.
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_instances.py
Outdated
Show resolved
Hide resolved
…ce function to simplify code and improve readability
…fier for task group, enhancing clarity in API requests
… for task group, improving API request structure
…bulk update logic to remove dry_run parameter for improved clarity
… for consistency across test groups
|
Thanks for the reviews @jason810496 @pierrejeambrun @bbovenzi! I've addressed all the feedback: Backend fixes: ✅ Removed the unreachable duplicate return statement in patch_task_instance The UI was previously calling endpoints that didn't exist in the backend, which would have caused all task group mark-as operations to fail with 404 errors. Ready for another review when you have time! |
…y and consistency in API requests
guan404ming
left a comment
guan404ming
left a comment
There was a problem hiding this comment.
There are still lots of function in backend is incompleted. How about let keep only backend changes in this pr to make it more easily to review and implement? Thanks!
There was a problem hiding this comment.
I think mostly tests mock wiring, not real logic here. Maybe we could just test here by e2e test instead of unit tests. We could handle in a follow up.
| } | ||
| }; | ||
|
|
||
| // Use direct API call until OpenAPI types are generated |
There was a problem hiding this comment.
I am not really sure why we need to call it directly here.
| # Default is 60 minutes; Min SQLAlchemy providers DB runs can exceed that slightly. | ||
| TOTAL_TEST_TIMEOUT: "${{ (inputs.test-group == 'providers' && inputs.downgrade-sqlalchemy == 'true') && '3900' || '3600' }}" | ||
| if: inputs.test-group == 'core' || inputs.skip-providers-tests != 'true' |
There was a problem hiding this comment.
Still exist, needs another rebase.
| service = BulkTaskInstanceService( | ||
| session=session, request=request, dag_id=dag_id, dag_run_id=dag_run_id, dag_bag=dag_bag, user=user | ||
| ).handle_request() | ||
| ) | ||
| return service.handle_request() |
There was a problem hiding this comment.
This change doesn't seem related and should be removed.
| @task_instances_router.patch( | ||
| task_instances_prefix + "/dry_run", | ||
| dependencies=[Depends(requires_access_dag(method="PUT", access_entity=DagAccessEntity.TASK_INSTANCE))], | ||
| operation_id="bulk_task_instances_dry_run", | ||
| ) | ||
| def bulk_task_instances_dry_run( | ||
| request: BulkBody[BulkTaskInstanceBody], | ||
| session: SessionDep, | ||
| dag_id: str, | ||
| dag_bag: DagBagDep, | ||
| dag_run_id: str, | ||
| user: GetUserDep, | ||
| ) -> TaskInstanceCollectionResponse: | ||
| """Bulk update task instances dry run - returns affected task instances without making changes.""" | ||
| service = BulkTaskInstanceService( | ||
| session=session, request=request, dag_id=dag_id, dag_run_id=dag_run_id, dag_bag=dag_bag, user=user | ||
| ) | ||
| return service.handle_request_dry_run() |
There was a problem hiding this comment.
This change deosn't seem related and should be removed.
| const downstream = selectedOptions.includes("downstream"); | ||
|
|
||
| // eslint-disable-next-line unicorn/no-null -- DAGRunResponse["note"] type requires null, not undefined | ||
| const [note, setNote] = useState<string | null>(null); |
There was a problem hiding this comment.
Initialise with "" and replace by null at the backend call so you don't need that eslint-disable-next-line unicorn/no-null.
Check other calls for [note, setNote] in the code base we never have to ignore this.
| ) | ||
| assert response.status_code == 200, response.text | ||
| response_data = response.json() | ||
| assert response_data["total_entries"] == 4 |
There was a problem hiding this comment.
Assert Task id ans state of the response tasks
| def test_dry_run_should_return_affected_task_instances(self, test_client, session): | ||
| """Test dry run returns affected task instances without making changes.""" | ||
| self.create_task_instances(session, dag_id=self.DAG_ID) | ||
|
|
||
| response = test_client.patch( | ||
| f"/dags/{self.DAG_ID}/dagRuns/{self.RUN_ID}/taskInstances/{self.TASK_ID}/dry_run", | ||
| params={"task_group_id": self.TASK_GROUP_ID}, | ||
| json={"new_state": self.NEW_STATE}, | ||
| ) | ||
| assert response.status_code == 200, response.text | ||
| response_data = response.json() | ||
| # section_1 has 3 tasks | ||
| assert response_data["total_entries"] == 3 | ||
|
|
||
| # Verify task_ids belong to section_1 | ||
| for ti in response_data["task_instances"]: | ||
| assert ti["task_id"].startswith("section_1.") |
There was a problem hiding this comment.
Merge this with the previous test
| assert response.status_code == 422 | ||
|
|
||
|
|
||
| class TestBulkTaskInstancesDryRun(TestTaskInstanceEndpoint): |
There was a problem hiding this comment.
Bulk is not appropriate, it qualifies the bulk update endpoint which is another endpoint, That's TestPatchTaskInstanceDryRunByTaskGroupId And it should probably be merged into the existing class for testing TestPatchTaskInstanceDryRun that already exist
| assert response.json() == {"task_instances": [], "total_entries": 0} | ||
|
|
||
|
|
||
| class TestPatchTaskInstanceByTaskGroupId(TestTaskInstanceEndpoint): |
There was a problem hiding this comment.
If possible Merge this into TestPatchTaskInstance. 1 test class = 1 endpoint. It's the same test endpoint with different parameter.
| TASK_ID = "start" | ||
| NEW_STATE = "failed" | ||
|
|
||
| def test_should_update_task_group_instances_state(self, test_client, session): |
There was a problem hiding this comment.
We probably need to add test for updating the 'note'. All tis should have the same note. It would be confusing for users to have the 'note' work only for single TI and not for groups.
| readonly onSuccess?: () => void; | ||
| }; | ||
|
|
||
| export const useBulkUpdateTaskInstances = ({ |
| /*! | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the |
There was a problem hiding this comment.
What is this file for? Seems unrelated.
8 participants
Description
This PR adds two new React components that restore the task group marking functionality:
MarkGroupTaskInstanceAsButton- A dropdown button component that allows users to select a state (success/failed) and opens a dialog for confirmationMarkGroupTaskInstanceAsDialog- A dialog component that provides:The button is integrated into the Group Task Instance page header, alongside the existing Clear button.
Screenshots
Solves #60121
Closes: #56103