You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "uranusjr (via GitHub)" <gi...@apache.org> on 2025/12/11 09:45:06 UTC
[I] Separate DeadlineAlert implementations in SDK and Core [airflow]
uranusjr opened a new issue, #59303:
URL: https://github.com/apache/airflow/issues/59303
### Body
I’ve been working on splitting SDK usages in Airflow Core (and vice versa, but removing SDK from Core is the current priority), see #52141. Most of the work is close to done, but I’m having some trouble isolating DeadlineAlert and related types. I’m hesitant to start working on them since development is still quite active around them and want to recruit some help from people more familiar with the progress.
Reading existing implementation I think we need to
1. Create Core-only deadline and callback types that are used in scheduler etc. These should live in `airflow.serialization.definitions`.
2. Move serialization to `airflow.serialization.encoders`.
3. Move deserialization to `airflow.serialization.decoders`; this can’t reference SDK types.
4. Remove `airflow.models.deadline` references in SDK. We should change DeadlineReference declarations to not use ReferenceModels, and create SerializedDeadlineReference types in the scheduler to reference ReferenceModels.
5. ReferenceModels should be moved out of `airflow.models` to ``airflow.serialization.definitions`.
I’m not entirely familiar to the current code structure and may be missing something. I’m also not sure if there are other plans to change those types and functions that may affect this. Please feel free to provide inputs on this @ferruzzi @ramitkataria (please also tag others involved).
### Committer
- [x] I acknowledge that I am a maintainer/committer of the Apache Airflow project.
--
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-unsubscribe@airflow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3663865723
Yeah I’ll try to provide what I can
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3709046790
Hello @uranusjr @ferruzzi I see that the linked PR: https://github.com/apache/airflow/pull/30125 is merged. But is that the wrong one? How are we doing on this one?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3712500394
Where are you seeing #30125? I think you may be looking at different issues?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "bil-ly (via GitHub)" <gi...@apache.org>.
bil-ly commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3647624860
@uranusjr Can i work on this issue?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3653689232
Indeed the main issue is ReferenceModels usages in the SDK. The `_deadline` column itself is not an issue, but the `deadline` property that uses it is. DagModel will not be able to reference serde logic and SDK classes when used in the scheduler, so that should be removed, or at least moved into the dag processor. That should be relatively minor once the ReferenceModels situation is resolved.
The two-step process should be fine as long as SDK classes are only used in the dag processor (this should be the case from your description).
I’ll hold off work around this area then. Feel free to send me a word if you need some info on what should be OK and what wouldn’t.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3713418915
> I thought I did a reasonable job separating the interests into SDk and public, but I guess not. Sorry about that. I have [a PR up](https://github.com/apache/airflow/pull/30125) which strips deadlines out of the Serialized Dag and I would greatly appreciate it if you held off until that was merged so I don't have to redo that work. But AFAIK that should be the last major refactor on the Deadlines code. I know [@ramitkataria](https://github.com/ramitkataria) and [@seanghaeli](https://github.com/seanghaeli) have Callback work they are actively working on, but I don't know offhand how that will be affected by the move. It might be better for you to move ahead and them/us to adapt, but I'll let them weigh in on that hopefully.
>
> I think your reorganization plan looks right, but just to be sure, here is the workflow after the above PR is merged:
>
> * User defines a DAG() with `deadline=DeadlineAlert()`
> * That gets stored in the DAG object as `deadline: list[DeadlineAlert]`
> * ((Changes start here)): When the DAG is serialized, each DeadlineAlert is written to the deadline_alert table and the serialized_dag stores a list[str] containing the table row reference instead of storing the entire serialized DeadlineAlert, and only fetched/expanded from the UUID when needed (usually at dagrun creation)
>
> I think your only real issue would be that task-sdk/src/airflow/sdk/definitions/deadline.py imports `from airflow.models.deadline import ReferenceModels` which shouldn't have crossed over? Other than maybe needing to untangle that, I think it's fairly straight-forward. On the SDk side you'd have
>
> ```
> SDK (task-sdk/):
> ├─ deadline.py
> ├─ DeadlineAlert (user-facing, no changes)
> └─ DeadlineReference (public interface, needs to NOT import ReferenceModels)
> ```
>
> and on the Core side:
>
> ```
> Core (airflow-core/src/airflow/):
> ├─ serialization/definitions/
> ├─ deadline.py
> │ ├─ SerializedDeadlineAlert
> │ └─ SerializedDeadlineReference ((I think maybe wrapping ReferenceModels here would untangle the cross-referencing??? Maybe???))
> └─ reference_models.py
> └─ Move the DeadlineReference implementation out of models/deadline.py?)
> │
> ├─ serialization/ ((new - pull these out of serialized_objects.py))
> ├─ encoders/deadline.py
> └─ decoders/deadline.py
> │
> └─ models/
> ├─ deadline.py (keep Deadline ORM model, remove ReferenceModels)
> └─ deadline_alert.py (keep DeadlineAlert ORM model)
> ```
>
> One thing that may simplify (or maybe complicate?) things after PR [#58248](https://github.com/apache/airflow/pull/58248) is that deserialization will now be a 2-step process and we only deserialize "on demand":
>
> * Step 1 - deserialize DAG: `dag.deadline` stays as `list[str]` of UUIDs - no Core types needed
> * Step 2 - expand on use: `_process_dagrun_deadline_alerts()` expands UUIDs to full objects - uses Core types
>
> I'm not entirely sure how that would look; maybe it'll simplify things for you.
>
> I think the only other major thing I have in mind for Deadlines was based on a comment in another PR that suggested renaming some internal stuff, which I guess would make sense after you do your reorganization. But IFF you think it's cleaner to do it when you move them, I was going to internally rename `DeadlineAlerts` and related object names to `deadline_def` - Short for `deadline definition`. Currently the user defines the `DeadlineAlert` in their Dag definition, the `DeadlineAlert` gets stored in the dag object as `deadline` (which is now a list of deadlines, making it even more confusing) and the `deadline` is used to calculate a `deadline`. Naming got pretty confusing and it's much more intuitive seeing "The user defines a Deadline Alert, stored as `deadline_def`, and that definition is used to calculate a `deadline`". But as I said, that's likely better as an entirely separate project after the move, in order to keep the changes easier to follow.
>
> Also, once PR [#58248](https://github.com/apache/airflow/pull/58248) is merged, I think we can finally safely drop `dag._deadline` entirely and that should simplify the split I think? That's likely not part of your split, but maybe it could make it easier?
@ferruzzi I saw it in this comment, maybe it was added by mistake?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3844939141
I took an initial stab at this, will do again at the remaining occurrences this week or the next
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3648219147
I thought I did a reasonable job separating the interests into SDk and public, but I guess not. Sorry about that. I have [a PR up](https://github.com/apache/airflow/pull/30125) which strips deadlines out of the Serialized Dag and I would greatly appreciate it if you held off until that was merged so I don't have to redo that work. But AFAIK that should be the last major refactor on the Deadlines code. I know @ramitkataria and @seanghaeli have Callback work they are actively working on, but I don't know offhand how that will be affected by the move. It might be better for you to move ahead and them/us to adapt, but I'll let them weigh in on that hopefully.
I think your reorganization plan looks right, but just to be sure, here is the workflow after the above PR is merged:
- User defines a DAG() with `deadline=DeadlineAlert()`
- That gets stored in the DAG object as `deadline: list[DeadlineAlert]`
- ((Changes start here)): When the DAG is serialized, each DeadlineAlert is written to the deadline_alert table and the serialized_dag stores a list[str] containing the table row reference instead of storing the entire serialized DeadlineAlert, and only fetched/expanded from the UUID when needed (usually at dagrun creation)
I think your only real issue would be that task-sdk/src/airflow/sdk/definitions/deadline.py imports `from airflow.models.deadline import ReferenceModels` which shouldn't have crossed over? Other than maybe needing to untangle that, I think it's fairly straight-forward. On the SDk side you'd have
```
SDK (task-sdk/):
├─ deadline.py
├─ DeadlineAlert (user-facing, no changes)
└─ DeadlineReference (public interface, needs to NOT import ReferenceModels)
```
and on the Core side:
```
Core (airflow-core/src/airflow/):
├─ serialization/definitions/
├─ deadline.py
│ ├─ SerializedDeadlineAlert
│ └─ SerializedDeadlineReference ((I think maybe wrapping ReferenceModels here would untangle the cross-referencing??? Maybe???))
└─ reference_models.py
└─ Move the DeadlineReference implementation out of models/deadline.py?)
│
├─ serialization/ ((new - pull these out of serialized_objects.py))
├─ encoders/deadline.py
└─ decoders/deadline.py
│
└─ models/
├─ deadline.py (keep Deadline ORM model, remove ReferenceModels)
└─ deadline_alert.py (keep DeadlineAlert ORM model)
```
One thing that may simplify (or maybe complicate?) things after PR https://github.com/apache/airflow/pull/58248 is that deserialization will now be a 2-step process and we only deserialize "on demand":
- Step 1 - deserialize DAG: `dag.deadline` stays as `list[str]` of UUIDs - no Core types needed
- Step 2 - expand on use: `_process_dagrun_deadline_alerts()` expands UUIDs to full objects - uses Core types
I'm not entirely sure how that would look; maybe it'll simplify things for you.
I think the only other major thing I have in mind for Deadlines was based on a comment in another PR that suggested renaming some internal stuff, which I guess would make sense after you do your reorganization. But IFF you think it's cleaner to do it when you move them, I was going to internally rename `DeadlineAlerts` and related object names to `deadline_def` - Short for `deadline definition`. Currently the user defines the `DeadlineAlert` in their Dag definition, the `DeadlineAlert` gets stored in the dag object as `deadline` (which is now a list of deadlines, making it even more confusing) and the `deadline` is used to calculate a `deadline`. Naming got pretty confusing and it's much more intuitive seeing "The user defines a Deadline Alert, stored as `deadline_def`, and that definition is used to calculate a `deadline`". But as I said, that's likely better as an entirely separate project after the move, in order to keep the changes easier to follow.
Also, once PR https://github.com/apache/airflow/pull/58248 is merged, I think we can finally safely drop `dag._deadline` entirely and that should simplify the split I think? That's likely not part of your split, but maybe it could make it easier?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3716773439
Sorry, I fell victim to my own terrible link-formatting habit. I searched for the number and couldn't find it.
Right now I have the main migration to extract the deadline out of the serialized dag into its own table, which now has a big conflict after the serdag refactor that happened over the holidays, but that is all I have currently in flight at this moment.
Sorry again for my confusion
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "ramitkataria (via GitHub)" <gi...@apache.org>.
ramitkataria commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3781724841
> I know @ramitkataria and @seanghaeli have Callback work they are actively working on, but I don't know offhand how that will be affected by the move.
Feel free to go ahead. There is no major deadline related changes from my end. I also likely won't have bandwidth to help with PRs for this issue for at least a few weeks but feel free to add me as a reviewer
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh closed issue #59303: Separate DeadlineAlert implementations in SDK and Core
URL: https://github.com/apache/airflow/issues/59303
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Separate DeadlineAlert implementations in SDK and Core [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on issue #59303:
URL: https://github.com/apache/airflow/issues/59303#issuecomment-3656771126
Sounds good. I've been trying to get a review on that work, if you happen to have the time it sounds like you'd have a good insight on it and we can prevent future work by doing ti right the first time??
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org