Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
ayush3singh commented on code in PR #50376:
URL: https://github.com/apache/airflow/pull/50376#discussion_r2083590789
##
providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:
##
@@ -444,6 +444,7 @@ class TaskInstanceInfo(InfoJsonEncodable):
includes = ["duration", "try_number", "pool", "queued_dttm", "log_url"]
casts = {
+"log_url": lambda ti: ti.log_url if getattr(ti, "log_url", -1) != -1
else None,
Review Comment:
Thanks @mobuchowski
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
mobuchowski commented on code in PR #50376:
URL: https://github.com/apache/airflow/pull/50376#discussion_r2083512761
##
providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:
##
@@ -444,6 +444,7 @@ class TaskInstanceInfo(InfoJsonEncodable):
includes = ["duration", "try_number", "pool", "queued_dttm", "log_url"]
casts = {
+"log_url": lambda ti: ti.log_url if getattr(ti, "log_url", -1) != -1
else None,
Review Comment:
We can just do `getattr(ti, "log_url", None)`, right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
amoghrajesh commented on code in PR #50376:
URL: https://github.com/apache/airflow/pull/50376#discussion_r2083385278
##
providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:
##
@@ -444,6 +444,7 @@ class TaskInstanceInfo(InfoJsonEncodable):
includes = ["duration", "try_number", "pool", "queued_dttm", "log_url"]
casts = {
+"log_url": lambda ti: ti.log_url if getattr(ti, "log_url", -1) != -1
else None,
Review Comment:
CC @mobuchowski @kacpermuda for review on this part.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
ayush3singh commented on code in PR #50376:
URL: https://github.com/apache/airflow/pull/50376#discussion_r2083271436
##
providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:
##
@@ -444,6 +444,7 @@ class TaskInstanceInfo(InfoJsonEncodable):
includes = ["duration", "try_number", "pool", "queued_dttm", "log_url"]
casts = {
+"log_url": lambda ti: ti.log_url if getattr(ti, "log_url", -1) != -1
else None,
Review Comment:
Hi @amoghrajesh
without this log_url gets included in the final object `TaskInstanceInfo`
and although it is set to none we have to adjust the utils testcases to add
`log_url` in assert statements as shown below
`
assert dict(TaskInstanceInfo(runtime_ti)) == {
"log_url": None,
`
and since log_url is now expected but its not available in Airflow 3.0.0 I
guess, so below compatibility test fails:
[provider distributions tests / Compat
3.0.0:P3.9](https://github.com/apache/airflow/actions/runs/14947763348/job/41993407704#logs)
if we make this change then the Test passes as it will keep a place holder
if not available.
I'm fairly new in contribution to airflow repo, please let me know if I'm
missing out on something.
Thanks for review comments
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
amoghrajesh commented on code in PR #50376:
URL: https://github.com/apache/airflow/pull/50376#discussion_r2083107941
##
providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:
##
@@ -444,6 +444,7 @@ class TaskInstanceInfo(InfoJsonEncodable):
includes = ["duration", "try_number", "pool", "queued_dttm", "log_url"]
casts = {
+"log_url": lambda ti: ti.log_url if getattr(ti, "log_url", -1) != -1
else None,
Review Comment:
Why do we need this one?
##
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##
@@ -158,6 +183,9 @@ def get_template_context(self) -> Context:
validated_params = process_params(self.task.dag, self.task,
dag_run_conf, suppress_exception=False)
+self.log_url = self.get_log_url_from_ti()
+self.mark_success_url = self.get_mark_success_url()
Review Comment:
Can we move this logic to "startup" instead? It doens't necessarily need to
have context in order to set these.
##
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##
@@ -137,6 +137,10 @@ class RuntimeTaskInstance(TaskInstance):
rendered_map_index: str | None = None
+log_url: str | None = None
+
+mark_success_url: str | None = None
Review Comment:
Also i do not understand the use case/ motivation behind this one
##
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##
@@ -137,6 +137,10 @@ class RuntimeTaskInstance(TaskInstance):
rendered_map_index: str | None = None
+log_url: str | None = None
+
+mark_success_url: str | None = None
Review Comment:
Let's please extract this one to a separate PR.
##
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##
@@ -1028,6 +1028,8 @@ def test_get_context_without_ti_context_from_server(self,
mocked_parse, make_ti_
_ti_context_from_server=None,
start_date=start_date,
)
+runtime_ti.log_url = runtime_ti.get_log_url_from_ti()
Review Comment:
Can we add a test that gets that asserts the log_url?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
ayush3singh commented on PR #50376: URL: https://github.com/apache/airflow/pull/50376#issuecomment-2868608840 @amoghrajesh please review 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
amoghrajesh commented on code in PR #50376: URL: https://github.com/apache/airflow/pull/50376#discussion_r2081061600 ## airflow-core/src/airflow/executors/workloads.py: ## @@ -71,6 +71,8 @@ class TaskInstance(BaseModel): context_carrier: dict | None = None queued_dttm: datetime | None = None +log_url: str | None = None Review Comment: Instead of passing it around, can we construct it on the supervisor side itself? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
ayush3singh commented on code in PR #50376: URL: https://github.com/apache/airflow/pull/50376#discussion_r2081262718 ## airflow-core/src/airflow/executors/workloads.py: ## @@ -71,6 +71,8 @@ class TaskInstance(BaseModel): context_carrier: dict | None = None queued_dttm: datetime | None = None +log_url: str | None = None Review Comment: I agree @amoghrajesh Thanks for the advise, I have added made changes to read everything on sdk instead of passing it from server -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
amoghrajesh commented on code in PR #50376: URL: https://github.com/apache/airflow/pull/50376#discussion_r2081096254 ## airflow-core/src/airflow/executors/workloads.py: ## @@ -71,6 +71,8 @@ class TaskInstance(BaseModel): context_carrier: dict | None = None queued_dttm: datetime | None = None +log_url: str | None = None Review Comment: Basically we have all the information in task_runner.py to construct the uri, lets do it there -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
amoghrajesh commented on code in PR #50376: URL: https://github.com/apache/airflow/pull/50376#discussion_r2081096254 ## airflow-core/src/airflow/executors/workloads.py: ## @@ -71,6 +71,8 @@ class TaskInstance(BaseModel): context_carrier: dict | None = None queued_dttm: datetime | None = None +log_url: str | None = None Review Comment: Basically we have all the information in task_runner.py to construct the uri, lets do it there, look in `get_template_context` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fixes issue RuntimeTaskInstance context does not contain log_url [airflow]
ayush3singh commented on PR #50376: URL: https://github.com/apache/airflow/pull/50376#issuecomment-2864552914 @tirkarthi for review and guidence -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
