Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
pierrejeambrun merged PR #44701: URL: https://github.com/apache/airflow/pull/44701 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
pierrejeambrun commented on code in PR #44701: URL: https://github.com/apache/airflow/pull/44701#discussion_r1882382180 ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,36 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] if nodes else None +exit_node_ref = nodes[-1] if nodes else None + +start_edges: list[dict] = [] +end_edges: list[dict] = [] + +for dependency_dag_id, dependencies in SerializedDagModel.get_dag_dependencies().items(): +for dependency in dependencies: +if dependency_dag_id != dag_id and dependency.target != dag_id: +continue + +# Add nodes +nodes.append( +{ +"id": dependency.node_id, +"label": dependency.node_id, Review Comment: Yes. `dependency.dependency_id` is better for the label. ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,36 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] if nodes else None +exit_node_ref = nodes[-1] if nodes else None + +start_edges: list[dict] = [] +end_edges: list[dict] = [] + +for dependency_dag_id, dependencies in SerializedDagModel.get_dag_dependencies().items(): +for dependency in dependencies: +if dependency_dag_id != dag_id and dependency.target != dag_id: +continue + +# Add nodes +nodes.append( +{ +"id": dependency.node_id, +"label": dependency.node_id, Review Comment: Yes. `dependency.dependency_id` is better for the label. updated thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
pierrejeambrun commented on PR #44701: URL: https://github.com/apache/airflow/pull/44701#issuecomment-2539282868 > example_trigger_controller_dag`: the dependency comes back as upstream when it should be downstream Good catch, fixed, also added a test for that. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
pierrejeambrun commented on PR #44701: URL: https://github.com/apache/airflow/pull/44701#issuecomment-2539140079 > 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) I will open a dedicated issue to track that because it needs its own set of change to be able to work. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
pierrejeambrun commented on code in PR #44701: URL: https://github.com/apache/airflow/pull/44701#discussion_r1882237887 ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,36 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] if nodes else None +exit_node_ref = nodes[-1] if nodes else None Review Comment: Exactly, there is an explicit call above when accessing the root dag group children. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
bbovenzi commented on code in PR #44701: URL: https://github.com/apache/airflow/pull/44701#discussion_r1880690283 ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,36 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] if nodes else None +exit_node_ref = nodes[-1] if nodes else None Review Comment: Nodes are already topologically sorted, right? ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,36 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] if nodes else None +exit_node_ref = nodes[-1] if nodes else None + +start_edges: list[dict] = [] +end_edges: list[dict] = [] + +for dependency_dag_id, dependencies in SerializedDagModel.get_dag_dependencies().items(): +for dependency in dependencies: +if dependency_dag_id != dag_id and dependency.target != dag_id: +continue + +# Add nodes +nodes.append( +{ +"id": dependency.node_id, +"label": dependency.node_id, Review Comment: the node_id is a bad label. There was also `dependency.dependency_id` is that more helpful? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
pierrejeambrun commented on code in PR #44701: URL: https://github.com/apache/airflow/pull/44701#discussion_r1879836056 ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,31 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] Review Comment: I'm not sure if a serialized dag can have no tasks. I tried locally and apparently empty dags (without tasks defined) are not saved to the db so I would say that `nodes` cannot be empty. But to be safe I still added a check for that, thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
pierrejeambrun commented on code in PR #44701: URL: https://github.com/apache/airflow/pull/44701#discussion_r1879836056 ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,31 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] Review Comment: I'm not sure if a serialized dag can have no tasks. I added a test and apparently the dag is not saved to the db so I would say that `nodes` cannot be empty. But to be safe I still added a check for that, thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]
shubhamraj-git commented on code in PR #44701: URL: https://github.com/apache/airflow/pull/44701#discussion_r1878624389 ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,31 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] Review Comment: Should we check for nodes or edges are empty? ## airflow/api_fastapi/core_api/routes/ui/structure.py: ## @@ -63,4 +63,31 @@ def structure_data( "edges": edges, } +if external_dependencies: +entry_node_ref = nodes[0] +exit_node_ref = nodes[-1] + +for dependency_dag_id, dependencies in SerializedDagModel.get_dag_dependencies().items(): +for dependency in dependencies: +if dependency_dag_id != dag_id and dependency.target != dag_id: +continue + +# Add nodes +nodes.append( +{ +"id": dependency.node_id, +"label": dependency.node_id, +"type": dependency.dependency_type, +} +) + +# Add edges +# start dependency +if dependency.target != dependency.dependency_type: +edges.insert(0, {"source_id": dependency.node_id, "target_id": entry_node_ref["id"]}) Review Comment: This operation shifts all elements in the list, might be expensive. Can we optimise it to something like ``` start_edges = [] end_edges = [] for . # Collect start and end edges if dependency.target != dependency.dependency_type: # Start dependency start_edges.append({"source_id": dependency.node_id, "target_id": entry_node_ref["id"]}) elif dependency.source != dependency.dependency_type: # End dependency end_edges.append({"source_id": exit_node_ref["id"], "target_id": dependency.node_id}) edges = start_edges + edges + end_edges ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org