uranusjr commented on code in PR #70805:
URL: https://github.com/apache/airflow/pull/70805#discussion_r3732728800
##########
task-sdk/tests/task_sdk/coordinators/executable/test_coordinator.py:
##########
@@ -370,13 +370,43 @@ def test_executables_root_accepts_list(self, tmp_path):
coordinator = ExecutableCoordinator(executables_root=[str(tmp_path),
other])
assert coordinator.executables_root == [tmp_path, other]
- def test_executables_root_required(self):
- with pytest.raises(TypeError, match="executables_root"):
- ExecutableCoordinator()
-
- def test_executables_root_must_be_non_empty(self):
- with pytest.raises(ValueError, match="executables_root"):
- ExecutableCoordinator(executables_root=None)
+ def test_executables_root_optional_defaults_to_empty(self):
+ # Neither an explicit root nor dag_bundle_name: co-located mode, valid.
+ coordinator = ExecutableCoordinator()
+ assert coordinator.executables_root == []
+ assert coordinator.dag_bundle_name is None
+
+ def test_none_executables_root_normalized_to_empty(self):
+ coordinator = ExecutableCoordinator(executables_root=None)
+ assert coordinator.executables_root == []
+
+ def test_root_and_dag_bundle_name_are_mutually_exclusive(self, tmp_path):
+ with pytest.raises(ValueError, match="at most one of
'executables_root' or 'dag_bundle_name'"):
+ ExecutableCoordinator(executables_root=[tmp_path],
dag_bundle_name="artifacts")
+
+ @patch("airflow.dag_processing.bundles.manager.DagBundlesManager")
+ def test_unconfigured_dag_bundle_name_raises(self, mock_manager):
+ mock_manager.is_bundle_configured.return_value = False
+ with pytest.raises(ValueError, match="unconfigured Dag bundle
'ghost'"):
+ ExecutableCoordinator(dag_bundle_name="ghost")
+
+ @patch("airflow.dag_processing.bundles.manager.DagBundlesManager")
+ def test_configured_dag_bundle_name_accepted(self, mock_manager):
+ mock_manager.is_bundle_configured.return_value = True
+ coordinator = ExecutableCoordinator(dag_bundle_name="artifacts")
+ assert coordinator.dag_bundle_name == "artifacts"
+ mock_manager.is_bundle_configured.assert_called_once_with("artifacts")
+
+ def test_build_command_scans_passed_roots_in_colocated_mode(self,
tmp_path):
+ # Co-located mode: no configured root, so the subclass must scan the
roots
+ # the base hands in rather than a configured executables_root field.
Review Comment:
This same comment appears three times in this PR. Not useful. I would add a
comment somewhere in the implementation (not tests) to explain what `colocated`
means, and remove all three explainations. A reader can find the comment easy
enough by the `colocated` part in the test name.
##########
task-sdk/src/airflow/sdk/coordinators/java/coordinator.py:
##########
@@ -200,16 +201,28 @@ class JavaCoordinator(SubprocessCoordinator):
jvm_args: list[str] = attrs.field(factory=list)
jars_root: list[pathlib.Path] = attrs.field(
converter=convert_roots,
- validator=attrs.validators.min_len(1),
+ factory=list,
)
main_class: str = ""
-
- def _build_execute_task_command(self, *, what: TaskInstance) ->
tuple[list[str], str | None]:
- jar = _JarInfo.find(self.jars_root, self.main_class)
+ _root_kwarg: ClassVar[str] = "jars_root"
+
+ @property
+ def _explicit_artifact_roots(self) -> list[pathlib.Path]:
+ return self.jars_root
+
+ def _build_execute_task_command(
+ self, *, what: TaskInstance, roots: list[pathlib.Path]
+ ) -> tuple[list[str], str | None]:
+ # TODO: Scanning a whole Dag bundle without an explicit main_class lets
+ # _JarInfo.find pick the first executable JAR in walk order, so
duplicate
+ # entrypoints across bundles resolve non-deterministically — the same
+ # duplicate-dag_id ambiguity the Python Dag path has. Reject it at the
+ # IMPORT_ERROR stage once AIP-85 exposes an interface to raise there.
Review Comment:
Too long and can go stale easily. One sentence with a reference to GH-71134
is enough (and more resistent to staling).
##########
task-sdk/src/airflow/sdk/coordinators/_subprocess.py:
##########
@@ -385,23 +400,148 @@ class SubprocessCoordinator(BaseCoordinator):
:param task_startup_timeout: Maximum time the coordinator waits for the
subprocess to connect to both servers, in seconds. The default is 10
seconds.
+ :param dag_bundle_name: Locate artifacts through a configured Dag bundle
rather
+ than an explicit root. Mutually exclusive with the subclass's explicit
root;
+ if neither is set, the task's own bundle is used. A named bundle
resolves to
+ its latest version; the task's own bundle is pinned to the run's
version.
"""
task_startup_timeout: float = 10.0
+ dag_bundle_name: str | None = None
+
+ # Name of the subclass's explicit-root kwarg, used only in error messages.
+ # Subclasses that expose an explicit root set this and override
+ # :meth:`_explicit_artifact_roots`; a subclass that does neither lands on
the
+ # task-bundle default instead of failing at execute time.
+ _root_kwarg: ClassVar[str] = "root"
+
+ # Classified once at construction by :meth:`_classify_artifact_source` and
+ # dispatched on by :meth:`_init_root_source` at execute time.
+ _artifact_source: _ArtifactSource | None = attrs.field(init=False,
default=None)
+ # The subclass's explicit root, recorded at construction so the base can
+ # resolve roots without knowing the subclass field name.
+ _configured_roots: list[pathlib.Path] = attrs.field(init=False,
factory=list)
+ # The task's own bundle, bound for the duration of a single
:meth:`execute_task`
+ # call by :meth:`_set_current_bundle` so :meth:`_init_root_source` can
resolve
+ # co-located artifacts.
Review Comment:
Same
##########
task-sdk/src/airflow/sdk/coordinators/_subprocess.py:
##########
@@ -385,23 +400,148 @@ class SubprocessCoordinator(BaseCoordinator):
:param task_startup_timeout: Maximum time the coordinator waits for the
subprocess to connect to both servers, in seconds. The default is 10
seconds.
+ :param dag_bundle_name: Locate artifacts through a configured Dag bundle
rather
+ than an explicit root. Mutually exclusive with the subclass's explicit
root;
+ if neither is set, the task's own bundle is used. A named bundle
resolves to
+ its latest version; the task's own bundle is pinned to the run's
version.
"""
task_startup_timeout: float = 10.0
+ dag_bundle_name: str | None = None
+
+ # Name of the subclass's explicit-root kwarg, used only in error messages.
+ # Subclasses that expose an explicit root set this and override
+ # :meth:`_explicit_artifact_roots`; a subclass that does neither lands on
the
+ # task-bundle default instead of failing at execute time.
+ _root_kwarg: ClassVar[str] = "root"
+
+ # Classified once at construction by :meth:`_classify_artifact_source` and
+ # dispatched on by :meth:`_init_root_source` at execute time.
Review Comment:
Arguably useless comment
--
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]
