Skip to content

Add docs infrastructure#13

Merged
mergify[bot] merged 36 commits into
qiskit-community:mainfrom
ElePT:add-docs-1
Jul 26, 2023
Merged

Add docs infrastructure#13
mergify[bot] merged 36 commits into
qiskit-community:mainfrom
ElePT:add-docs-1

Conversation

@ElePT
Copy link
Copy Markdown
Collaborator

@ElePT ElePT commented Jul 24, 2023

Summary

This PR adds the docs infrastructure for the API reference and tutorials. The templates are taken from Qiskit/qiskit#10455.

Details and comments

  • The tutorial added is only to test that the infrastructure works. A follow-up PR will handle the actual tutorial migration from qiskit-tutorials.
  • Missing:
  • Github Actions setup

@ElePT ElePT requested review from Cryoris and woodsp-ibm as code owners July 24, 2023 09:26
Comment thread docs/conf.py
@ElePT
Copy link
Copy Markdown
Collaborator Author

ElePT commented Jul 24, 2023

When I build the docs locally, the API Reference tab does not appear in the sidebar, I can only access it if I click on the .html directly... Any thoughts?

Screenshot 2023-07-24 at 11 28 53

@ElePT ElePT added this to the 0.1.0 milestone Jul 24, 2023
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 24, 2023

Pull Request Test Coverage Report for Build 5671046568

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.544%

Totals Coverage Status
Change from base Build 5659909752: 0.0%
Covered Lines: 6741
Relevant Lines: 7445

💛 - Coveralls

@ElePT
Copy link
Copy Markdown
Collaborator Author

ElePT commented Jul 24, 2023

I see that the CI fails on sphinx-build -M html docs docs/_build -W -T --keep-going here, but I am not able to reproduce this failure locally. Is there anything I am missing regarding tags and/or releasenotes? [Edit: tried to fix with bd2c87e, but it looks like I lack permissions to run ignore_untagged_notes.sh].

@woodsp-ibm
Copy link
Copy Markdown
Member

but it looks like I lack permissions to run ignore_untagged_notes.sh].

Does it have the executable attribute set?

@woodsp-ibm
Copy link
Copy Markdown
Member

I am missing regarding tags and/or releasenotes

Maybe try adding a release note - could do a prelude one and put text that is similar explanation to what Terra is saying about the deprecation/move. We need something like that here anyway. Not sure if it will fix things or not.

@woodsp-ibm
Copy link
Copy Markdown
Member

When I build the docs locally, the API Reference tab does not appear in the sidebar, I can only access it if I click on the .html directly... Any thoughts?

What version of the qiskit-sphinx-theme do you have. I think the apps are set with this qiskit_sphinx_theme~=1.12.0 - not sure about other settings in the conf.py without checking through them. The requirements allow > 1.11 and I know Eric has been altering to be more restrictive. Here its pulling in the latest 1.13.1 which may require some conf changes. Its based on Furo now I believe.


jobs:
docs_publish:
if: ${{ startsWith(github.ref, 'refs/heads/stable') && contains('["mtreinish","Cryoris","ElePT","woodsp-ibm"]', github.actor) }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why this part about the maintainer?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docs deploy is manually run which publishes them to the live site - we didn't want just anyone to be able to run this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Only people with write access could run it though. Is it necessary to have the extra check?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As far as I am aware anyone can run actions - we set it up for the apps and this is what they have. One could do some more checking again. For now I want to keep it.

run: |
echo "earliest_version: 0.1.0" >> releasenotes/config.yaml
tools/ignore_untagged_notes.sh
make html
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm surprised these aren't using Tox. I'd recommend that because it will move the configuration of your third-party dependencies (the virtualenv) into tox.ini rather than this YAML file. tox -e docs will install all the right requirements already without CI needing to know about e.g. pip install jupyter.

Copy link
Copy Markdown
Member

@woodsp-ibm woodsp-ibm Jul 24, 2023

Choose a reason for hiding this comment

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

Since the VM is created for each job and requirements etv installed into that base it was all done via makes rather than have yet another virtual env on top of that. I am trying to recall why pip install jupyter - I think it was just a workaround as some transitive dependent that used to be installed did not. Either way this all needs to get setup in an expedited fashion and following what we have working in the apps was the chosen path. Things can be toxified if wanted later, we need to get this all setup and running asap to have the release in the next couple of days.

Comment on lines +86 to +87
make clean_sphinx
make html
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto using tox

name: documentation
path: docs/_build/html/artifacts/documentation.tar.gz
if: ${{ !cancelled() }}
- run: make doctest
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto tox

run: |
echo "earliest_version: 0.1.0" >> releasenotes/config.yaml
tools/ignore_untagged_notes.sh
make html
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto tox. But also this is much slower than necessary. This will rebuild all the docs. Instead, you only need to run the tutorials to check if they've broken. See Qiskit/qiskit#10443 for how I use nbconvert.

Comment thread docs/conf.py

# -- Options for Doctest --------------------------------------------------------

doctest_default_flags = (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you using doctest?

Copy link
Copy Markdown
Collaborator Author

@ElePT ElePT Jul 24, 2023

Choose a reason for hiding this comment

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

It's ran but not used anywhere yet. I think it could be useful if we added a "getting started" page with a code example like the apps modules do.

@@ -0,0 +1,338 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did this tutorial already exist? Or it's new content?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is from qiskit-tutorials, not a history-preserving copy, but I thought we could do that in a follow-up.

Comment thread requirements-dev.txt Outdated
Comment thread requirements-dev.txt Outdated
Comment thread tox.ini Outdated
ElePT and others added 4 commits July 24, 2023 16:48
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Comment thread tools/ignore_untagged_notes.sh Outdated
Comment thread requirements-dev.txt Outdated
Comment thread requirements-dev.txt
Comment thread .github/workflows/deploy-docs.yml Outdated
Comment thread docs/conf.py Outdated
Comment thread docs/conf.py Outdated
Comment thread .github/workflows/deploy-docs.yml Outdated
Comment thread tools/deploy_documentation.sh Outdated
@woodsp-ibm
Copy link
Copy Markdown
Member

@Eric-Arellano With this theme the links in the sidebar, when they refer to modules, are not matching whats in the page. In the page, since they are sub-folders of the current content, they have just the name. In the sidebar they are fully qualified paths which as you go down the structure gets longer and longer. Any thought

image

Also, the notebook that is here, as a placeholder to have all the algorithms tutorials, looking at the tutorials in the html output it does not come out properly as a tile (you can download a build artifact and see - either say the documentation zip or a tutorials one where they are built out in the content, rather than being included as-is). Are we missing some config for this theme - I did not double check myself yet - had been more concerned with getting things working.

@ElePT
Copy link
Copy Markdown
Collaborator Author

ElePT commented Jul 25, 2023

Answering #13 (comment) et al not to leave all of the tox comments hanging: I think it could be good to simplify the setup, but given that we now have a runnable CI and limited time before the release, I would not like to break it again just now. I would leave these improvements for a follow-up.

Comment thread docs/conf.py
Comment thread docs/getting_started.rst Outdated
Comment thread docs/index.rst
.. toctree::
:hidden:

API References <apidocs/algorithms>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
API References <apidocs/algorithms>
About Qiskit Algorithms <self>
Getting Started <getting_started>
API References <apidocs/algorithms>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A minor comment. In the apps the reference text is 'Overview' which is also what is on the page. As Qiskit Algorithms is at the top of the sidebar there and on the page, having it yet again there in the text seems rather repetitive as hopefully the context is clear. But its easy enough to change at any point of course, at least this adds the link back to the start which was missing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't have a strong preference :) Others use Documentation Home for that reason. Overview works for me too.

@Eric-Arellano
Copy link
Copy Markdown
Contributor

Also, the notebook that is here, as a placeholder to have all the algorithms tutorials, looking at the tutorials in the html output it does not come out properly as a tile (you can download a build artifact and see - either say the documentation zip or a tutorials one where they are built out in the content, rather than being included as-is). Are we missing some config for this theme - I did not double check myself yet - had been more concerned with getting things working.

Oof, it looks like you're hitting Qiskit/qiskit_sphinx_theme#518. I've only been able to figure that out for one project, which was removing sphinx-tabs. But you're not using sphinx-tabs. It will require more debugging.

Note that it is a problem on the Pytorch theme too, only not as obvious because of some styling Pytorch uses. Experiments didn't have time to debug so copied in the CSS file directly to work around it: qiskit-community/qiskit-experiments#1225.

With this theme the links in the sidebar, when they refer to modules, are not matching whats in the page. In the page, since they are sub-folders of the current content, they have just the name. In the sidebar they are fully qualified paths which as you go down the structure gets longer and longer. Any thought

I'm not reproducing that. Perhaps you have to scroll down on the left sidebar more?

Screenshot 2023-07-25 at 1 18 07 PM

@woodsp-ibm
Copy link
Copy Markdown
Member

With this theme the links in the sidebar, when they refer to modules, are not matching whats in the page. In the page, since they are sub-folders of the current content, they have just the name. In the sidebar they are fully qualified paths which as you go down the structure gets longer and longer. Any thought

I'm not reproducing that. Perhaps you have to scroll down on the left sidebar more?

I mean the names in the sidebar eg are qiskit_algorithms.gradients whereas in the page for qiskit_algorithms its just gradients as that is the sub-module from that context. While technically its right in the sidebar its seems odd that the for the classes its just their name - ie not prefixed with qiskit_algorithms etc - yet the module is a fully qualified name

image

My take on time_evolvers would have just been to have a link to time_evolvers where variational and trotterization were modules documented there rather than hoist it to the level it is. Either way again its a fully qualified name in the sidebar rather than relative.

@Eric-Arellano
Copy link
Copy Markdown
Contributor

Eric-Arellano commented Jul 25, 2023

The Furo theme highlights whatever the current page is. If you are at apidocs/algorithms.html, and scroll down your page to see the Gradients subsection, you'll have your screenshot:

Screenshot 2023-07-25 at 1 44 04 PM

That's as expected. Now, click on the Gradients link either from the page or the left sidebar, and you go to the page stubs/qiskit_algorithms.gradients.html, with this view:

Screenshot 2023-07-25 at 1 45 05 PM

This is all expected. It's how the base Furo theme and autodoc/autosummary work.

@woodsp-ibm
Copy link
Copy Markdown
Member

Yes but I am talking about whats in the sidebar - it just seems off that the navigation in the page has gradients as the entry, since its in the context of the qiskit_algorithms page, but the corresponding nested entry in sidebar is fully qualified qiskit_algorithm.gradients - since its all nested that full qualification seems unneeded - the classes there as I mentioned do not have the fully qualified path name. But I get once you land on the page it links to, then then full context is there in the page.

@Eric-Arellano
Copy link
Copy Markdown
Contributor

Yes but I am talking about whats in the sidebar - it just seems off that the navigation in the page has gradients as the entry, since its in the context of the qiskit_algorithms page, but the corresponding nested entry in sidebar is fully qualified qiskit_algorithm.gradients - since its all nested that full qualification seems unneeded - the classes there as I mentioned do not have the fully qualified path name. But I get once you land on the page it links to, then then full context is there in the page.

If you wanted to change the left sidebar, you would need to figure out how to get autodoc/autosummary to not set the page title for each module page to be the fully qualified module. The Sphinx theme simply uses whatever the page title is for the left sidebar.

@woodsp-ibm
Copy link
Copy Markdown
Member

woodsp-ibm commented Jul 25, 2023

Ok so its more like alter the title of the target page to be that same name, as long as the full context is there near the top it does not have to be in the title - none of the classes pages are that way. Anyway it was just something that jumped out at me - something for later though if we want to change things

@woodsp-ibm
Copy link
Copy Markdown
Member

@Eric-Arellano I see the search output is a little different. Before if I seached for VQE (like now in the terra docs) it gives hits like PVQD and Minimizer which it still does. The difference was before is that additional text was present so you could see the hit in context e.g search in current Qiskit (terra API ref for VQE) which may be enough for you to want to check it out or skip/

image

here all I see is and I guess I just have to assume it found VQE in that link somewhere.

image

Is this output as expected, do we need to configure anything further....

@woodsp-ibm woodsp-ibm mentioned this pull request Jul 25, 2023
8 tasks
@ElePT
Copy link
Copy Markdown
Collaborator Author

ElePT commented Jul 26, 2023

Note that it is a problem on the Pytorch theme too, only not as obvious because of some styling Pytorch uses. Experiments didn't have time to debug so copied in the CSS file directly to work around it: qiskit-community/qiskit-experiments#1225.

Thanks @Eric-Arellano. I tried copying over the CSS file and running make html locally and it doesn't seem to fix the issue in this case. It might be because the sources of the issue aren't the sphinx-tabs. As this tutorial is just a placeholder, and seeing that this might be difficult to debug, I would not worry much about it now, and maybe try again to fix it (or work around it) when the tutorials are officially migrated to this repo. It worked!!

ElePT and others added 7 commits July 26, 2023 17:43
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
@mergify mergify Bot merged commit 63886f9 into qiskit-community:main Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants