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

Switch to using the sphinx-autoapi extension for auto building the API reference. It better maintained and produces nicer formatted docs. #1030

Merged
merged 14 commits into from
Oct 4, 2024

Conversation

cdbf1
Copy link
Contributor

@cdbf1 cdbf1 commented Aug 16, 2024

The sphinx auto-summary, auto-doc and apidoc extensions have more recently been improved upon by the single extension sphinx-autoapi which I think is simpler and build nicer looking documentation.

This PR updates our docs to use sphinx-autoapi

@vtomole vtomole self-requested a review August 29, 2024 16:27
Copy link
Member

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

After running ./checks/build_docs.py locally I get

WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.AQTICCX in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.AQTITOFFOLI in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.CR in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.ZX in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.AQTICCX in cirq_superstaq
WARNING: Cannot resolve import of cirq_superstaq.ops.AQTITOFFOLI in cirq_superstaq
WARNING: Cannot resolve import of cirq_superstaq.ops.CR in cirq_superstaq
WARNING: Cannot resolve import of cirq_superstaq.ops.ZX in cirq_superstaq

but the docs do indeed build. I think it's fine to merge this and fix the warnings in follow-up PRs.

@cdbf1
Copy link
Contributor Author

cdbf1 commented Sep 20, 2024

After running ./checks/build_docs.py locally I get

WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.AQTICCX in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.AQTITOFFOLI in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.CR in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.qubit_gates.ZX in cirq_superstaq.ops
WARNING: Cannot resolve import of cirq_superstaq.ops.AQTICCX in cirq_superstaq
WARNING: Cannot resolve import of cirq_superstaq.ops.AQTITOFFOLI in cirq_superstaq
WARNING: Cannot resolve import of cirq_superstaq.ops.CR in cirq_superstaq
WARNING: Cannot resolve import of cirq_superstaq.ops.ZX in cirq_superstaq

but the docs do indeed build. I think it's fine to merge this and fix the warnings in follow-up PRs.

Turns out Sphinx didn't like the double = signs so I changed that. I also fixed (hopefully!) all the other warnings that Sphinx was generating.

It might be worth in future having the build fail if there are any Sphinx warnings.

Copy link
Contributor

@richrines1 richrines1 left a comment

Choose a reason for hiding this comment

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

adding a few nits - leaving the big picture review to @vtomole

we should also check that this works w/ the other repo before merging

gss.SuperstaqServerException: If unable to get the status of the job from the API or
cancellations were unsuccessful.
general_superstaq.SuperstaqServerException: If unable to get the status of the job
from the API orcancellations were unsuccessful.
Copy link
Contributor

Choose a reason for hiding this comment

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

honestly i kinda like the smaller indent here so maybe it's worth reconsidering, but i think our style specifies four spaces instead of three for these indents

Suggested change
from the API orcancellations were unsuccessful.
from the API orcancellations were unsuccessful.

(same occurs elsewhere in this pr)

Copy link
Contributor Author

@cdbf1 cdbf1 Sep 26, 2024

Choose a reason for hiding this comment

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

This was an oversight on my part sorry. I've always used 3-spaces for indenting docstrings and thought this was a requirement, when it turns out its not. I have hopefully now fixed all the places I modified this

Comment on lines 163 to 164
general_superstaq.SuperstaqServerException: If unable to get the status of the job
from the API.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this allowed?

Suggested change
general_superstaq.SuperstaqServerException: If unable to get the status of the job
from the API.
SuperstaqServerException: If unable to get the status of the job from the API.

not super important but it'd be nice not to have to write out general_superstaq in docstrings when we use gss everywhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is possible but I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I managed to fix this so we can continue using gss, css, and qss in docstrings. Note that the ~ which I have now included is there to make the compiled documentation look nicer by suppressing the general_superstaq in the return type.

@@ -33,16 +32,7 @@ def run(*args: str, sphinx_paths: list[str] | None = None) -> int:

docs_dir = os.path.join(check_utils.root_dir, "docs")

Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to add something like

Suggested change
if not os.path.isdir(os.path.join(docs_dir, "source")):
print(check_utils.warning("No docs to build."))
return 0

@vtomole
Copy link
Member

vtomole commented Oct 3, 2024

@cdbf1 to confirm that this doesn't affect server docs, then this is good to go.

@cdbf1
Copy link
Contributor Author

cdbf1 commented Oct 4, 2024

@cdbf1 to confirm that this doesn't affect server docs, then this is good to go.

@vtomole see the minor fixes that are needed for server-superstaq here: https://github.com/Infleqtion/server-superstaq/pull/3407

Copy link
Member

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

LGTM

@cdbf1 cdbf1 merged commit b0cebf7 into main Oct 4, 2024
20 checks passed
@cdbf1 cdbf1 deleted the feature/move_to_sphinx_autoapi branch October 4, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants