Add name fields to SDK deadline alerts#64926
Add name fields to SDK deadline alerts#64926imrichardwu wants to merge 12 commits intoapache:mainfrom
Conversation
Updated deadline alert messages for clarity and consistency.
bbovenzi
left a comment
There was a problem hiding this comment.
Let's remove the i18n json and browse tab changes. Those are better to stay with their UI branches.
Also, given that we made the interval + reference more readable in your other PR. I feel like description is no longer necessary. Let's just do name
providers/common/ai/src/airflow/providers/common/ai/plugins/www/__init__.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Propagates DeadlineAlert.name through SDK/core serialization into the metadata DB and adjusts the UI API/OpenAPI shapes for deadline-related responses.
Changes:
- Add optional
nameto the Task SDKDeadlineAlertand include it in core serialization encode/decode. - Persist
nameintoDeadlineAlertDB rows when creating alerts from serialized DAG data, and updatenamewhen reusing existing deadline UUIDs. - Remove
description/alert_descriptionfrom the UI API models and the generated UI OpenAPI client types; addDeadlinesto the UIMenuItemenum.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| task-sdk/src/airflow/sdk/definitions/deadline.py | Adds optional name to SDK DeadlineAlert. |
| airflow-core/src/airflow/serialization/encoders.py | Emits name in serialized deadline alert payloads. |
| airflow-core/src/airflow/serialization/decoders.py | Parses name from serialized deadline alert payloads. |
| airflow-core/src/airflow/serialization/definitions/deadline.py | Adds field constant + name to SerializedDeadlineAlert. |
| airflow-core/src/airflow/models/serialized_dag.py | Writes/updates DeadlineAlertModel.name when persisting serialized DAGs. |
| airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py | Removes description fields from UI API response models. |
| airflow-core/src/airflow/api_fastapi/common/types.py | Adds DEADLINES to MenuItem. |
| airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml | Removes description fields from schemas; adds Deadlines to menu enum. |
| airflow-core/src/airflow/ui/openapi-gen/requests/types.gen.ts | Regenerated client types (drops description fields; adds Deadlines). |
| airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts | Regenerated client schemas (drops description fields; adds Deadlines). |
| airflow-core/tests/unit/models/test_deadline_alert.py | Removes assertions/data for description. |
| airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_deadlines.py | Removes alert_description assertions and related test naming/docs. |
Comments suppressed due to low confidence (1)
airflow-core/src/airflow/serialization/encoders.py:218
encode_deadline_alert()uses a literal "name" key while the decoder and other call sites useDeadlineAlertFields.NAME. For consistency and to avoid drift/typos, consider usingDeadlineAlertFields.NAMEhere as well.
from airflow.sdk.serde import serialize
return {
"name": getattr(d, "name", None),
"reference": encode_deadline_reference(d.reference),
"interval": d.interval.total_seconds(),
"callback": serialize(d.callback),
}
There was a problem hiding this comment.
Overall looks good to me, just one nit.
I'll leave the final word to @ferruzzi on the name update logic.
| CONFIG = "Config" | ||
| CONNECTIONS = "Connections" | ||
| DAGS = "Dags" | ||
| DEADLINES = "Deadlines" |
There was a problem hiding this comment.
Are you using this? Because if we do not adjust permissions associated to this, this won't be useful.
There was a problem hiding this comment.
Yes this should only be included once we have a deadlines page in the browse tab
There was a problem hiding this comment.
Just wondering, is the reason for doing it later to prevent bloat, in case I do not end up working on the deadline page task?
|
Why are we removing description? I think other than that it looks fine from my side |
We are removing description because we decided that we should not rely on description in telling the users what the deadline and deadline alerts are. |
|
I'm not sure I follow. If the user wants to set a description, why not let them? Let's say we work for Big Company. The person in Marketing who is using the deadline may not be the dev/admin person who wrote the code for the deadline. Having a description seems reasonable to me. |
Having interval + reference and name should provide enough detail about the deadline without the ui feeling bloated. Because I did add description before and it felt like there was too much info and a bit confusing. |
amoghrajesh
left a comment
There was a problem hiding this comment.
Need clarification on why we are dropping this as it looks to be a breaking change. If this is intentional, we will need a newsfragment
|
|
||
| id: UUID | ||
| name: str | None = None | ||
| description: str | None = None |
There was a problem hiding this comment.
Why are we dropping this? This will qualify as a breaking change for clients consuming this rest API since it was sent across in Airflow <= 3.2 API response?
There was a problem hiding this comment.
I added this in aprevious pr. No one should have used the api yet, so it should be ok. I initially decided to add a description because I thought the same thing on what @ferruzzi was saying above, but after talking to @bbovenzi in my ui pr, we decided to drop it. If I made any mistake in my judgement @bbovenzi please correct me.
There was a problem hiding this comment.
I see, thanks for clarifying that.
There was a problem hiding this comment.
If it didnt make it in a release, I am ok
There was a problem hiding this comment.
If 3.2.1 hasn't been released yet it should be good
Changes that I flagged didn't make it in a release
Adds name and alert id fields to DeadlineAlert, exposed through the API. (json edited files are for ui change for my pr in the future) @bbovenzi
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.