Skip to content

AIP-84 Add external dependencies to GET Structure Data Endpoint - #44701

Merged
pierrejeambrun merged 5 commits into
apache:mainfrom
astronomer:AIP-84-add-external-dependencies-to-structure
Dec 12, 2024
Merged

AIP-84 Add external dependencies to GET Structure Data Endpoint#44701
pierrejeambrun merged 5 commits into
apache:mainfrom
astronomer:AIP-84-add-external-dependencies-to-structure

Conversation

@pierrejeambrun

@pierrejeambrunpierrejeambrun commented Dec 5, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Related to: #42367

Adds the external_dependencies query parameter.

Supports trigger, sensor, asset, asset_aliases.

Sensor is back because ExternalTaskSensor is creating a sensor dependency type.

For now all that is external to the dag, and not specific to the task_level. Meaning that they are attached to the dag (first task of the dag) and not to the specific tasks. (i.e task outlets for assets for instance)

@pierrejeambrunpierrejeambrun added the AIP-84 Modern Rest API label Dec 5, 2024
@pierrejeambrunpierrejeambrun self-assigned this Dec 5, 2024
@boring-cyborgboring-cyborgBot added the area:UI Related to UI/UX. For Frontend Developers. label Dec 5, 2024
@pierrejeambrun
pierrejeambrun marked this pull request as draft December 5, 2024 18:53
pierrejeambrun
pierrejeambrun commented Dec 5, 2024
View reviewed changes
Comment threadtests/api_fastapi/core_api/routes/ui/test_structure.py
@pierrejeambrun
pierrejeambrunforce-pushed the AIP-84-add-external-dependencies-to-structure branch from 46ee957 to 64f3cc9CompareDecember 10, 2024 09:40
@pierrejeambrun
pierrejeambrun marked this pull request as ready for review December 10, 2024 13:41
@pierrejeambrunpierrejeambrun changed the title AIP-84 Add external dependencies to GET Structure Data Endpoint Part 1AIP-84 Add external dependencies to GET Structure Data EndpointDec 10, 2024
@pierrejeambrun
pierrejeambrunforce-pushed the AIP-84-add-external-dependencies-to-structure branch from d76d6b1 to 8789aaaCompareDecember 10, 2024 14:26
shubhamraj-git
shubhamraj-git reviewed Dec 10, 2024
View reviewed changes

@shubhamraj-gitshubhamraj-git left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!!

Comment threadairflow/api_fastapi/core_api/routes/ui/structure.py Outdated
Comment threadairflow/api_fastapi/core_api/routes/ui/structure.py Outdated
@pierrejeambrun
pierrejeambrunforce-pushed the AIP-84-add-external-dependencies-to-structure branch from 5ea5c6f to 87b5121CompareDecember 11, 2024 13:20
bbovenzi
bbovenzi reviewed Dec 11, 2024
View reviewed changes

@bbovenzibbovenzi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Testing locally, I noticed two issues:

example_external_task_marker_parent: did not have any downstream deps

example_trigger_controller_dag`: the dependency comes back as upstream when it should be downstream

Comment threadairflow/api_fastapi/core_api/routes/ui/structure.py
Comment threadairflow/api_fastapi/core_api/routes/ui/structure.py Outdated
@pierrejeambrunpierrejeambrun mentioned this pull request Dec 12, 2024
Merged
@pierrejeambrun

pierrejeambrun commented Dec 12, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

example_external_task_marker_parent: did not have any downstream deps

Indeed. That's a limitation we have at the moment in the backend in detect_task_dependencies. only TriggerDagRunOperator and ExternalTaskSensor are supported. ExternalTaskMarker is not. We might want to add it but this will impact the serialized dags and I'm not sure yet how free we are of updating that. (and how this dependencies are then used by the scheduler I assume). We will need to discuss that with other to confirm the feasibility of that approach.

I will open a dedicated issue to track that because its not straight forward.

edit: Dedicated issue here #44879

@pierrejeambrun

pierrejeambrun commented Dec 12, 2024

Copy link
Copy Markdown
MemberAuthor

example_trigger_controller_dag`: the dependency comes back as upstream when it should be downstream

Good catch, fixed, also added a test for that.

bbovenzi
bbovenzi approved these changes Dec 12, 2024
View reviewed changes

@bbovenzibbovenzi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm!

@pierrejeambrun
pierrejeambrun merged commit e0377eb into apache:mainDec 12, 2024
@pierrejeambrun
pierrejeambrun deleted the AIP-84-add-external-dependencies-to-structure branch December 12, 2024 23:28
ellisms pushed a commit to ellisms/airflow that referenced this pull request Dec 13, 2024
@pierrejeambrun@ellisms
AIP-84 Add external dependencies to GET Structure Data Endpoint (apac…
b5a73e0
…he#44701)
* Working state
* Add tests for external task sensor
* Add test for TriggerDagRunOperator
* Fix following code review
* Update following code review
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
@pierrejeambrun@got686-yandex
AIP-84 Add external dependencies to GET Structure Data Endpoint (apac…
7425d2b
…he#44701)
* Working state
* Add tests for external task sensor
* Add test for TriggerDagRunOperator
* Fix following code review
* Update following code review
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shubhamraj-gitshubhamraj-gitshubhamraj-git left review comments
@bbovenzibbovenzibbovenzi approved these changes
@ryanahamiltonryanahamiltonAwaiting requested review from ryanahamilton
@jscheffljschefflAwaiting requested review from jscheffl
@ephraimbuddyephraimbuddyAwaiting requested review from ephraimbuddy
@ashbashbAwaiting requested review from ashb

Assignees

@pierrejeambrunpierrejeambrun

Labels

AIP-84Modern Rest APIarea:UIRelated to UI/UX. For Frontend Developers.

Projects

No open projects
AIP-84 MODERN REST API
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pierrejeambrun@bbovenzi@shubhamraj-git