Sphinx ignores decorated functions such as `@solid` due to missing signature

[jdb78]

Problem: When documenting a codebase that contains functions decorated with @solid (or other dagster decorators), the functions are not picked up by automodule, i.e. they are not documented.

Cause: The reason for this behaviour is that sphinx looks for the doc attribute which must be copied if a function is decorated.

Fix: Copying the doc attribute, e.g. by using functools or dynamically creating a doc attribute.

[mgasner]

hmm, this should be implemented https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/core/definitions/decorators.py#L148 but it's possible we missed a layer of indirection. can you provide a repro?

[jdb78]

Thanks for being so responsive!!

The repo in question is private, so I cannot share it.

Functions such as

@solid
def my_pipeline_func_1(context):
    pass

@solid()
def my_pipeline_function_2(context):
    pass

are ignored by the sphinx automodule

.. automodule:: my_module
   :members:
   :undoc-members:
   :show-inheritance:

This can be fixed by modifying the solid decorator

def decorator_with_arguments(f):
    """
    a decorator decorator, allowing the decorator to be used as:
    @decorator(with, arguments, and=kwargs)
    or
    @decorator
    """
    @functools.wraps(f)
    def new_dec(*args, **kwargs):
        if len(args) == 1 and len(kwargs) == 0 and callable(args[0]):
            # actual decorated function
            args[0].__decorator__ = f.__name__
            realf = f(args[0])
        else:
            # decorator arguments
            def realf(realf):
                realf.__decorator__ = f.__name__
                realf.__decorator_args__ = args
                realf.__decorator_kwargs__ = kwargs
                return f(realf, *args, **kwargs)
        return realf
    return new_dec

@decorator_with_arguments
def my_solid_decorator(fn, **solid_kwargs):
    @functools.wraps(fn)
    def wrapped(*args, **kwargs):
        return solid(**solid_kwargs)(fn)(*args, **kwargs)
    return wrapped

The reason why the current implementation does not work, I suspect is that a function definition needs to be created that acts as a wrapper. Only then, it can hold the doc attribute. For example, this does not work:

@decorator_with_arguments
def my_solid_decorator(fn, **solid_kwargs):
    wrapped = functools.wraps(fn)solid(**solid_kwargs)(fn)
    return wrapped

It should be very easy to replicate the behaviour in an existing repo. If you struggle to recreate the bug, I am happy to create a dedicated repo.

[alangenfeld]

We have this test which for@solid and @lambda_solid https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py#L434-L458

I fixed @pipeline and @composite_solid in https://dagster.phacility.com/D2844

[jdb78]

I am afraid this is not enough as the function signature is not preserved as sphinx thus skips it. You probably want a test like here: https://github.com/sphinx-doc/sphinx/blob/3.x/tests/test_ext_apidoc.py

[alangenfeld]

So our decorators do not return functions, but instances of our definition classes such as SolidDefinition. I believe if you add :inherited-members: to your automodule config, they should start showing up with the correct name and docstring.

I am looking in to what we can do to transfer function signatures from the wrapped function for sphinx to document.

[mgasner]

We could autogen docstrings for the SolidDefinition instances that included an Args block with a custom wrapper. I can maybe take a crack at this

[jdb78]

I checked and using :inherited-members: is unfortunately not doing the trick. Hope we can get this fixed soon. It is a real blocker for documenting a project.

[alangenfeld]

Thanks for trying that @jdb78. I had some time this morning to set up a repro locally and discovered the issue is due to our decorators returning instances of classes as suspected, but the reason is that none of the sphinx autodoc Documenter classes know how to handle that https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/ext/autodoc/__init__.py#L1766-L1777

Will do some more digging to see what we can do.

[jdb78]

Is there a way to work around this issue by e.g. returning a function with the correct signature that returns the instance of the class? It certainly would work but is admittedly a bit hacky.

[alangenfeld]

Ya that is one potential solution - but I believe quite costly due to the number of places that would need to check for and unpack the function carrying the actual Definition instance.

[muthugit]

I've fixed by adding ..auto_function in .rst file without adding the custom decorator or overriding solid decorator

 :members:

    .. autofunction:: <function name>

[jdb78]

@muthugit Great to know that works. I wonder if docs can become automatic with automodule as well.

[skorski]

Is there a resolution on documentation? We use autodoc and I'm not sure where to add @muthugit suggestion of autofunction. This is certainly a big missing feature for us as all solids are excluded from our documentation.

[bigdawgmj]

This is a gap, and would like to see this issue resolved.

[jwebster42]

Any guesses on where this is in the queue? I started using dagster too, but if I can't include it in my docs and there are no plans to fix this, then I need to jury rig another solution.

[yuhan]

Hi @bigdawgmj, @jwebster42! We are investigating options to address this issue. Will keep you posted once we have a next step.

Meanwhile, to get your solids documented, you can manually document them like: (unfortunately as a stop gap)

.. automodule:: <module name>
    :members:

        .. autofunction:: <solid name>

[yuhan]

After investigation, a path forward could be to write a custom sphinx parser to address this particular case. At the moment, we do not have bandwidth to do so. So moving this issue to backlog and will revisit it in the future

[isaacharrisholt]

I managed to get this working by adapting what Celery did for Sphinx. I've included the code here. It would be great to see this added to Dagster at some point in the future. parser.txt