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

Dedicated class pages are missing type hints for properties because of autosummary .. autoproperty:: quirk #2346

Open
Eric-Arellano opened this issue Nov 19, 2024 · 0 comments

Comments

@Eric-Arellano
Copy link
Collaborator

Eric-Arellano commented Nov 19, 2024

Problem: missing type hints

With Sphinx's autosummary extension, we're using .. autoattribute:: for functions marked with @property, rather than .. autoproperty:::

https://github.com/Qiskit/qiskit-addon-obp/blob/2412cb29c9d314a5ad568acdd5be558f7053c0f2/docs/_templates/autosummary/class.rst?plain=1#L20

This messes up the return type for properties. For example, the return type is defined on this @property:

https://github.com/Qiskit/qiskit-addon-mpf/blob/191dc8162510719ca3b50e5a25e9a1453419629a/qiskit_addon_mpf/backends/tenpy_tebd/evolver.py#L59-L62

However, the Sphinx output—and thus our docs—are missing that return type:

### evolved\_time
<Attribute id="qiskit_addon_mpf.backends.tenpy_tebd.TEBDEvolver.evolved_time">
Returns the current evolution time.
</Attribute>

This is fixed if we use .. autoproperty::; Sphinx will correctly set the return type.

So, why not use .. autoproperty::?

This is specifically an issue when we use .. autosummary:: to document a class. Properties work correctly when you use .. autoclass::, such as for inline class documentation in a module page.

The issue is that our template tells .. autoclass:: to not document any members so that we can instead manually document them by calling .. autoattribute and .. automethod:

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}
   :no-members:
   :no-inherited-members:
   :no-special-members:
   :show-inheritance:

{% block attributes_summary %}
  {% if attributes %}
   .. rubric:: Attributes
    {% for item in attributes %}
   .. autoattribute:: {{ item }}
    {%- endfor %}
  {% endif %}
{% endblock -%}

{% block methods_summary %}
  {% set wanted_methods = (methods | reject('==', '__init__') | list) %}
  {% if wanted_methods %}
   .. rubric:: Methods
    {% for item in wanted_methods %}
   .. automethod:: {{ item }}
    {%- endfor %}
  {% endif %}
{% endblock %}

In our Autosummary template, we manually list out every member so that we can add the lines .. rubric:: Attributes and .. rubric:: Methods, which get converted into h2 headings. These headings give important hierarchy to class pages that we do not want to lose:

Image

So, we cannot simply use .. autoclass:: to list all the members for us because we would lose the Attributes and Methods headings.

Ideally, we could do something like this:

{% block attributes_summary %}
  {% if attributes %}
   .. rubric:: Attributes
    {% for item in attributes %}
       {% if item is property %}
   .. autoproperty:: {{ item}}
       {% else %}
   .. autoattribute:: {{ item }}
       {% endif %}
    {%- endfor %}
  {% endif %}
{% endblock -%}

However, that type of if statement will not work because attributes is simply a list of strings, with both normal attributes and properties, such as ['foo', 'bar', 'my_prop']. There is no way in the Jinja template to determine whether the string corresponds to an @property, and Sphinx fails if you try using any Python builtins like dir() or hasattr(). What we really would need is for Autosummary to inject a properties variable, but it's not available: https://www.sphinx-doc.org/en/master/usage/extensions/autosummary.html#customizing-templates.

Rejected workaround: manually list every class

This wouldn't be a problem if we had docs authors give up on autosummary and manually call .. autoclass::, .. autoproperty::, .. autoattribute::, and .. automethod::, along with adding .. rubric:: Attributes and .. rubric:: Methods where appropriate. However, that is not realistic to expect.

Proposed solution: our script

We will switch the APIs to use .. autoclass:: in their Autosummary template to document their members, rather than manually iterating through them. Their template will look like this:

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}
   :show-inheritance:

That will fix the property issue, but now we'll be missing the Attributes and Methods h2 headings! So, we will have our qiskit/documentation script add back those headings to our MDX files in the appropriate location.

TBD:

  • Decide whether we should add the Attributes and Methods headings for inline classes on module pages.
    • Downside: increase the heading hierarchy for those classes.
    • Upside: more clear what is a method vs. attribute/property
@Eric-Arellano Eric-Arellano changed the title Autosummary template needs to use .. autoproperty:: for properties Dedicated class pages are missing type hints for properties because of autosummary .. autoproperty:: quirk Nov 21, 2024
Eric-Arellano added a commit to Qiskit/qiskit-ibm-runtime that referenced this issue Nov 25, 2024
Closes #1877. We now use `EstimatorPubLike` and `SamplerPubLike` for the type hints, rather than Sphinx fully expanding the types into extremely long values shown in #1877.

To fix the type hints, we have to stop using `sphinx-autodoc-typehints` and instead use autodoc's builtin support for type hints. I didn't realize we were still using `sphinx-autodoc-typehints` - the other Qiskit projects migrated off a while ago.

This PR also makes some other improvements to align with changes we made in Qiskit Addons & qiskit-ibm-transpiler:

* always show inheritance in class pages for the base class
* index page should show the PyPI package name in code syntax
* don't show signatures in `autosummary` tables, since they're noisy
* show type hints for parameters that don't have docstring

## Introduces a regression: type hints for properties

Unfortunately, switching off of `sphinx-autodoc-typehints` results in Qiskit/documentation#2346. This is a quirk of autosummary that we cannot easily fix. We have a plan to fix it, although realistically won't get to it until Q1 and possibly Q2.

In the meantime, consider repeating some of the type information in the docstring. For example, in

```
<Attribute id="qiskit_ibm_runtime.EstimatorV2.mode">
    Return the execution mode used by this primitive.
```

You could rewrite to say

```
<Attribute id="qiskit_ibm_runtime.EstimatorV2.mode">
    Return the execution mode used by this primitive, either :class:~`.Session` or ~:class:~`.Batch`.
```
Eric-Arellano added a commit to Eric-Arellano/qiskit-ibm-runtime that referenced this issue Nov 25, 2024
Closes Qiskit#1877. We now use `EstimatorPubLike` and `SamplerPubLike` for the type hints, rather than Sphinx fully expanding the types into extremely long values shown in Qiskit#1877.

To fix the type hints, we have to stop using `sphinx-autodoc-typehints` and instead use autodoc's builtin support for type hints. I didn't realize we were still using `sphinx-autodoc-typehints` - the other Qiskit projects migrated off a while ago.

This PR also makes some other improvements to align with changes we made in Qiskit Addons & qiskit-ibm-transpiler:

* always show inheritance in class pages for the base class
* index page should show the PyPI package name in code syntax
* don't show signatures in `autosummary` tables, since they're noisy
* show type hints for parameters that don't have docstring

## Introduces a regression: type hints for properties

Unfortunately, switching off of `sphinx-autodoc-typehints` results in Qiskit/documentation#2346. This is a quirk of autosummary that we cannot easily fix. We have a plan to fix it, although realistically won't get to it until Q1 and possibly Q2.

In the meantime, consider repeating some of the type information in the docstring. For example, in

```
<Attribute id="qiskit_ibm_runtime.EstimatorV2.mode">
    Return the execution mode used by this primitive.
```

You could rewrite to say

```
<Attribute id="qiskit_ibm_runtime.EstimatorV2.mode">
    Return the execution mode used by this primitive, either :class:~`.Session` or ~:class:~`.Batch`.
```
kt474 pushed a commit to Qiskit/qiskit-ibm-runtime that referenced this issue Nov 25, 2024
)

Closes #1877. We now use `EstimatorPubLike` and `SamplerPubLike` for the type hints, rather than Sphinx fully expanding the types into extremely long values shown in #1877.

To fix the type hints, we have to stop using `sphinx-autodoc-typehints` and instead use autodoc's builtin support for type hints. I didn't realize we were still using `sphinx-autodoc-typehints` - the other Qiskit projects migrated off a while ago.

This PR also makes some other improvements to align with changes we made in Qiskit Addons & qiskit-ibm-transpiler:

* always show inheritance in class pages for the base class
* index page should show the PyPI package name in code syntax
* don't show signatures in `autosummary` tables, since they're noisy
* show type hints for parameters that don't have docstring

## Introduces a regression: type hints for properties

Unfortunately, switching off of `sphinx-autodoc-typehints` results in Qiskit/documentation#2346. This is a quirk of autosummary that we cannot easily fix. We have a plan to fix it, although realistically won't get to it until Q1 and possibly Q2.

In the meantime, consider repeating some of the type information in the docstring. For example, in

```
<Attribute id="qiskit_ibm_runtime.EstimatorV2.mode">
    Return the execution mode used by this primitive.
```

You could rewrite to say

```
<Attribute id="qiskit_ibm_runtime.EstimatorV2.mode">
    Return the execution mode used by this primitive, either :class:~`.Session` or ~:class:~`.Batch`.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

1 participant