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 2022/02/25 05:48:27 UTC
[GitHub] [airflow] uranusjr opened a new pull request #21815: Male BaseSensorOperator.deps a class variable
uranusjr opened a new pull request #21815:
URL: https://github.com/apache/airflow/pull/21815
This is the easiest way to get mapped operator's deps to work without jumping through too many hoops. This does introduce some potential backward incompatibility for peopel relying on the (internal?) dep class mechanism, but I _think_ it is acceptable.
--
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 #21815: Ensure a mappable operator's deps is a set, and convert BaseSensorOperator.deps to a class variable
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#discussion_r816636698
##########
File path: airflow/models/mappedoperator.py
##########
@@ -284,7 +285,10 @@ def get_serialized_fields(cls):
@staticmethod
@cache
def deps_for(operator_class: Type["BaseOperator"]) -> FrozenSet[BaseTIDep]:
- return operator_class.deps | {MappedTaskIsExpanded()}
+ operator_deps = operator_class.deps
+ if not isinstance(operator_deps, collections.abc.Set):
+ raise UnmappableOperator(f"cannot map operator; f{operator_class}.deps must be a set")
Review comment:
"deps must be defined as a class-level variable, not a property"?
--
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 edited a comment on pull request #21815: Make BaseSensorOperator.deps a class variable
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1054205795
> > may break/not work with customer operators if someone sets deps on an instance level :(
>
> Maybe we can detect that (easy enough, simply check the attribute is actually a set/collection on the class level and not a descriptor), and raise an error saying the operator needs to be fixed? This can likely be done on DAG-parsing time.
:+1: I think it's unlikely enough to affect many people, so if we put a note in updating about this it should be good enough
@evgenyshulman I remember on the multi-tenancy call that you do this. Would this cause problems for 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] ashb commented on pull request #21815: Make BaseSensorOperator.deps a class variable
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1050615372
This fixes the current issue, but may break/not work with customer operators if someone sets deps on an instance level :(
--
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 #21815: Male BaseSensorOperator.deps a class variable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1050548246
This gets me pass the error, but hits another one:
```
Traceback (most recent call last):
File "dags/t.py", line 23, in <module>
target_time=templates_xcomarg
File "/opt/airflow/airflow/models/mappedoperator.py", line 215, in apply
end_date=end_date,
File "<attrs generated init airflow.models.mappedoperator.MappedOperator>", line 24, in __init__
File "/opt/airflow/airflow/models/mappedoperator.py", line 259, in __attrs_post_init__
self._validate_argument_count()
File "/opt/airflow/airflow/models/mappedoperator.py", line 300, in _validate_argument_count
op = self._create_unmapped_operator(mapped_kwargs=mocked_mapped_kwargs, real=False)
File "/opt/airflow/airflow/models/mappedoperator.py", line 453, in _create_unmapped_operator
**mapped_kwargs,
File "/opt/airflow/airflow/models/baseoperator.py", line 374, in apply_defaults
result = func(self, **kwargs, default_args=default_args)
File "/opt/airflow/airflow/sensors/date_time.py", line 69, in __init__
f"Expected str or datetime.datetime type for target_time. Got {type(target_time)}"
TypeError: Expected str or datetime.datetime type for target_time. Got <class 'unittest.mock.MagicMock'>
```
I guess we do need a class-level `validate` hook on operators.
--
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 #21815: Ensure a mappable operator's deps is a set, and convert BaseSensorOperator.deps to a class variable
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1056963709
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 merged pull request #21815: Ensure deps is set, convert BaseSensorOperator to classvar
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #21815:
URL: https://github.com/apache/airflow/pull/21815
--
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 #21815: Make BaseSensorOperator.deps a class variable
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1054205795
> > may break/not work with customer operators if someone sets deps on an instance level :(
>
> Maybe we can detect that (easy enough, simply check the attribute is actually a set/collection on the class level and not a descriptor), and raise an error saying the operator needs to be fixed? This can likely be done on DAG-parsing time.
:+1: I think it's unlikely enough to affect anyway, so if we put a note in updating about this it should be good.
@evgenyshulman I remember on the multi-tenancy call that you do this. Would this cause problems for 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] uranusjr commented on a change in pull request #21815: Ensure a mappable operator's deps is a set, and convert BaseSensorOperator.deps to a class variable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#discussion_r817568990
##########
File path: airflow/models/mappedoperator.py
##########
@@ -284,7 +285,10 @@ def get_serialized_fields(cls):
@staticmethod
@cache
def deps_for(operator_class: Type["BaseOperator"]) -> FrozenSet[BaseTIDep]:
- return operator_class.deps | {MappedTaskIsExpanded()}
+ operator_deps = operator_class.deps
+ if not isinstance(operator_deps, collections.abc.Set):
+ raise UnmappableOperator(f"cannot map operator; f{operator_class}.deps must be a set")
Review comment:
Updated
--
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 #21815: Make BaseSensorOperator.deps a class variable
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1050616549
> TypeError: Expected str or datetime.datetime type for target_time. Got <class 'unittest.mock.MagicMock'>
> ```
>
> I guess we do need a class-level `validate` hook on operators.
We're going to hit this sort of error more and more with other operators. We might need to rethink how we validate the args
--
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 #21815: Make BaseSensorOperator.deps a class variable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1054193428
> may break/not work with customer operators if someone sets deps on an instance level :(
Maybe we can detect that (easy enough, simply check the attribute is actually a set/collection on the class level and not a descriptor), and raise an error saying the operator needs to be fixed? This can likely be done on DAG-parsing time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #21815: Make BaseSensorOperator.deps a class variable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#issuecomment-1054234046
Check added.
--
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 #21815: Ensure a mappable operator's deps is a set, and convert BaseSensorOperator.deps to a class variable
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#discussion_r815881591
##########
File path: airflow/models/mappedoperator.py
##########
@@ -284,7 +285,10 @@ def get_serialized_fields(cls):
@staticmethod
@cache
def deps_for(operator_class: Type["BaseOperator"]) -> FrozenSet[BaseTIDep]:
- return operator_class.deps | {MappedTaskIsExpanded()}
+ operator_deps = operator_class.deps
+ if not isinstance(operator_deps, collections.abc.Set):
+ raise UnmappableOperator(f"cannot map operator; f{operator_class}.deps must be a set")
Review comment:
Can we be more explicit here -- if it is defined as a property to tell the user what is wrong as this could easily be confusing to the user if they have it as:
```python
@property
def deps(self):
return {MyDep()}
```
Because at first glance this _is_ a set.
--
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 #21815: Ensure a mappable operator's deps is a set, and convert BaseSensorOperator.deps to a class variable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21815:
URL: https://github.com/apache/airflow/pull/21815#discussion_r816066090
##########
File path: airflow/models/mappedoperator.py
##########
@@ -284,7 +285,10 @@ def get_serialized_fields(cls):
@staticmethod
@cache
def deps_for(operator_class: Type["BaseOperator"]) -> FrozenSet[BaseTIDep]:
- return operator_class.deps | {MappedTaskIsExpanded()}
+ operator_deps = operator_class.deps
+ if not isinstance(operator_deps, collections.abc.Set):
+ raise UnmappableOperator(f"cannot map operator; f{operator_class}.deps must be a set")
Review comment:
Anything suggestion on wording? I’m struggling to come up with a succinct description.
--
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