search

rigetti / qiskit-rigetti

Qiskit provider serving Rigetti hardware & simulator backends.
Apache License 2.0
7 stars 7 forks source link

Breaking: Added Sphinx documentation, converted all docstrings to Google style, tightened public API. #8

Closed dbanty closed 3 years ago

dbanty commented 3 years ago

Breaking Changes

The breaking change here is to tighten up the public API and only export things from one spot (e.g. instead of exporting RigettiQCSProvider from both qiskit_rigetti_provider and qiskit_rigetti_provider.qcs_provider it's only exported from the top module. Doing this early should prevent some of the mess we get in pyQuil with extra paths. It could be that the ideal way to shape this API is not the way I shaped in, so it's worth poking around to be sure.

Docstring Changes

There are a few changes to docstrings to make them play nice:

  1. They are all converted to Google style
  2. Any follow-on to the initial description of a class was pushed downward. Particularly the literal blocks describing gates. This is in following standards for docstrings where there should be a single standalone line summarizing the class. Without this change (putting :: down a couple lines) Sphinx does some weird stuff in the summaries.
  3. The "constructs a ..." line in __init__ functions for classes was removed. This is because the docstring of __init__ is treated as an extension of the Class' docstring. I might have missed a couple of those but the intention was to get rid of them so the only thing in the __init__ docstrings are Args, Raises, etc.

Sphinx

The main components of the docs are:

  1. Sphinx, and a couple Makefile rules to build/serve the docs locally
  2. The furo theme just because I think it looks nice and sort of GitBook-ish, should line up fairly well with our other documentation with a bit of theming.
  3. MyST Parser for allowing Markdown. Right now it's just including the README.md in index, but custom docs can be written in Markdown now.
  4. sphinx.ext.napoleon to allow Google-style docstrings
  5. sphinx-autoapi which is an alternative to autodoc which works nicer (doesn't actually import the code it's documenting).

How to test this?

From the project root do make docs && make serve-docs then go to http://localhost:8000

ameyer-rigetti commented 3 years ago

Overall this is looking great. Some other things I'm thinking about:

dbanty commented 3 years ago

Can you configure this to publish somewhere, like RTD, similar to what pyQuil does? (and ensure the check runs on PRs, which could remove the need to add an explicit CI job to test make docs)

I think in order to do RTD this repo needs to be open source. Are we ready to do that yet?

The theme is great! Is there a way for users to select dark/light?

It's based on your system theme! I see the light version, you must be using the dark system theme (or auto and viewing at night).

Some items don't seem to be links like I would expect (e.g. Apply qiskit_rigetti_provider.gates.xy.XYGate on the docs for QuilCircuit.xy)

There are some pages that seem duplicated.

Ick... I think it's doing both of those things because it has the same behavior as autodoc & mkdocstrings in that it uses the fully qualified path for imported types 🙄 rather than using their re-exported path. I'll see if I can find a way to bypass that.

dbanty commented 3 years ago

@ameyer-rigetti I believe everything other than deploying is resolved, and we can't deploy until this repo is public.

ameyer-rigetti commented 3 years ago

I think in order to do RTD this repo needs to be open source. Are we ready to do that yet?

OK, we can wait on this then (maybe just a brief GHA job to check that make docs succeeds in the meantime?). We'll publish this soon, but will wait until after to get docs publishing. Given this, would you also document the make task in the readme, so users can view the docs in the meantime?

It's based on your system theme! I see the light version, you must be using the dark system theme (or auto and viewing at night).

🤯 😮 . Still wonder if there's a way to override -- as I think the light will be readable for most. Just my preference/opinion though, so willing to budge.

Ick... I think it's doing both of those things because it has the same behavior as autodoc & mkdocstrings in that it uses the fully qualified path for imported types 🙄 rather than using their re-exported path. I'll see if I can find a way to bypass that.

Not a blocker. This is all still better than no docs and is something we could improve.

dbanty commented 3 years ago

Given this, would you also document the make task in the readme, so users can view the docs in the meantime?

It already is 😁.

🤯 😮 . Still wonder if there's a way to override -- as I think the light will be readable for most. Just my preference/opinion though, so willing to budge.

I think we should defer to what the users want, since that's the reason the browser setting exists and is considered best practice. If we do want to override it with this theme we'd have to manually set all the colors for the dark theme to something lighter (https://pradyunsg.me/furo/customisation/colors/#how-light-and-dark-mode-work).

Note that every browser defaults to light mode unless the user has set it to dark mode. So in my case (with Firefox) I specifically set it to adapt to the system theme, so when my system is dark so are all my websites (e.g. GitHub).

If the dark mode is hard to read, maybe we should just adjust the colors on that mode to still be dark but also be more readable?

ameyer-rigetti commented 3 years ago

@dbanty OK, no worries then. Not worth it to update. I personally like the dark theme on my macOS but prefer light webpages, but I'm guessing users could still change the browser setting independently. Perhaps all that we should do here is simply document in the readme that the light/dark is dependent on user OS/browser settings.

ameyer-rigetti commented 3 years ago

Also note that I merged a new PR. My apologies that there now may be a few more docs strings to update

ameyer-rigetti commented 3 years ago

Also note that I merged a new PR. My apologies that there now may be a few more docs strings to update

BTW, with this change comes some new packages: hooks.pre_compilation and hooks.pre_execution. I'm not sure the best approach regarding a single import path (like what you did for other packages/classes), as I'd like to preserve these paths because it makes it clearer whether the hook is meant for thebefore_execute or before_compile option when running a circuit (vs importing from the top-level, where the names don't distinguish whether it relates to compile vs execute). Hope that makes sense.

dbanty commented 3 years ago

Ready for re-review @ameyer-rigetti

dbanty commented 3 years ago

@ameyer-rigetti the typo has been fixed and pytest will run doctests now (good thing, since the example was causing an exception 😅. I also changed around the development docs in the README since:

  1. Running type checking on Python > 3.7 fails
  2. We should probably mention Poetry since it's required to get set up

It also occurs to me that our usage of Makefile is not super friendly to Windows users, so maybe we should link to a recommended way to install GNU make on that platform? I don't have a recommendation, but maybe someone does?

ameyer-rigetti commented 3 years ago

@dbanty

It also occurs to me that our usage of Makefile is not super friendly to Windows users, so maybe we should link to a recommended way to install GNU make on that platform?

Yeah, I don't like make in general for this exact reason. Your suggestion in the meantime sounds like a reasonable mitigation.

rigetti-githubbot commented 3 years ago

:tada: This PR is included in version 0.3.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: