Re: [PR] AIP-84 Add external dependencies to GET Structure Data Endpoint [airflow]

2024-12-12 Thread via GitHub


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]

2024-12-12 Thread via GitHub


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]

2024-12-12 Thread via GitHub


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]

2024-12-12 Thread via GitHub


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]

2024-12-12 Thread via GitHub


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]

2024-12-11 Thread via GitHub


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]

2024-12-11 Thread via GitHub


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]

2024-12-11 Thread via GitHub


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]

2024-12-10 Thread via GitHub


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