Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AIP-84 Migrate Clear Dag Run public endpoint to FastAPI #42975

Merged
merged 24 commits into from
Nov 15, 2024

Conversation

rawwar
Copy link
Collaborator

@rawwar rawwar commented Oct 13, 2024

related to #42701

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Oct 13, 2024
@pierrejeambrun pierrejeambrun added the legacy api Whether legacy API changes should be allowed in PR label Oct 14, 2024
@rawwar rawwar closed this Oct 30, 2024
@rawwar rawwar force-pushed the kalyan/API-84/clear_dag_run branch from a418bcf to 4fc16f1 Compare October 30, 2024 06:39
@rawwar rawwar reopened this Oct 30, 2024
@rawwar
Copy link
Collaborator Author

rawwar commented Nov 3, 2024

@pierrejeambrun , I noticed that in legacy implementation, for clear dag run endpoint with dry_run=True, the response is supposed to be a TaskInstanceCollection. However, the response only includes few attributes of TI. Below is an example response :

{
  "task_instances": [
    {
      "dag_id": "example_astronauts",
      "dag_run_id": "manual__2024-11-03T14:05:08.832062+00:00",
      "execution_date": "2024-11-03T14:05:08.832062+00:00",
      "task_id": "print_astronaut_craft"
    },{
      "dag_id": "example_astronauts",
      "dag_run_id": "manual__2024-11-03T14:05:08.832062+00:00",
      "execution_date": "2024-11-03T14:05:08.832062+00:00",
      "task_id": "get_astronauts"
    }

Related schema: TaskInstanceReferenceSchema
Should I keep the same? Or should I return all the details?

I tried to return all details. But, to do that, it seems I can't reuse the methods in the legacy implementation as it is causing the following error.

Error extracting attribute: DetachedInstanceError: Parent instance <TaskInstance at 0x31af2e880> is not bound to a Session; lazy load operation of attribute 'task_instance_note' cannot proceed (Background on this error at: https://sqlalche.me/e/14/bhk3) [type=get_attribute_error, input_value=<TaskInstance: example_as....832062+00:00 [success]>, input_type=TaskInstance]

I guess, I just need to use joinload. But, thinking if I should update here or just rewrite a select query within clear_dag_run method

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 5, 2024

I think we can do the same and return a partial response. There is a way to do that in fastapi specifying the response model. And we need to document it in the swagger with example responses.

DetachedInstance error is most certainly due to a bad session handling. (session used to fetch objects is closed too early or something similar)

@rawwar rawwar marked this pull request as ready for review November 8, 2024 03:09
@rawwar
Copy link
Collaborator Author

rawwar commented Nov 8, 2024

I think we can do the same and return a partial response. There is a way to do that in fastapi specifying the response model. And we need to document it in the swagger with example responses.

I went with returning the entire object. Is that fine? If not, I can return the partial response. I already looked into how to do it

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.

Nice!

A few suggestions/questions

airflow/api_fastapi/core_api/routes/public/dag_run.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/dag_run.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/dag_run.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_dag_run.py Outdated Show resolved Hide resolved
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.

Nice, a few minor suggestions and ready to merge, thanks

airflow/api_fastapi/core_api/routes/public/dag_run.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/dag_run.py Outdated Show resolved Hide resolved
@pierrejeambrun pierrejeambrun merged commit c3aabba into apache:main Nov 15, 2024
52 checks passed
@rawwar rawwar deleted the kalyan/API-84/clear_dag_run branch November 15, 2024 08:24
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. legacy api Whether legacy API changes should be allowed in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants