Re: [PR] Add prek hook to keep supervisor schema class name in sync with its `type` literal [airflow]
jason810496 commented on code in PR #66899: URL: https://github.com/apache/airflow/pull/66899#discussion_r3239491745 ## task-sdk/src/airflow/sdk/execution_time/comms.py: ## Review Comment: Good idea! Having it as unit tests will be much simpler than the prek hook. (CI will raise the error anyway even the drift will not be caught in the first place when commit 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add prek hook to keep supervisor schema class name in sync with its `type` literal [airflow]
amoghrajesh commented on code in PR #66899:
URL: https://github.com/apache/airflow/pull/66899#discussion_r3239356906
##
task-sdk/src/airflow/sdk/execution_time/comms.py:
##
@@ -509,7 +509,7 @@ def from_xcom_response(cls, xcom_response: XComResponse) ->
XComResult:
class XComCountResponse(BaseModel):
len: int
-type: Literal["XComLengthResponse"] = "XComLengthResponse"
+type: Literal["XComCountResponse"] = "XComCountResponse"
Review Comment:
Ah symmetrically wrong!
##
dev/breeze/doc/images/output_pr_auto-triage.svg:
##
Review Comment:
Unrelated change?
##
task-sdk/src/airflow/sdk/execution_time/comms.py:
##
@@ -869,7 +869,7 @@ class GetXComCount(BaseModel):
dag_id: str
run_id: str
task_id: str
-type: Literal["GetNumberXComs"] = "GetNumberXComs"
+type: Literal["GetXComCount"] = "GetXComCount"
Review Comment:
Yeah I think we are safe here, but just to be sure, can you check if go-sdk
is fine reference wise?
##
task-sdk/src/airflow/sdk/execution_time/comms.py:
##
Review Comment:
Just wondering if you thought of a unit test covering this would be simpler
to maintain compared to a prek hook?
Something like:
```python
def test_supervisor_schema_type_literals_match_class_names():
from typing import Annotated, get_args, get_origin
from airflow.sdk.execution_time.comms import ToTask, ToSupervisor
for union_name, union in [("ToTask", ToTask), ("ToSupervisor",
ToSupervisor)]:
union_type = get_args(union)[0] if get_origin(union) is Annotated
else union
for member in get_args(union_type):
if not isinstance(member, type):
continue
field = member.model_fields.get("type")
assert field is not None, f"{member.__name__} in {union_name}
has no `type` field"
args = get_args(field.annotation)
assert len(args) == 1 and args[0] == member.__name__, (
f"{member.__name__}.type literal is {args[0]!r}, expected
{member.__name__!r}"
)
```
--
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] Add prek hook to keep supervisor schema class name in sync with its `type` literal [airflow]
jason810496 commented on code in PR #66899: URL: https://github.com/apache/airflow/pull/66899#discussion_r3239274858 ## task-sdk/src/airflow/sdk/execution_time/comms.py: ## @@ -869,7 +869,7 @@ class GetXComCount(BaseModel): dag_id: str run_id: str task_id: str -type: Literal["GetNumberXComs"] = "GetNumberXComs" +type: Literal["GetXComCount"] = "GetXComCount" Review Comment: I made the `type` literal field here in sync with class name. Additionally, there isn't any Lang-SDK references the supervisor schema so far, so this _should_ not impact anything IMO. -- 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] Add prek hook to keep supervisor schema class name in sync with its `type` literal [airflow]
jason810496 commented on code in PR #66899: URL: https://github.com/apache/airflow/pull/66899#discussion_r3239274858 ## task-sdk/src/airflow/sdk/execution_time/comms.py: ## @@ -869,7 +869,7 @@ class GetXComCount(BaseModel): dag_id: str run_id: str task_id: str -type: Literal["GetNumberXComs"] = "GetNumberXComs" +type: Literal["GetXComCount"] = "GetXComCount" Review Comment: I made the `type` here in sync with class name. Additionally, there isn't any Lang-SDK references the supervisor schema so far, so this _should_ not impact anything IMO. -- 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]
