Qiskit / Qiskit/documentation

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

Open
#2,346 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug 🐛 infra 🏗️
Dominant language
Jupyter Notebook
Stars
106
Forks
223
Avg merge
1d 8h
Merged PRs (30d)
72

Description

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:

https://github.com/Qiskit/documentation/blob/298d8a3fcf84c741cd04ab372db4f4ee9dd2bfc7/docs/api/qiskit-addon-mpf/backends-tenpy-tebd-tebd-evolver.mdx?plain=1#L40-L44

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

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the Autosummary class template referenced in the issue and compare its generated MDX example with the source property declaration. Then inspect the qiskit/documentation script that inserts headings. Done means generated class pages retain property return types while restoring the Attributes and Methods headings, with the inline-class behavior decision resolved.

Written by the indexing model from the issue text.

Assessment

Domain
documentation, tooling
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.