You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/12/14 19:11:48 UTC
[GitHub] [airflow] ashb opened a new pull request #20297: Rename TaskMixin to DependencyMixin
ashb opened a new pull request #20297:
URL: https://github.com/apache/airflow/pull/20297
It was used on things that weren't tasks (such as XComArg and
EdgeModifier) so the name was in-correct, and I want to disambiguate
this from a concept I need to add for AIP-42 (Dynamic task mapping) of
things that actually _are_ Tasks or Task-like objects.
Since `Union[TaskMixin, Sequence[TaskMixin]]` was uses in so many places
I created a type alias for this of `DependencyMixinOrList`
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769624856
##########
File path: airflow/models/baseoperator.py
##########
@@ -1662,10 +1660,11 @@ def defer(
raise TaskDeferred(trigger=trigger, method_name=method_name, kwargs=kwargs, timeout=timeout)
-Chainable = Union[BaseOperator, "XComArg", EdgeModifier, "TaskGroup"]
+# TODO: Deprecate for Airflow 3.0
+Chainable = Union[DependencyMixin, Sequence[DependencyMixin]]
Review comment:
I don't think this alias is used anywhere beyond the usage a few lines below.
--
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
[GitHub] [airflow] uranusjr commented on pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#issuecomment-994854734
> Also I wish sphinxcontrib-spelling was clever enough to ignore class and module names from the code base
I suspect it is for `sphinx.ext.autodoc`, but `sphinx-autoapi`’s output is different.
--
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
[GitHub] [airflow] eladkal commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769044779
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
yeah we should keep the old file for backcompact (though I'd be surprised if users use this class in custom code?)
If we are going to have `mixins.py` it probably means that [skipmixin.py](https://github.com/apache/airflow/blob/main/airflow/models/skipmixin.py) should be moved there too?
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769018677
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
In a local change (that I separated this out from) I'm adding in a new "TaskLike" mixin.
We could change this from taskmixins to just mixins.py, but probably need to keep the old file for backcompat :(
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
WDYT @eladkal @dstandish ?
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769583714
##########
File path: airflow/models/baseoperator.py
##########
@@ -1662,10 +1660,11 @@ def defer(
raise TaskDeferred(trigger=trigger, method_name=method_name, kwargs=kwargs, timeout=timeout)
-Chainable = Union[BaseOperator, "XComArg", EdgeModifier, "TaskGroup"]
+# TODO: Deprecate for Airflow 3.0
+Chainable = Union[DependencyMixin, Sequence[DependencyMixin]]
Review comment:
Maybe it’s actually a good idea to keep using this name…? It’s unfortunately not super easy to deprecate a type alias.
--
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
[GitHub] [airflow] ashb commented on pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#issuecomment-994786935
> Also need to add `DependencyMixin` to spellings I think.
Yeah, I didn't get that locally. I wonder why.
(Also I wish sphinxcontrib-spelling was clever enough to ignore class and module names from the code base)
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769490499
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
Yeah I realised overnight that this is a case of RAS Syndrome (RAS = Redundant Acronym Syndrome 😁 )
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769023300
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
+DependencyMixinOrList = Union["DependencyMixin", Sequence["DependencyMixin"]]
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: DependencyMixinOrList):
Review comment:
yeah long way off, true
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769046987
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
if we're renaming the class anyway what do we need the old module for 🤔
unless you're aliasing `TaskMixin=DependencyMixin` somewhere for compat?
unrelated but i am a tad skeptical of name TaskLike but was waiting to see what the class looks like before trying to think of any alternatives... i know maybe there's not gonna be a better option. but ideally we could convey something about the quality of the "likeness" because e.g. what if we compose with multiple classes that are "task-like" in their own indepedent way.... e.g. TaskLike1 and TaskLike2
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769019607
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
+DependencyMixinOrList = Union["DependencyMixin", Sequence["DependencyMixin"]]
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: DependencyMixinOrList):
Review comment:
Mmmmm, that's 3.9 only isnt it, so a way off, but I think I agree on this alias not helping.
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769033259
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
+DependencyMixinOrList = Union["DependencyMixin", Sequence["DependencyMixin"]]
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: DependencyMixinOrList):
Review comment:
https://mypy.readthedocs.io/en/stable/runtime_troubles.html#future-annotations-import-pep-563 -- form 3.7 and from-future-annotations we can have it I think! Which is just weeks away.
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r768968186
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
+DependencyMixinOrList = Union["DependencyMixin", Sequence["DependencyMixin"]]
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: DependencyMixinOrList):
Review comment:
I'm not 100% sold on this name, but it felt very verbose the number of places we had `Union["TaskMixin", Sequence["TaskMixin"]]` used.
Is this actually any easier to understand, or is the indirection just hiding this and making it harder to grok?
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769056243
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
> unrelated but i am a tad skeptical of name TaskLike
Yeah, it's not the _best_ name, espeically as it will probably include TaskGroups. It's "a node in the DAG graph, so either a Task/BaseOperator (mapped or unmapped) or a TaskGroup (again, mapped or unmapped)" -- so it will have `dag` properties, and a few other things like upstream/downstream relations.
More than happy for better name ideas!
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769057446
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
"DagGraphNode" is an option, but in some ways it means as little as "TaskLike"
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769125963
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,76 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
-
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Set a task or a task list to be directly upstream from the current task."""
raise NotImplementedError()
@abstractmethod
- def set_downstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_downstream(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Set a task or a task list to be directly downstream from the current task."""
raise NotImplementedError()
- def update_relative(self, other: "TaskMixin", upstream=True) -> None:
+ def update_relative(self, other: "DependencyMixin", upstream=True) -> None:
"""
Update relationship information about another TaskMixin. Default is no-op.
Override if necessary.
"""
- def __lshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __lshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Implements Task << Task"""
self.set_upstream(other)
return other
- def __rshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Implements Task >> Task"""
self.set_downstream(other)
return other
- def __rrshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rrshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Called for Task >> [Task] because list don't have __rshift__ operators."""
self.__lshift__(other)
return self
- def __rlshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rlshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Called for Task << [Task] because list don't have __lshift__ operators."""
self.__rshift__(other)
return self
+
+
+class TaskMixin(DependencyMixin):
+ """:meta private:"""
+
+ def __init_subclass__(cls) -> None:
Review comment:
Ah yes makes sense thank you
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769003523
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
may need to rename the module too
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769016318
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
+DependencyMixinOrList = Union["DependencyMixin", Sequence["DependencyMixin"]]
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: DependencyMixinOrList):
Review comment:
one vote for verbose in this case
eventually it would become `"TaskMixin" | Sequence["TaskMixin"]` which maybe ain’t as bad?
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769018677
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
In a local change (that I separated this out from) I'm adding in a new "TaskLike" mixin.
We could change this from taskmixins to just mixins, but probably need to keep the old file for backcompat :(
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#issuecomment-994749638
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] uranusjr commented on pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#issuecomment-994750094
Also need to add `DependencyMixin` to spellings I think.
--
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
[GitHub] [airflow] ashb merged pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #20297:
URL: https://github.com/apache/airflow/pull/20297
--
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
[GitHub] [airflow] eladkal commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769015993
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
does it make sense to keep the file name `taskmixin.py` ?
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769048753
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
> unless you're aliasing TaskMixin=DependencyMixin somewhere for compat?
https://github.com/apache/airflow/pull/20297/files#diff-30230355a7c5c56d9bc0b6025e028d5bd5bc500383093de4bde01f5af522eb47R81-R90
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769102609
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,76 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
-
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Set a task or a task list to be directly upstream from the current task."""
raise NotImplementedError()
@abstractmethod
- def set_downstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_downstream(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Set a task or a task list to be directly downstream from the current task."""
raise NotImplementedError()
- def update_relative(self, other: "TaskMixin", upstream=True) -> None:
+ def update_relative(self, other: "DependencyMixin", upstream=True) -> None:
"""
Update relationship information about another TaskMixin. Default is no-op.
Override if necessary.
"""
- def __lshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __lshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Implements Task << Task"""
self.set_upstream(other)
return other
- def __rshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Implements Task >> Task"""
self.set_downstream(other)
return other
- def __rrshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rrshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Called for Task >> [Task] because list don't have __rshift__ operators."""
self.__lshift__(other)
return self
- def __rlshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rlshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Called for Task << [Task] because list don't have __lshift__ operators."""
self.__rshift__(other)
return self
+
+
+class TaskMixin(DependencyMixin):
+ """:meta private:"""
+
+ def __init_subclass__(cls) -> None:
Review comment:
This warns when you do `class X(TaskMixin):` -- i.e. right were you need to update the class definition. not at construction time which would point to where the sbuclass is constrcuted.
--
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
[GitHub] [airflow] ashb commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769049336
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
> though I'd be surprised if users use this class in custom code?
I too would be, but I am quite passionate about support backcompat if we 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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769016318
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
+DependencyMixinOrList = Union["DependencyMixin", Sequence["DependencyMixin"]]
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: DependencyMixinOrList):
Review comment:
one vote for verbose in this case. don't think it makes it easier to understand.
eventually it would become `"TaskMixin" | Sequence["TaskMixin"]` which maybe ain’t as bad?
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769078734
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
fwiw `DagGraphNode` seems a lot better to me
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769284602
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,78 @@
# specific language governing permissions and limitations
Review comment:
G in DAG already means Graph so this should be `DAGNode`.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20297: Rename TaskMixin to DependencyMixin
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20297:
URL: https://github.com/apache/airflow/pull/20297#discussion_r769101462
##########
File path: airflow/models/taskmixin.py
##########
@@ -15,61 +15,76 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
from abc import abstractmethod
from typing import Sequence, Union
-class TaskMixin:
- """
- Mixing implementing common chain methods like >> and <<.
-
- In the following functions we use:
- Task = Union[BaseOperator, XComArg]
- No type annotations due to cyclic imports.
- """
+class DependencyMixin:
+ """Mixing implementing common dependency setting methods methods like >> and <<."""
@property
- def roots(self):
- """Should return list of root operator List[BaseOperator]"""
+ def roots(self) -> Sequence["DependencyMixin"]:
+ """
+ List of root nodes -- ones with no upstream dependencies.
+
+ a.k.a. the "start" of this sub-graph
+ """
raise NotImplementedError()
@property
- def leaves(self):
- """Should return list of leaf operator List[BaseOperator]"""
+ def leaves(self) -> Sequence["DependencyMixin"]:
+ """
+ List of leaf nodes -- ones with only upstream dependencies.
+
+ a.k.a. the "end" of this sub-graph
+ """
raise NotImplementedError()
@abstractmethod
- def set_upstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_upstream(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Set a task or a task list to be directly upstream from the current task."""
raise NotImplementedError()
@abstractmethod
- def set_downstream(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def set_downstream(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Set a task or a task list to be directly downstream from the current task."""
raise NotImplementedError()
- def update_relative(self, other: "TaskMixin", upstream=True) -> None:
+ def update_relative(self, other: "DependencyMixin", upstream=True) -> None:
"""
Update relationship information about another TaskMixin. Default is no-op.
Override if necessary.
"""
- def __lshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __lshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Implements Task << Task"""
self.set_upstream(other)
return other
- def __rshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Implements Task >> Task"""
self.set_downstream(other)
return other
- def __rrshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rrshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Called for Task >> [Task] because list don't have __rshift__ operators."""
self.__lshift__(other)
return self
- def __rlshift__(self, other: Union["TaskMixin", Sequence["TaskMixin"]]):
+ def __rlshift__(self, other: Union["DependencyMixin", Sequence["DependencyMixin"]]):
"""Called for Task << [Task] because list don't have __lshift__ operators."""
self.__rshift__(other)
return self
+
+
+class TaskMixin(DependencyMixin):
+ """:meta private:"""
+
+ def __init_subclass__(cls) -> None:
Review comment:
curious why you only warn for subclasses? why not warn any time this is used?
--
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