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

autodoc.typehints integration not working #273

Closed
samschott opened this issue Mar 23, 2021 · 16 comments
Closed

autodoc.typehints integration not working #273

samschott opened this issue Mar 23, 2021 · 16 comments

Comments

@samschott
Copy link

samschott commented Mar 23, 2021

When inspecting the data returned by parse_functiondef I can see that the type hints are correctly picked up by autoapi. However, including sphinx.ext.autodoc and specifying a autodoc_typehints config value of "description" or "none" does not seem to have any effect at all.

I'm not sure how exactly the integration with sphinx.ext.autodoc works so it's hard to tell what might be going wrong. Do you have any pointers? It might just be an obvious oversight on my side.

@samschott
Copy link
Author

After some investigation, it appears that the annotations are added to the temp_data here:

def _record_typehints(self, obj):
if isinstance(obj, (PythonClass, PythonFunction, PythonMethod)):
annotations = self.app.env.temp_data.setdefault("annotations", {})
annotations[obj.id] = obj.obj["annotations"]

And they should be picked up by autodoc.typehints here:

https://github.com/sphinx-doc/sphinx/blob/7ecf6b88aa5ddaed552527d2ef60f1bd35e98ddc/sphinx/ext/autodoc/typehints.py#L58-L60

But there are not, because temp_data is cleared before the build here:

https://github.com/sphinx-doc/sphinx/blob/7ecf6b88aa5ddaed552527d2ef60f1bd35e98ddc/sphinx/builders/__init__.py#L487-L489

Am I reading this correctly? There are a lot of moving parts that come together in the code, so it's difficult to immediately understand the execution flow.

@samschott
Copy link
Author

So I can see two options:

  1. Store the annotations somewhere else and inject them after the temp_data has been cleared. This works in my initial tests but has the disadvantage that the annotations now show up both in the parameters list and in the function signature. sphinx.ext.autodoc.typehints promises to remove the annotations from the signature when setting autodoc_typehints = "description".
  2. Merge the annotations into the description in autoapi itself and optionally remove them from the signature. Write them to the generated rst files. This would provide better control.

@AWhetter
Copy link
Collaborator

Option 1 was my first instinct as well. It seems silly to have to reimplement this behaviour if it exists in Sphinx already. What do you mean by "has the disadvantage that the annotations now show up both in the parameters list and in the function signature."? I'm guessing that you mean that because the template files write out the signature of the function/method to include the signature, that setting autodoc_typehints = "description" means that the signature is included in the description as well as the signature?
If that's the case, I think we should go with option 1 and remove the writing out of type hints ourselves in the template. It's better for users to have the option to control this behaviour through opting into using the typehints extension and setting autodoc_typehints than it is to have to edit the templates themselves.
However the downside with doing that right now is that I don't think that the typehints extension supports overloads properly. So we would be losing overload type hints to gain the ability to have typehints only in the description. I might be mistaken though so I need to double check.

@samschott
Copy link
Author

samschott commented Mar 25, 2021

It seems silly to have to reimplement this behaviour if it exists in Sphinx already.

That's a good point.

I'm guessing that you mean that because the template files write out the signature of the function/method to include the signature, that setting autodoc_typehints = "description" means that the signature is included in the description as well as the signature?

Yes, that's what I meant. Essentially, you end up presenting the type information twice when it's only really needed once.

However the downside with doing that right now is that I don't think that the typehints extension supports overloads properly.

I think you are right but would have to check the implementation. This might be a situation where autoapi could potentially provide an additional option autodoc_typehints = "both" in addition to "description" and "none". But I don't know if it's worth the effort.

@samschott
Copy link
Author

On the note of overloads, I am not even sure how they would be typically documented in the doc string. Is there any convention for this?

@AWhetter
Copy link
Collaborator

AWhetter commented Mar 26, 2021

Not really because the convention is to document the types in the signature. The only other thing that you could call a convention (at least from what I've seen so this is by no means official) would be the format of pybind docstrings:

"""Overloaded function.

1. add(arg0: int, arg1: int) -> int

Add two integers together.

2. add(arg0: float, arg1: float) -> float

Add two floating points numbers together.
"""

Side note: pybind outputs the signature of the function on the first line (eg https://github.com/pybind/pybind11/blob/0e01c243c7ffae3a2e52f998bacfe82f56aa96d9/tests/test_factory_constructors.py#L91) because it's coming from C++ so the signature can't be inspected by Python. I haven't included that in the above example because we're talking about Python functions with a real signature.

@AWhetter
Copy link
Collaborator

AWhetter commented Apr 1, 2021

I think the right thing to do here is to add support for overloads to autodoc.typehints and to do so by adding a new event that we can connect to it and register both overloads and regular type signatures with it. We're kinda hooking into a private part of Sphinx by using temp_data I think so it would be nice to have a public way of declaring these type hints.

@samschott
Copy link
Author

Yes, I agree that the temp_data approach is a bit fragile. It's also a bit difficult to understand for newcomers: it took me some time to figure out what exactly is stored in temp_data at which point during the build process.

Support for overloads in autodoc.typehints would be great!

@daniel3550
Copy link

I'm running into the same issue. Typehints do show up in function signature but do not show up in description with autodoc_typehints = 'description'

@AWhetter
Copy link
Collaborator

AWhetter commented Apr 3, 2021

I've finally got around to trying to reproduce your problem, and I can't. For me I do see parameter types get included in the docstring when autodoc_typehints = "description".

For example the following function will be rendered as the following

def myfunc(a: int, b: str) -> bool:
    """This is my function.

    :param a: A number.
    :param b: A string.

    :returns: Sometimes true.
    """
This is my function

Parameters: * a (int) - A number.
            * b (str) - A string.

Returns: Sometimes true.
Return type: bool

What is incorrect when compared to autodoc is that signature types are still displayed. This is fixable.
However the extension isn't as separate from autodoc as it once was. It won't add signature types because it relies on the regular autodoc extension to output them when autodoc_typehints = "signature". This is partly why the extension doesn't support overloads and in fact it won't output overloads at all when autodoc_typehints is anything other than signature.

Adding proper overload support to autodoc.typehints isn't as easy as I initially thought. So for now I will emulate the behaviour of autodoc and output types to the signature only when autodoc_typehints = "signature" and overloads won't be output when it is set to anything else.

Given that I've had to integrate even more deeply with Sphinx I'm going to consider our integration with autodoc.typehints experimental until it properly supports overload functions and we have a public way of registering type hints with it.

@daniel3550
Copy link

@AWhetter I'm using napolean (first item in conf.py extensions[]), perhaps that is causing an issue? The compiled RST looks very similar to yours, but I don't get types in the html descriptions. Maybe I'm missing something else? Using autodoc only I get types in signature and description as shown below.

python code

def myfunc(a: int, b: str) -> bool:
    """This is my function.

    Args:
        a: A number.
        b: A string.

    Returns:
        Sometimes true.
    """
    if a > 10 and len(b) > 0:
        return True

resulting autoapi index.rst

.. function:: myfunc(a: int, b: str) -> bool

   This is my function.

   :param a: A number.
   :param b: A string.

   :returns: Sometimes true.

output
image

using autodoc without autoapi, I get
image

@brentyi
Copy link
Contributor

brentyi commented Apr 6, 2021

Hi!

I'm running into this issue as well; it seems to be caused by exactly the lines that @samschott cited above. I've added some changes for reproducing the issue based on the py3example test setup here: brentyi@f13542f

In short, I have two classes documented and hinted identically.
The first generates:
image

The second generates (note the missing types!):
image

If I add this line:

    print(fullname, len(annotations))

after https://github.com/sphinx-doc/sphinx/blob/7ecf6b88aa5ddaed552527d2ef60f1bd35e98ddc/sphinx/ext/autodoc/typehints.py#L58, I get:

example.A.test 4
example2.B.test 0

Sphinx's internals are pretty magic to me, but the annotations are cleared after example.py is processed but before example2.py is processed. I was also able to reproduce this sans Napoleon: brentyi@6a6efc0

@samschott
Copy link
Author

@brentyi, thanks for providing an example to reproduce this. Looking through what you have posted here and going through my own docs, I can confirm that type hints are indeed added correctly for a single module but get cleared for all others.

@brentyi
Copy link
Contributor

brentyi commented Apr 24, 2021

@AWhetter any chance we could re-open this issue? We have a reproducible case above; @daniel3550 also posted an example independently.

No rush, but this feature is pretty critical for folks using type hints. 🙂

@AWhetter
Copy link
Collaborator

Apologies for taking so long to address this critical issue. This should be fixed in v1.8.1.

@brentyi
Copy link
Contributor

brentyi commented Apr 25, 2021

Thanks @AWhetter!! This will be super useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants