Re: [PR] Adding Task ids in DAG details API endpoint #37564 [airflow]
potiuk merged PR #37866: URL: https://github.com/apache/airflow/pull/37866 -- 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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on PR #37866: URL: https://github.com/apache/airflow/pull/37866#issuecomment-2041097727 Working on new Implementation -- 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] Adding Task ids in DAG details API endpoint #37564 [airflow]
potiuk commented on PR #37866: URL: https://github.com/apache/airflow/pull/37866#issuecomment-2040760457 tests failing -- 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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on PR #37866:
URL: https://github.com/apache/airflow/pull/37866#issuecomment-2016869645
> If we call the field `tasks`, users will expect more information, not just
`task_ids`. You may change the field to task_ids. In the future, we can
deprecate that and have `tasks` fields that would have more information about
the said tasks
@ephraimbuddy, that's a good suggestion. I will go ahead and rename the
field as you suggested. Regarding @hussein-awala's suggestion, should we
include only the task_ids as of now or have an identical response as the
/dags/{dag_id}/tasks endpoint?
--
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] Adding Task ids in DAG details API endpoint #37564 [airflow]
ephraimbuddy commented on PR #37866: URL: https://github.com/apache/airflow/pull/37866#issuecomment-2016608212 If we call the field `tasks`, users will expect more information, not just `task_ids`. You may change the field to task_ids. In the future, we can deprecate that and have `tasks` fields that would have more information about the said tasks -- 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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on PR #37866:
URL: https://github.com/apache/airflow/pull/37866#issuecomment-2016517181
> > We can always add more details to the tasks as you mentioned
>
> The problem with big projects like Airflow is maintaining backward
compatibility; returning more data in the future instead of a simple key could
be an issue for some users, and removing this simple key will be considered a
breaking change. In this case, we will need to add more useless parameters to
the query to determine whether to add the extra information.
>
@hussein-awala, I completely agree with your suggestion. What I meant by the
statement "We can always add more details to the tasks as you mentioned" is
that, in addition to task keys, we can include more information such as setup
and teardown etc.. in the response using just one parameter, rather than using
multiple parameters for extra information.
> I suggest adding a response identical to the get /dags/{dag_id}/tasks
endpoint response
Adding a response identical to the above endpoint is a nice idea. I will
implement the same. 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] Adding Task ids in DAG details API endpoint #37564 [airflow]
potiuk commented on PR #37866: URL: https://github.com/apache/airflow/pull/37866#issuecomment-2015121177 > @potiuk? Would you kindly review this request? Your input would be greatly appreciated. Thank you. I think it's worth waiting for those who already reviewed it. They migh have more context and provide better feedback. I cannot physically review every single PR here, and calling me directly in case you do not know if I am going to look at it is not a good idea. -- 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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on PR #37866: URL: https://github.com/apache/airflow/pull/37866#issuecomment-2015069441 @potiuk? Would you kindly review this request? Your input would be greatly appreciated. Thank you. -- 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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on code in PR #37866:
URL: https://github.com/apache/airflow/pull/37866#discussion_r1531733577
##
airflow/api_connexion/schemas/dag_schema.py:
##
@@ -149,6 +149,16 @@ def get_params(obj: DAG):
return {k: v.dump() for k, v in params.items()}
+class DAGDetailSchemaWithTasksInfo(DAGDetailSchema):
+"""DAG with task ids details."""
+
+tasks = fields.Method("get_tasks", dump_only=True)
+@staticmethod
+def get_tasks(obj: DAG) -> list[str]:
+"""Get DAG task ids."""
+return list(obj.task_dict.keys())
Review Comment:
@ephraimbuddy @pierrejeambrun @potiuk Can you please review the PR changes.
--
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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on code in PR #37866:
URL: https://github.com/apache/airflow/pull/37866#discussion_r1531733577
##
airflow/api_connexion/schemas/dag_schema.py:
##
@@ -149,6 +149,16 @@ def get_params(obj: DAG):
return {k: v.dump() for k, v in params.items()}
+class DAGDetailSchemaWithTasksInfo(DAGDetailSchema):
+"""DAG with task ids details."""
+
+tasks = fields.Method("get_tasks", dump_only=True)
+@staticmethod
+def get_tasks(obj: DAG) -> list[str]:
+"""Get DAG task ids."""
+return list(obj.task_dict.keys())
Review Comment:
@ephraimbuddy @pierrejeambrun @potiuk Can you please review the PR changes.
--
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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on code in PR #37866:
URL: https://github.com/apache/airflow/pull/37866#discussion_r1531733577
##
airflow/api_connexion/schemas/dag_schema.py:
##
@@ -149,6 +149,16 @@ def get_params(obj: DAG):
return {k: v.dump() for k, v in params.items()}
+class DAGDetailSchemaWithTasksInfo(DAGDetailSchema):
+"""DAG with task ids details."""
+
+tasks = fields.Method("get_tasks", dump_only=True)
+@staticmethod
+def get_tasks(obj: DAG) -> list[str]:
+"""Get DAG task ids."""
+return list(obj.task_dict.keys())
Review Comment:
@ephraimbuddy @pierrejeambrun Can you please review the PR the changes.
--
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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on code in PR #37866:
URL: https://github.com/apache/airflow/pull/37866#discussion_r1531733577
##
airflow/api_connexion/schemas/dag_schema.py:
##
@@ -149,6 +149,16 @@ def get_params(obj: DAG):
return {k: v.dump() for k, v in params.items()}
+class DAGDetailSchemaWithTasksInfo(DAGDetailSchema):
+"""DAG with task ids details."""
+
+tasks = fields.Method("get_tasks", dump_only=True)
+@staticmethod
+def get_tasks(obj: DAG) -> list[str]:
+"""Get DAG task ids."""
+return list(obj.task_dict.keys())
Review Comment:
@ephraimbuddy @pierrejeambrun Can you please review the PR as well.
--
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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on code in PR #37866:
URL: https://github.com/apache/airflow/pull/37866#discussion_r1510340473
##
airflow/api_connexion/schemas/dag_schema.py:
##
@@ -149,6 +149,16 @@ def get_params(obj: DAG):
return {k: v.dump() for k, v in params.items()}
+class DAGDetailSchemaWithTasksInfo(DAGDetailSchema):
+"""DAG with task ids details."""
+
+tasks = fields.Method("get_tasks", dump_only=True)
+@staticmethod
+def get_tasks(obj: DAG) -> list[str]:
+"""Get DAG task ids."""
+return list(obj.task_dict.keys())
Review Comment:
@hussein-awala, regarding your question about adding more details to the
tasks such as upstream lists, downstream lists, setup status, teardown status,
and other functionalities, it's feasible and beneficial.
We can always add more details to the tasks as you mentioned. I have written
flexible code to easily accommodate additional functionalities as part of the
"DAGDetailSchemaWithTasksInfo" schema, as per future requirements. Task IDs
play a crucial role, potentially saving numerous API calls, particularly when
dealing with a large number of DAGs if task IDs are included in the DAG details
API info. Also, all these by default won't come in an API response, details
will only come in case when "include_tasks" param is true.
I understand your stance of waiting for other reviews before making any
changes. Let's gather additional feedback to ensure alignment before proceeding
with modifications.
--
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] Adding Task ids in DAG details API endpoint #37564 [airflow]
msoni1369 commented on code in PR #37866:
URL: https://github.com/apache/airflow/pull/37866#discussion_r1510340473
##
airflow/api_connexion/schemas/dag_schema.py:
##
@@ -149,6 +149,16 @@ def get_params(obj: DAG):
return {k: v.dump() for k, v in params.items()}
+class DAGDetailSchemaWithTasksInfo(DAGDetailSchema):
+"""DAG with task ids details."""
+
+tasks = fields.Method("get_tasks", dump_only=True)
+@staticmethod
+def get_tasks(obj: DAG) -> list[str]:
+"""Get DAG task ids."""
+return list(obj.task_dict.keys())
Review Comment:
@hussein-awala, regarding your question about adding more details to the
tasks such as upstream lists, downstream lists, setup status, teardown status,
and other functionalities, it's feasible and beneficial.
We can always add more details to the tasks as you mentioned. I have written
flexible code to easily accommodate additional functionalities as part of the
"DAGDetailSchemaWithTasksInfo" schema, as per future requirements. Task IDs
play a crucial role, potentially saving numerous API calls, particularly when
dealing with a large number of DAGs, if task IDs are included in the DAG
details API info.
I understand your stance of waiting for other reviews before making any
changes. Let's gather additional feedback to ensure alignment before proceeding
with modifications.
--
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] Adding Task ids in DAG details API endpoint #37564 [airflow]
hussein-awala commented on code in PR #37866:
URL: https://github.com/apache/airflow/pull/37866#discussion_r1510331779
##
airflow/api_connexion/schemas/dag_schema.py:
##
@@ -149,6 +149,16 @@ def get_params(obj: DAG):
return {k: v.dump() for k, v in params.items()}
+class DAGDetailSchemaWithTasksInfo(DAGDetailSchema):
+"""DAG with task ids details."""
+
+tasks = fields.Method("get_tasks", dump_only=True)
+@staticmethod
+def get_tasks(obj: DAG) -> list[str]:
+"""Get DAG task ids."""
+return list(obj.task_dict.keys())
Review Comment:
I wonder if it is possible/useful to add more details to the tasks (upstream
list, downstream list, is setup? is teardown? ...)
I have no strong opinion on this, let's wait for other reviews before
changing it
--
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
