Dagster decorators and signature mutation

[medihack]

First, I want to mention that I really love Dagster. It's super easy to set up, the API is very clean and dagit is great looking. The thing I am a bit ambivalent about is how the Dagster decorators do manipulate the function signatures, which in some opinion is bad practice. I myself am undecided about this and therefore would like to discuss it here.

When for example using a function with a parameter that is decorated as @op and using it later, then Pylint complains about a missing argument:

@op
def hello(context, name: str):
    context.log.info(f"Hello, {name}!")

@job
def hello_dagster():
    hello("Buddy")  # No value for argument 'name' in function callpylint(no-value-for-parameter)

When enabling type checking with Pyright it would also fail. Ok, one could disable those messages by using # pylint: disable=no-value-for-parameter and type: ignore, but you have to do this on every line where you use such a decorated function (to disable it completely is not good practice). There were also some issue reports regarding signatures modifications by decorators on the Github Pylint page (and also Pyright), but all were closed as it doesn't seem fixable.

Alternatively (at least for the Pylint error) one could add signature-mutators=graph, job, op into the .pylintrc (IMHO this should at least be documented in the official Dagster docs).

I looked a bit around how other frameworks do this. Prefect for example allows to extend a Task class (to totally circumvent the decorator problem) or when using a decorator fetch the context from an imported object.

I am not sure what the best way would be, but I thought it is worth discussing.

[dpeng817]

Hi @medihack ! Thanks for opening this.

At the very least, definitely agreed that we should document the .pylintrc fix. We actually use that within the dagster repo to circumvent this very issue.

The decorator syntax that we use is just sugar for constructing definition objects. Ie, the @op decorator constructs an instance of OpDefinition, but for folks who this may rub the wrong way, it is totally possible to construct these definition objects directly.

The dsl employed in the @graph/@job decorators, which causes the pylint issue, is ultimately just used to build out the dependency structure, but this can also be manually constructed.

In general, the decorator syntax provides Dagster with a clean and approachable API, and we're unlikely to move away from that approach. There are other API-level things we could add to help get rid of the pylint error, such as allowing people to call a .invoke() method instead of directly invoking their ops within jobs:

@job
def hello_dagster():
    hello.invoke(world.invoke())

But ultimately this is additional boilerplate. It might be worth supporting this in addition to the current method of invocation.

[medihack]

Thanks a lot for your clarifications. I totally agree and a hint in the documentation regarding Pylint would be nice.