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/04/07 15:17:31 UTC
[GitHub] [airflow] norm opened a new pull request, #22834: Operator name separate from class
norm opened a new pull request, #22834:
URL: https://github.com/apache/airflow/pull/22834
Closes #22762.
<!--
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] uranusjr commented on a diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r846630702
##########
airflow/decorators/python.py:
##########
@@ -55,6 +55,10 @@ def __init__(self, *, python_callable, op_args, op_kwargs, **kwargs) -> None:
**kwargs,
)
+ @property
+ def get_operator_ui_name(self):
+ return 'PythonOperator'
Review Comment:
But the metrics are currently using `task_type`. Since we’re introducing a new property (like in this PR), we can do whatever we want.
--
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 diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r945551849
##########
airflow/models/baseoperator.py:
##########
@@ -1458,6 +1466,7 @@ def get_serialized_fields(cls):
'start_date',
'end_date',
'_task_type',
+ '_operator_name',
Review Comment:
Oh, `_operator_name` is on MappedOperator, so rather than adding this here, add that in to the same named function in MappedOperator instead.
--
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] norm commented on a diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
norm commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r845280122
##########
airflow/decorators/python.py:
##########
@@ -55,6 +55,10 @@ def __init__(self, *, python_callable, op_args, op_kwargs, **kwargs) -> None:
**kwargs,
)
+ @property
+ def get_operator_ui_name(self):
+ return 'PythonOperator'
Review Comment:
@ashb mentioned metrics to me as a reason not to choose that.
--
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 #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #22834:
URL: https://github.com/apache/airflow/pull/22834#issuecomment-1217672873
Only providers test is failing/stalling. Unrelated to this change. Merging.
--
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 diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r947317960
##########
airflow/operators/empty.py:
##########
@@ -29,6 +29,7 @@ class EmptyOperator(BaseOperator):
ui_color = '#e8f7e4'
inherits_from_empty_operator = True
+ custom_operator_name = 'Empty'
Review Comment:
```suggestion
```
--
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] norm commented on a diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
norm commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r946455774
##########
airflow/models/baseoperator.py:
##########
@@ -1458,6 +1466,7 @@ def get_serialized_fields(cls):
'start_date',
'end_date',
'_task_type',
+ '_operator_name',
Review Comment:
I'm not sure I understand your suggestion — `_operator_name` is the same trick as `_task_type` to preserve the name of *any* operator during serialisation. It is not specific to one?
--
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] closed pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #22834: Operator name separate from class
URL: https://github.com/apache/airflow/pull/22834
--
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 diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r944620161
##########
airflow/models/baseoperator.py:
##########
@@ -1458,6 +1466,7 @@ def get_serialized_fields(cls):
'start_date',
'end_date',
'_task_type',
+ '_operator_name',
Review Comment:
I don't think this is needed anymore, as operator_name is now a property, not a plain attribute?
--
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 #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #22834:
URL: https://github.com/apache/airflow/pull/22834#issuecomment-1216833314
> <img alt="Screen Shot 2022-08-16 at 11 51 37 AM" width="289" src="https://user-images.githubusercontent.com/4600967/184923646-4f06afe6-32d3-4b39-b5f5-46bb18b053a3.png">
>
> Is this expected?
Yes. Previously it showed `_PythonDecoratorOperator` or some such, which was.... ick.
--
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 diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r946567757
##########
airflow/models/baseoperator.py:
##########
@@ -1458,6 +1466,7 @@ def get_serialized_fields(cls):
'start_date',
'end_date',
'_task_type',
+ '_operator_name',
Review Comment:
On my (admittedly somewhat cursory) reading of this code, a BaseOperator should never have an attribute called `_operator_name`, that only exists as a field on `MappedOperator` instances. Have I read it wrong?
--
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] norm commented on pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
norm commented on PR #22834:
URL: https://github.com/apache/airflow/pull/22834#issuecomment-1091870164
The UI will need to change to use the new value.
--
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 diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r937747705
##########
airflow/decorators/python.py:
##########
@@ -55,6 +55,10 @@ def __init__(self, *, python_callable, op_args, op_kwargs, **kwargs) -> None:
**kwargs,
)
+ @property
+ def get_operator_ui_name(self):
+ return 'PythonOperator'
Review Comment:
Yes, TP is right.
--
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 #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22834:
URL: https://github.com/apache/airflow/pull/22834#issuecomment-1200537306
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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] norm commented on a diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
norm commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r946717853
##########
airflow/operators/empty.py:
##########
@@ -29,6 +29,7 @@ class EmptyOperator(BaseOperator):
ui_color = '#e8f7e4'
inherits_from_empty_operator = True
+ custom_operator_name = 'Empty'
Review Comment:
Ah, yes, I did that when updating the tests to prove I wasn't just accidentally getting `task_type`. I'll revert that and use a specific class that's just in the tests.
--
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] norm commented on a diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
norm commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r945075182
##########
airflow/models/baseoperator.py:
##########
@@ -1458,6 +1466,7 @@ def get_serialized_fields(cls):
'start_date',
'end_date',
'_task_type',
+ '_operator_name',
Review Comment:
I followed how `_task_type` is handled, which is why it was there. Now, if I take it out, `tests/serialization/test_dag_serialization.py::test_operator_expand_serde` fails. I'll have a further dig into it.
--
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 #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb merged PR #22834:
URL: https://github.com/apache/airflow/pull/22834
--
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 diff in pull request #22834: WIP: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r941087849
##########
airflow/models/baseoperator.py:
##########
@@ -1345,6 +1347,10 @@ def get_direct_relatives(self, upstream: bool = False) -> Iterable["DAGNode"]:
def __repr__(self):
return "<Task({self.task_type}): {self.task_id}>".format(self=self)
+ @property
+ def get_operator_name(self):
+ return self.operator_name
+
Review Comment:
... and we could implement this as
```python
def get_operator_name(self) -> str:
return self.operator_name or type(self).__name__
```
p.s. when I use `property` I’d drop `get_`, and vice versa.
##########
airflow/models/baseoperator.py:
##########
@@ -700,6 +700,8 @@ class derived from this one results in the creation of a task object,
# Set to True for an operator instantiated by a mapped operator.
__from_mapped = False
+ operator_name: str = 'BaseOperator'
Review Comment:
Maybe a better default would be `operator_name = ""`?
--
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 diff in pull request #22834: WIP: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r941086699
##########
airflow/decorators/python_virtualenv.py:
##########
@@ -44,6 +44,8 @@ class _PythonVirtualenvDecoratedOperator(DecoratedOperator, PythonVirtualenvOper
# there are some cases we can't deepcopy the objects (e.g protobuf).
shallow_copy_attrs: Sequence[str] = ('python_callable',)
+ operator_name: str = 'PythonVirtualenvOperator'
Review Comment:
Is this `@task.virtualenv`?
--
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] bbovenzi commented on pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #22834:
URL: https://github.com/apache/airflow/pull/22834#issuecomment-1216828588
<img width="289" alt="Screen Shot 2022-08-16 at 11 51 37 AM" src="https://user-images.githubusercontent.com/4600967/184923646-4f06afe6-32d3-4b39-b5f5-46bb18b053a3.png">
Is this expected?
--
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 diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r845266860
##########
airflow/decorators/python.py:
##########
@@ -55,6 +55,10 @@ def __init__(self, *, python_callable, op_args, op_kwargs, **kwargs) -> None:
**kwargs,
)
+ @property
+ def get_operator_ui_name(self):
+ return 'PythonOperator'
Review Comment:
I’m thinking maybe we should let this return `@task.python`; does that look better in the UI?
##########
airflow/decorators/python.py:
##########
@@ -55,6 +55,10 @@ def __init__(self, *, python_callable, op_args, op_kwargs, **kwargs) -> None:
**kwargs,
)
+ @property
+ def get_operator_ui_name(self):
+ return 'PythonOperator'
Review Comment:
I’m thinking maybe we should let this return `"@task.python"`; does that look better in the UI?
--
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 #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #22834:
URL: https://github.com/apache/airflow/pull/22834#issuecomment-1091877762
The value also needs to be accessible from a MappedOperator; I think this requires the value to be class-level on BaseOperator (and subclasses), but a property (or instance method) on MappedOperator.
--
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 #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22834:
URL: https://github.com/apache/airflow/pull/22834#issuecomment-1136552308
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r946630871
##########
airflow/operators/empty.py:
##########
@@ -29,6 +29,7 @@ class EmptyOperator(BaseOperator):
ui_color = '#e8f7e4'
inherits_from_empty_operator = True
+ custom_operator_name = 'Empty'
Review Comment:
Only remaining question is if we want this (or should we leave it as `EmptyOperator`)
--
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] norm commented on a diff in pull request #22834: Operator name separate from class
Posted by GitBox <gi...@apache.org>.
norm commented on code in PR #22834:
URL: https://github.com/apache/airflow/pull/22834#discussion_r946456226
##########
airflow/models/baseoperator.py:
##########
@@ -1458,6 +1466,7 @@ def get_serialized_fields(cls):
'start_date',
'end_date',
'_task_type',
+ '_operator_name',
Review Comment:
(and tests fail if I move it as suggested, although that could be that the test is doing the wrong thing)
--
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