Skip to content

Conversation

@suiting-young
Copy link
Contributor

class BaseOperator(...):
    @property
    def task_type(self) -> str: ...

The task_type property is designed to show customized name (for human to distinguish, not to import code)
and is allowed to override in sub-class, if I understand correctly.


My Use Case

I need to distinguish operators by connection type, something likes:

from airflow.providers.jdbc.operators.jdbc import JdbcOperator

class DistinctJdbcOperator(JdbcOperator):
    @property
    def task_type(self):
        return self.conn.conn_type.capitalize() + 'JdbcOperator'

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we use task_type to get the class name itself and was not intended to have a Human Readable name.

We also use it here for validation:

for ope in plugins_manager.operator_extra_links:
for operator in ope.operators:
if (
operator.__name__ == encoded_op["_task_type"]
and operator.__module__ == encoded_op["_task_module"]
):
op_extra_links_from_plugin.update({ope.name: ope})

@suiting-young
Copy link
Contributor Author

was not intended to have a Human Readable name.

My use case isn't to place arbitrary sentences there, but create some fake-sub-classes (that my users will treat them as real class names).

In 21a91c4 (2nd commit) I extended BaseOperatorLink.operators to accept str.
So that task_type-customized classes can have their own links.

This won't affect existing code because

  • No operator overrode task_type in official source tree.
  • Custom installation has no reason (and actually, no effect) to override task_type without 328a43a (1st commit).

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

operator.__name__ == encoded_op["_task_type"]
and operator.__module__ == encoded_op["_task_module"]
):
) if not isinstance(operator, str) else (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find a cleaner way of writing this -- it is hard/impossible to grok what it is going.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted this part to a function in 1fde244.

@ashb
Copy link
Member

ashb commented Mar 17, 2021

Code comments aside, I'm not sure if we want to support this behaviour - I'm trying to think through future implications where we might use the serialized representation for more (such as in the worker for actually creating Operator objects), and enabling this PR might stop us doing that.

@suiting-young
Copy link
Contributor Author

might use the serialized representation for more (such as in the worker for actually creating Operator objects)

Do you mean

  • serialize the whole operator (e.g. through pickle),
  • or only serialize import path, and arguments of constructor?

And either way, execute the deserialized operator without its DAG?

@ashb
Copy link
Member

ashb commented Mar 19, 2021

I'm not thinking pickle, no, so something closer to the second point.

@ashb
Copy link
Member

ashb commented Mar 31, 2021

Closing this for now as won't fix, sorry.

@ashb ashb closed this Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants