Make context optional in operators

[dstandish]

Many operators don't use it.

And when developing locally, if your operator does not use it, it's very convenient to call task.execute() to test your task.

So that type would be Union[Optional[dict], dict] in essence.

WDYT?

[turbaszek]

I'm not convinced. All our tests do op.execute(context={}) or similar and I don't see this as a something troublesome. Also, testing operators out side of the Airflow context is not best practice in my opinion. Hooks - yes, operators - no.

But that's my subjective feeling 😄

[dstandish]

well my fingers say it is troublesome ;)

[dstandish]

but... on serious note... given that it is de facto optional, why not make it explicitly optional?

[potiuk]

I'd say far better solution would be to have empty dict as default if not https://stackoverflow.com/questions/26320899/why-is-the-empty-dictionary-a-dangerous-default-value-in-python .

I think The problem with setting it as Optional would mean that the mutlitude of operators (the custom ones) would have to be prepared to accept 'execute' without context, which is not backwards-incompatible I am afraid. Of course we can make sure that Airflow itself does not do it and always passes a context. So this is limited to only manual execution in tests and some custom code.

I am a bit on the fence with this one.

[turbaszek]

And btw. doing context=None in BaseOperator will not propagate to other ops where we override the execute method, or am I wrong?

[dstandish]

i am just asking the question ... sortof in a vacuum.... from a python perspective essentially...

consider:

class BaseOperator:
    def execute(self, context=None):
        pass

class MyXcomOp(BaseOperator):
    def execute(self, context):
        # uses context

Do we have a fundamental problem with doing this, aside from airflow-specific technical considerations?

[dstandish]

context=None in BaseOperator will not propagate to other ops where we override the execute

correct but my wish is not that every operator must have context=None
i just mean, with this change, to allow it
just, blueskying

[turbaszek]

@dstandish so I'm lost, I assume that you can just do context=None in your custom operator. I think this is a best way as it's operator of your design and you know that it does not use context for example (no issue with xcom_pushes for instance).

[potiuk]

@dstandish so I'm lost, I assume that you can just do context=None in your custom operator. I think this is a best way as it's operator of your design and you know that it does not use context for example (no issue with xcom_pushes for instance).

Yep. You are totally right indeed Tomek. But I think what @dstandish is about is to make it more of a 'standard' so that gradually all operators will also be written in the way to accept context = None. More of a 'best practice' of what people do when they look at or copy from other operators.

[dstandish]

you can just do context=None in your custom operator

right

but what if someone wanted to contribute an operator in airflow where context is never used inside execute. what is wrong with having the signature be context=None?

that is the question here.

[turbaszek]

pycharm wrinkles an eyebrow (and so did @ashb ;) , hence this discussion) , but that's all

It will also highlight any usages of xcom_push because it expects Dict not Optional[Dict] and here is pylint for doing context=None in base op:

************* Module airflow.operators.python
airflow/operators/python.py:111:4: W0222: Signature differs from overridden 'execute' method (signature-differs)
airflow/operators/python.py:232:4: W0222: Signature differs from overridden 'execute' method (signature-differs)

[dstandish]

xcom_push is not called from execute 🤔

[turbaszek]

It's quite common to call it to make it easier to access some values like this:

airflow/airflow/providers/google/cloud/operators/automl.py

Line 120 in e7dbed2

self.xcom_push(context, key="model_id", value=model_id)

So I would say it's a known practice that we have to have in mind.

[dstandish]

yeah this makes sense

in this case, i think that it would be fine for this operator to require context (even though, it possibly becomes inconsistent with parent class signature). or if appropriate, could make it optional and handle the case appropriately in any usage of context.

[turbaszek]

As I said previously:

To me this change is more like "allow airflow operators(=execute functions) to be called outside Airflow" and this is something bigger than signatures and practices.

So I would suggest bringing this up to the dev list.