Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undesired "<SomeOperator>.execute cannot be called outside TaskInstance!" warning #39413

Closed
1 of 2 tasks
zhaow-de opened this issue May 5, 2024 · 9 comments
Closed
1 of 2 tasks
Labels
area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet pending-response stale Stale PRs per the .github/workflows/stale.yml policy file

Comments

@zhaow-de
Copy link

zhaow-de commented May 5, 2024

Apache Airflow version

2.9.0

If "Other Airflow 2 version" selected, which one?

No response

What happened?

Decorated operators using the classic API trigger warning message ".execute cannot be called outside TaskInstance!"

What you think should happen instead?

PR #37937 introduced a new check to avoid the mixed usage of classic and decorated operators. Because a boundary condition of checking the decorators is missing, the operators cannot be decorated if a DAG uses the classic API.

Hopefully, the check should only be triggered if the operator is decorated with airflow.decorators.task

How to reproduce

Run the DAG below:

from airflow import DAG
from airflow.operators.python import PythonOperator


def deco(cls):
    orig_init = cls.__init__

    def new_init(self, *args, default_args=None, **kwargs):
        orig_init(self, *args, **kwargs)
        self.default_args = default_args

    cls.__init__ = cls._apply_defaults(new_init)
    return cls


@deco
class AlloyPythonOperator(PythonOperator):
    def execute(self, context):
        super().execute(context)


def no_ops():
    pass


with DAG(
    dag_id="test-dag",
    catchup=False,
):
    AlloyPythonOperator(
        task_id="trigger-execute",
        python_callable=no_ops,
    )

Operating System

Ubuntu 22.04 LTS, but the issue is OS independant

Versions of Apache Airflow Providers

core

Deployment

Other Docker-based deployment

Deployment details

No response

Anything else?

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@zhaow-de zhaow-de added area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels May 5, 2024
Copy link

boring-cyborg bot commented May 5, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@eladkal
Copy link
Contributor

eladkal commented May 6, 2024

cc @dabla WDYT?

@Taragolis
Copy link
Contributor

My 2 cents, this code use internals methods, as result it is expected that this approach could be broken on changes in airflow codebase

def deco(cls):
    orig_init = cls.__init__

    def new_init(self, *args, default_args=None, **kwargs):
        orig_init(self, *args, **kwargs)
        self.default_args = default_args

    cls.__init__ = cls._apply_defaults(new_init)
    return cls

@dabla
Copy link
Contributor

dabla commented May 6, 2024

WDYT

Seems like the deco method is considered as calling he execute method of the PythonOperator outside of the TaskInstance, this is the protection mechanism which would warn you when mixing task decorated tasks with native operators. But normally it should still work atm, as this only triggers a warning.

Ok think I understand, it's because the class AlloyPythonOperator is instantiated outside the class TaskInstance, due to the deco method, so the sentinel which acts as a guard isn't set and thus Airflow considers that the execute method is called outside the TaskInstance

@zhaow-de
Copy link
Author

zhaow-de commented May 6, 2024

Thanks for the investigation and feedback, @Taragolis and @dabla!

Yes, everything still works despite the warning message. I created this issue just as a "preventive" measure because @potiuk wrote in the closing comment of #37937:

Having a warning is "good enough" for now. If we decide to do something more (for example allow such use to behave more "as expected") we can always add it later.

What would you recommend if we need to inject some common logic into the constructor and the execute method of around 15 different (standard) operators? Or shall I patch the TaskInsntance and BaseOperator classes to insert the __sentinel property?

@dabla
Copy link
Contributor

dabla commented May 6, 2024

Thanks for the investigation and feedback, @Taragolis and @dabla!

Yes, everything still works despite the warning message. I created this issue just as a "preventive" measure because @potiuk wrote in the closing comment of #37937:

Having a warning is "good enough" for now. If we decide to do something more (for example allow such use to behave more "as expected") we can always add it later.

What would you recommend if we need to inject some common logic into the constructor and the execute method of around 15 different (standard) operators? Or shall I patch the TaskInsntance and BaseOperator classes to insert the __sentinel property?

In your example above, if you only have this "issue" of common logic with the PythonOpertor, then I would suggest you use the @task decorated method, there you could delegate to another non decorated method having all the common logic. Personally, I would not try to "mess" with native operators and start extending them.

Copy link

github-actions bot commented Jun 9, 2024

This issue has been automatically marked as stale because it has been open for 14 days with no response from the author. It will be closed in next 7 days if no further activity occurs from the issue author.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jun 9, 2024
Copy link

This issue has been closed because it has not received response from the issue author.

@zachliu
Copy link
Contributor

zachliu commented Oct 8, 2024

i don't think the deco caused this warning. remove the @deco you'd still get the same warning
see #41470

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet pending-response stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

No branches or pull requests

6 participants