Swap Dag Parsing to use the TaskSDK machinery. by ashb · Pull Request #44972 · apache/airflow

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb · 2024-12-16T23:25:04Z

As part of Airflow 3 DAG definition files will have to use the Task SDK for
all their classes, and anything involving running user code will need to be
de-coupled from the database in the user-code process.

This change moves all of the "serialization" change up to the
DagFileProcessorManager, using the new function introduced in #44898 and the
"subprocess" machinery introduced in #44874.

Important Note: this change does not remove the ability for dag processes
to access the DB for Variables etc. That will come in a future change.

Some key parts of this change:

It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.
This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)
A number of classes that we need to send between processes have been
converted to Pydantic for ease of serialization.
In order to not have to serialize everything in the subprocess and deserialize everything
in the parent Manager process, we have created a LazyDeserializedDAG class
that provides lazy access to much of the properties needed to create update
the DAG related DB objects, without needing to fully deserialize the entire
DAG structure.
Classes switched to attrs based for less boilerplate in constructors.
Internal timers convert to time.monotonic where possible, and time.time
where not, we only need second diff between two points, not datetime objects
With the earlier removal of "sync mode" for SQLite in Remove "single process" restrictions on SQLite in favour of using WAL mode #44839 the need for
separate TERMIANTE and END messages over the control socket can go

Co-authored-by: Jed Cunningham 66968678+jedcunningham@users.noreply.github.com
Co-authored-by: Daniel Imberman daniel.imberman@gmail.com

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

ashb · 2024-12-16T23:25:47Z

The tests aren't 100% finished yet.

And this change is larger than I would have liked, but at least it's a net-negative change

ashb · 2024-12-16T23:49:38Z

I don't expect tests to pass yet, but I want to give people the chance to see this PR, and I know @jedcunningham is waiting on this for some of his DAG versioning work.

kaxil · 2024-12-17T06:59:15Z

Right now this puts a
nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK.

I think that is unavoidable, as user code will come from Task SDK

This will be addressed before release (we have talked about introducing a
new "apache-airflow-base-executor" dist where this subprocess+supervisor
could live, as the "execution_time" folder in the Task SDK is more a feature
of the executor, not of the TaskSDK itself)

Right, I think processor will have to depend on both Task SDK (user-facing code) + Base Executor dist -- after that separation

kaxil

Partial review, will be coming back to it in an hour

amoghrajesh

I mainly reviewed the processor and manager files. The other changes seem mostly reactive. Overall, I like the improvements, especially the reuse of the execution time machinery here. Few initial comments, nothing serious but mostly nits.

amoghrajesh · 2024-12-18T08:28:58Z

-
-class DagFileProcessor(LoggingMixin):
+@attrs.define()
+class DagFileProcessorProcess(WatchedSubprocess):


Yeah sounds good

ashb · 2024-12-18T23:09:02Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore

@kaxil Any ideas the best plan for that one?

ashb · 2024-12-18T23:09:26Z

(I still need to rename a class and file, but that is a non-meaningful/non-review-impacting change.

kaxil · 2024-12-19T08:43:53Z

Right, I think this should now pass the tests, the only thing I'm not sure about this is the xfail I've put for the "simple ti roundtrip exec config tests" -- Either we should remove it or make it work, but I'm not sure if we need to pass down executor config via TI anymore
@kaxil Any ideas the best plan for that one?

Since we are planning to handle callbacks via Executor/worker interface too -- don't think we need to pass it explicitly from TI, instead just handle it on the server side before sending TI/request to the worker.

kaxil

Some minor comments and the renaming of file & classes can happen in a separate PR too

ashb · 2024-12-19T12:01:34Z

I'm going to run this with full tests, I want the kube tests to see if there is something broken not covered by unit tests

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMIANTE and END messages over the control socket can go. Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

…e sense any more

ashb · 2024-12-19T12:18:19Z

+ assert "a.py" in resp.import_errors
+
+
+# @conf_vars({("logging", "dag_processor_log_target"): "stdout"})


Leaving these comments out for now as I want to overhaul the logging in #45072

As part of Airflow 3 DAG definition files will have to use the Task SDK for all their classes, and anything involving running user code will need to be de-coupled from the database in the user-code process. This change moves all of the "serialization" change up to the DagFileProcessorManager, using the new function introduced in apache#44898 and the "subprocess" machinery introduced in apache#44874. **Important Note**: this change does not remove the ability for dag processes to access the DB for Variables etc. That will come in a future change. Some key parts of this change: - It builds upon the WatchedSubprocess from the TaskSDK. Right now this puts a nasty/unwanted depenednecy between the Dag Parsing code upon the TaskSDK. This will be addressed before release (we have talked about introducing a new "apache-airflow-base-executor" dist where this subprocess+supervisor could live, as the "execution_time" folder in the Task SDK is more a feature of the executor, not of the TaskSDK itself.) - A number of classes that we need to send between processes have been converted to Pydantic for ease of serialization. - In order to not have to serialize everything in the subprocess and deserialize everything in the parent Manager process, we have created a `LazyDeserializedDAG` class that provides lazy access to much of the properties needed to create update the DAG related DB objects, without needing to fully deserialize the entire DAG structure. - Classes switched to attrs based for less boilerplate in constructors. - Internal timers convert to `time.monotonic` where possible, and `time.time` where not, we only need second diff between two points, not datetime objects. - With the earlier removal of "sync mode" for SQLite in apache#44839 the need for separate TERMINATE and END messages over the control socket can go. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> Co-authored-by: Daniel Imberman <daniel.imberman@gmail.com>

ashb added area:Scheduler including HA (high availability) scheduler area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK area:task-sdk labels Dec 16, 2024

boring-cyborgBot added area:CLI area:serialization labels Dec 16, 2024

ashb force-pushed the dag-parsing-uses-task-sdk branch from 32176b9 to 1215213Compare December 16, 2024 23:29

ashb commented Dec 16, 2024
View reviewed changes

Comment threadtests/listeners/test_dag_import_error_listener.py

ashb commented Dec 16, 2024
View reviewed changes

Comment threadairflow/dag_processing/manager.py

ashb commented Dec 16, 2024
View reviewed changes

Comment threadairflow/dag_processing/manager.py

ashb commented Dec 16, 2024
View reviewed changes

Comment threadairflow/dag_processing/manager.py

ashb commented Dec 16, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py

ashb commented Dec 16, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py

ashb commented Dec 16, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py

ashb force-pushed the dag-parsing-uses-task-sdk branch from 1215213 to 9f04c14Compare December 16, 2024 23:46

kaxil reviewed Dec 17, 2024
View reviewed changes

amoghrajesh reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/collection.py
Comment threadairflow/dag_processing/manager.py
Comment threadairflow/dag_processing/manager.py Outdated
Comment threadairflow/dag_processing/manager.py

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/manager.py

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/manager.py
Comment threadairflow/dag_processing/manager.py Outdated
Comment threadairflow/dag_processing/manager.py
Comment threadairflow/callbacks/callback_requests.py Outdated

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py Outdated

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py Outdated

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py Outdated

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/dag_processing/processor.py Outdated

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/serialization/serialized_objects.py Outdated

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadairflow/serialization/serialized_objects.py

kaxil reviewed Dec 17, 2024
View reviewed changes

Comment threadtests/dag_processing/test_manager.py

ashb force-pushed the dag-parsing-uses-task-sdk branch 2 times, most recently from 93b8d53 to 553049eCompare December 17, 2024 15:56

jedcunningham reviewed Dec 17, 2024
View reviewed changes

amoghrajesh reviewed Dec 18, 2024
View reviewed changes

ashb force-pushed the dag-parsing-uses-task-sdk branch 3 times, most recently from e61c595 to f6487c9Compare December 18, 2024 23:08

ashb marked this pull request as ready for review December 18, 2024 23:09

ashb requested review from XD-DENG, bolkedebruin and ephraimbuddy as code owners December 18, 2024 23:09

kaxil added the full tests needed We need to run full set of tests for this PR to merge label Dec 19, 2024

kaxil reviewed Dec 19, 2024
View reviewed changes

Comment threadairflow/dag_processing/collection.py
Comment threadairflow/serialization/serialized_objects.py

kaxil approved these changes Dec 19, 2024
View reviewed changes

Comment threadairflow/dag_processing/manager.py Outdated
Comment threadairflow/dag_processing/processor.py Outdated

ashband others added 2 commits December 19, 2024 12:02

Remove some tests about "roundtripping" a simple TI -- they don't mak…
b22af18
…e sense any more

ashb force-pushed the dag-parsing-uses-task-sdk branch from f6487c9 to b22af18Compare December 19, 2024 12:04

ashb commented Dec 19, 2024
View reviewed changes

ashb mentioned this pull request Dec 19, 2024
Tidy up DagFileProcessor logging (remove reload and double loading etc) by porting it to structlog #45072
Closed

ashb merged commit 8774f28 into apache:mainDec 19, 2024

ashb deleted the dag-parsing-uses-task-sdk branch December 19, 2024 14:19

ashb mentioned this pull request Dec 19, 2024
Correctly set owners on DagModel after TaskSDK parsing change #45094
Merged

amoghrajesh mentioned this pull request Jan 8, 2025
AIP-72: Add support to get Variables in task SDK to author tasks #45458
Closed

kaxil mentioned this pull request Aug 14, 2025
mp_start_method was removed via 11acefa462b6 #54502
Closed
2 tasks

FrankYang0529 mentioned this pull request Jun 24, 2026
Remove stale metrics no longer emitted from the metrics registry #68945
Merged
1 task