pierrejeambrun commented on code in PR #74040:
URL: https://github.com/apache/airflow/pull/74040#discussion_r4165740630
##########
airflow-core/src/airflow/dag_processing/processor.py:
##########
@@ -573,18 +573,17 @@ def in_process_api_server() -> InProcessExecutionAPI:
@attrs.define(kw_only=True)
-class DagFileProcessorProcess(WatchedSubprocess, LoggingMixin):
+class BaseDagFileProcessorProcess(WatchedSubprocess, LoggingMixin):
"""
- Parses dags with Task SDK API.
-
- This class provides a wrapper and management around a subprocess to parse
a specific DAG file.
+ Parse one Dag file in a child process for the Dag processor manager.
- Since DAGs are written with the Task SDK, we need to parse them in a task
SDK process such that
- we can use the Task SDK definitions when serializing. This prevents
potential conflicts with classes
- in core Airflow.
+ The child's output goes to the file's parse log, and its requests are
answered with
+ :attr:`client`. The parse is done once the child has exited and all its
sockets are closed;
+ :attr:`parsing_result` then holds what it sent. Subclasses start the child
and send it the
+ parse request.
"""
- logger_filehandle: BinaryIO
+ logger_filehandle: BinaryIO | None = None
Review Comment:
Should this attribute be removed from the base class completely? Other
langsdk process use sockets and won't use this. It feels weird to always have
this declared in the base class while only the python one will actually use it.
Maybe that should be moved down. Base class can use 'has attr' instead.
##########
airflow-core/src/airflow/dag_processing/processor.py:
##########
@@ -573,18 +573,17 @@ def in_process_api_server() -> InProcessExecutionAPI:
@attrs.define(kw_only=True)
-class DagFileProcessorProcess(WatchedSubprocess, LoggingMixin):
+class BaseDagFileProcessorProcess(WatchedSubprocess, LoggingMixin):
"""
- Parses dags with Task SDK API.
-
- This class provides a wrapper and management around a subprocess to parse
a specific DAG file.
+ Parse one Dag file in a child process for the Dag processor manager.
- Since DAGs are written with the Task SDK, we need to parse them in a task
SDK process such that
- we can use the Task SDK definitions when serializing. This prevents
potential conflicts with classes
- in core Airflow.
+ The child's output goes to the file's parse log, and its requests are
answered with
+ :attr:`client`. The parse is done once the child has exited and all its
sockets are closed;
+ :attr:`parsing_result` then holds what it sent. Subclasses start the child
and send it the
+ parse request.
"""
- logger_filehandle: BinaryIO
+ logger_filehandle: BinaryIO | None = None
Review Comment:
Should this attribute be removed from the base class completely? Other
langsdk process use sockets and won't use this. It feels weird to always have
this declared in the base class while only the python one will actually use it.
Maybe that should be moved down. Base class can use 'has attr' instead if
necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
