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

Methods are documented on class pages #10455

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Jul 20, 2023

Currently, methods have their own HTML page. This PR changes them to live on the class page.

Motivations:

  1. Speed up our docs build. Before: 8 minutes, 20 seconds. After: 4 minutes, 30 seconds.
  2. Better user experience. While it's true we sometimes have very long method entries, most of our methods are small and it's a frustrating user experience to have to change pages so much. Compare this to e.g. Python docs, where the entire module is documented on a single page. An IBM design team suggested we make this change.

A follow-up will move functions to their module page, rather than dedicated pages.

No redirects

We decided it's not worth adding redirects for method pages. Sphinx will already properly update all cross-references to the right place, and we don't expect there to be many external links to method pages.

@Eric-Arellano Eric-Arellano changed the title [wip] Methods are documented on class pages Methods are documented on class pages Jul 20, 2023
@Eric-Arellano Eric-Arellano marked this pull request as ready for review July 20, 2023 15:40
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

@Eric-Arellano
Copy link
Collaborator Author

Preview the docs at https://dev.azure.com/qiskit-ci/b6f351e4-6a09-4bf6-b029-4b7806a7bde7/_apis/build/builds/48160/artifacts?artifactName=html_docs&api-version=7.0&%24format=zip. Note that the design is improved with the upcoming Furo theme. This PR unblocks it.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5612762310

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 86.083%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 91.65%
Totals Coverage Status
Change from base Build 5611896973: 0.002%
Covered Lines: 72947
Relevant Lines: 84740

💛 - Coveralls

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty inline with the minimal change I was expecting.

Is it worth completely removing the loops from the class summary templates, and switching the :no-members: option to :members:, so autodoc manages it all itself? I've found raw autodoc to be quite sane, and for it to generally improve over time as well.

@coruscating
Copy link

Is it possible to add links on the right sidebar for each section (Attributes, Methods, etc.)? For example, QuantumCircuit is now 40+ pages long without any subheadings on the right sidebar, and I find it pretty difficult to navigate.

@jakelishman
Copy link
Member

For QuantumCircuit in particular, we'll probably want to write its documentation a bit more manually than the autosummary format because it's so long, it could really do with a bit more human interaction to group the methods into logical groups.

Changing the .. rubric:: lines to a heading directive instead (e.g. underlining "Attributes" with ------------) should add toctree entries to the page, though I can't remember off the top of my head how well docutils will interact with the text-templating that autosummary does to insert these if there's already headings in the summarised page. I think it works ok because autosummary always creates a new page, but it'd need testing.

@Eric-Arellano
Copy link
Collaborator Author

Is it worth completely removing the loops from the class summary templates, and switching the :no-members: option to :members:, so autodoc manages it all itself?

I don't think so because we want the .. rubric directive to add the heading for methods vs attributes.

--

@coruscating that's a Pytorch problem. The example docs use the exact same templates at this PR:

Screenshot 2023-07-20 at 9 59 47 AM

Screenshot 2023-07-20 at 10 00 44 AM

This PR and the followup to remove functions both unblock us switching to Furo in the metapackage - we'll switch over right away after.

Further, this change doesn't go "live" until we do the final 0.25 release because the metapackage builds off of releases, not main. So, I don't think this UX problem will actually impact users in reality.

@coruscating
Copy link

@Eric-Arellano That's good to see the new theme automatically adds links to individual attributes and methods, though I'd prefer to have actual headings like "Methods" and "Attributes" that they're listed under, otherwise it'll be a very long unsorted list on some pages.

@jakelishman We tried changing .. rubric:: to headings in Experiments, but that broke things because of unhappy interactions between napoleon and Sphinx: qiskit-community/qiskit-experiments#1077 (comment). I'm not sure if this would also be a problem here.

@jakelishman
Copy link
Member

Helena: the reason it might be different for you compared to us is that you guys were (or at least appeared to be) modifying a docstring that was embedded into more free-form other pages, where you didn't have control on what other section-heading constructs were used. In the current setup, autosummary generates a new page for anything that triggers the class.rst template, and that page has a very fixed structure, so there (probably) aren't any other headings of the same logic level with which we might conflict if we unilaterally pick a punctuation mark to use in the heading here.

@jakelishman
Copy link
Member

Eric: in my vein of "raw autodoc is usually pretty sensible": there's an option to control the grouping (though admittedly, I don't know if it adds section headings).

@Eric-Arellano Eric-Arellano added documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: API Change Include in the "Changed" section of the changelog labels Jul 20, 2023
@Eric-Arellano Eric-Arellano added this to the 0.25.0 milestone Jul 20, 2023
@mtreinish mtreinish added Changelog: None Do not include in changelog and removed Changelog: API Change Include in the "Changed" section of the changelog labels Jul 20, 2023
@Eric-Arellano
Copy link
Collaborator Author

Good find @jakelishman. But looks like we don't want to use it. With this template:

{{ objname | escape | underline }}

.. currentmodule:: {{ module }}

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

That means we won't have rubrics/headers and the members will appear as subheaders rather than top-level entries:

Screenshot 2023-07-20 at 11 12 26 AM

--

though I'd prefer to have actual headings like "Methods" and "Attributes" that they're listed under, otherwise it'll be a very long unsorted list on some pages.

I was able to get headers working rather than rubrics:

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

But, it doesn't impact the right sidebar at all.

Screenshot 2023-07-20 at 11 15 54 AM

@jakelishman
Copy link
Member

Ah, that's a shame. I wonder if autodoc would consider that as a feature request, if it's the only thing keeping us from using it. They've generally been pretty receptive to me in the past, I think - if we like the idea, I can take it up with them.

@Eric-Arellano
Copy link
Collaborator Author

if we like the idea, I can take it up with them.

As long as we, of course, don't block this PR on it, sure! Upstreaming config is always nice.

@jakelishman
Copy link
Member

Yeah, totally fine by me - I don't even think we're using Sphinx 7 yet.

@jakelishman
Copy link
Member

I'd have to look into why changing the templates to insert a heading doesn't impact the sidebar, but whether or not the documented objects themselves get inserted into the toctree is controlled by the toc_object_entries global Sphinx option.

@Eric-Arellano
Copy link
Collaborator Author

but whether or not the documented objects themselves get inserted into the toctree is controlled by the toc_object_entries global Sphinx option.

Yeah, and we're using the default of True already.

--

To clarify, is anything blocking this PR from approval? (Other than time / trying to get out rc1, of course)

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from my side - I'd forgotten I hadn't done it.

@jakelishman jakelishman removed this pull request from the merge queue due to a manual request Jul 20, 2023
@jakelishman
Copy link
Member

I'm taking this out the merge queue because it'd use time that we need for the rc1 release right now. We can add it after that.

@Eric-Arellano Eric-Arellano added this pull request to the merge queue Jul 21, 2023
Merged via the queue into Qiskit:main with commit e55389f Jul 21, 2023
mergify bot pushed a commit that referenced this pull request Jul 21, 2023
@Eric-Arellano Eric-Arellano deleted the no-method-pages branch July 21, 2023 21:56
github-merge-queue bot pushed a commit that referenced this pull request Jul 21, 2023
(cherry picked from commit e55389f)

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
to24toro pushed a commit to to24toro/qiskit-terra that referenced this pull request Aug 3, 2023
github-merge-queue bot pushed a commit to qiskit-community/qiskit-experiments that referenced this pull request Nov 6, 2023
### Summary
Moved all the methods and attributes into class pages to speed up the
building process of the documentation with the new ecosystem sphinx
theme.

This PR continues the work done by @Eric-Arellano in
#1231.

See Qiskit/qiskit#10455 for more information.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
github-merge-queue bot pushed a commit to qiskit-community/qiskit-experiments that referenced this pull request Nov 6, 2023
### Summary
Moved all the methods and attributes into class pages to speed up the
building process of the documentation with the new ecosystem sphinx
theme.

This PR continues the work done by @Eric-Arellano in
#1231.

See Qiskit/qiskit#10455 for more information.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
github-merge-queue bot pushed a commit to qiskit-community/qiskit-experiments that referenced this pull request Nov 6, 2023
### Summary
Moved all the methods and attributes into class pages to speed up the
building process of the documentation with the new ecosystem sphinx
theme.

This PR continues the work done by @Eric-Arellano in
#1231.

See Qiskit/qiskit#10455 for more information.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
github-merge-queue bot pushed a commit to qiskit-community/qiskit-experiments that referenced this pull request Nov 6, 2023
### Summary
Moved all the methods and attributes into class pages to speed up the
building process of the documentation with the new ecosystem sphinx
theme.

This PR continues the work done by @Eric-Arellano in
#1231.

See Qiskit/qiskit#10455 for more information.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
nkanazawa1989 pushed a commit to nkanazawa1989/qiskit-experiments that referenced this pull request Jan 17, 2024
### Summary
Moved all the methods and attributes into class pages to speed up the
building process of the documentation with the new ecosystem sphinx
theme.

This PR continues the work done by @Eric-Arellano in
qiskit-community#1231.

See Qiskit/qiskit#10455 for more information.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants