Qiskit / qiskit-metapackage

Qiskit is an open-source SDK for working with quantum computers at the level of circuits, algorithms, and application modules.
https://qiskit.org
Apache License 2.0
3.03k stars 750 forks source link

[WAIT TO MERGE] Add extension to add deprecated directives #1680

Closed Eric-Arellano closed 1 year ago

Eric-Arellano commented 1 year ago

Summary

Rework of https://github.com/Qiskit/qiskit-terra/pull/8600.

Qiskit repos can add the attribute __qiskit_deprecations__ to any function object, which is done automatically by the @deprecated decorators (https://github.com/Qiskit/qiskit-terra/pull/9611 adds this for Terra). Then, this PR's extension knows to look for that attribute and converts it into Sphinx's deprecated directives.

This design frees up docstring authors from needing to directly insert deprecated directives in their docstring, which is finicky to format correctly and would require duplicating information from the runtime warnings they already have.

Details and comments

This renders all deprecations in-between the function's docstring and any metadata, like arguments and the return type. There can be >1 deprecation - each will be its own warning box.

For example (although this doesn't have the Sphinx Theme formatting):

Screenshot 2023-02-23 at 2 24 28 PM

Sometimes, we deprecate arguments rather than the function itself. Originally, I was trying to get deprecations for arguments to show up in the "Parameters" section for its entry, like this:

Screenshot 2023-02-23 at 2 35 25 PM

But, this requires a bad hack: to get the deprecation directive to render, we must remove any docstring written for that parameter. For example, this docstring gives important context for anyone who has not yet migrated:

Screenshot 2023-02-23 at 2 32 03 PM

(Trying to get the deprecation to show up next to the parameter also resulted in complex and confusing code.)

So instead, the deprecation is rendered in-between the function docstring and the parameter list:

Screenshot 2023-02-23 at 2 37 13 PM
javabster commented 1 year ago

@jakelishman in response to your top level comment I'd be hesitant to add this extension to the qiskit_sphinx_theme as it takes the control away from individual repos about whether they want to use this extension or not. In future as we move towards different documentation/theming implementations for Qiskit and the Qiskit Ecosystem it's possible other ecosystem projects might not want to standardise something like this with qiskit itself (although truthfully the picture is still murky here).

On a technical note, I'm not sure how a sphinx theme interacts with extensions or if it's even possible to have it implement an extension on a target project. If my (admittedly quite limited) understanding is correct extensions are usually imported and run from the conf.py before the theme itself is added. As far as I'm aware the sphinx theme doesn't install any custom extensions itself currently, and if it is possible it may take some additional work to implement. If you have any deeper knowledge on this or suggestions lmk 😄

jakelishman commented 1 year ago

I'm fine with it not being in the Sphinx theme if you think it's better, but I do think it needs to go in a package that's directly installable during the build processes for any module that does want to use it (if not only because in this current form, it's only usable by the metapackage). I'd also thought that "deprecation messages" were something you maybe did want to consider standardising across components as part of one coherent "docs" experience, but sure, I don't feel strongly and that's your decision 👍.

For "installable extensions": sure, they can 100% be packaged up and deployed to PyPI, then you load them by putting their import paths in extensions in conf.py. That's what sphinx_autodoc_typehints does, for example. This one isn't being built into a proper packaged extension yet, it's just using the ad-hoc binding mechanism by adding hooks directly in conf.py, but the principle is exactly the same.

Eric-Arellano commented 1 year ago

In future as we move towards different documentation/theming implementations for Qiskit and the Qiskit Ecosystem it's possible other ecosystem projects might not want to standardise something like this with qiskit itself

My vague understanding is that people still must explicitly opt into our extension in their conf.py.

I'd also thought that "deprecation messages" were something you maybe did want to consider standardising across components as part of one coherent "docs" experience

This is something I'm interested in. Today, I'm trying to improve Terra's deprecation.py so that it can cover more edge cases and be used by Applications. (Or, if they don't like those changes, I can always refactor their own deprcation helpers to set __qiskit_deprecations__)

but I do think it needs to go in a package that's directly installable during the build processes for any module that does want to use it

If we made it an installable package on this meta-package, then we would get a circular dependency whenever Terra and friends depend on the meta-package.

So, I think we need to either

a) go with this PR for now and Deal With It Later if it's proving problematic, or b) add it to the Sphinx theme

I don't have any major time crunches, so I'd bias towards me doing the more correct thing of adding to the Theme.

jakelishman commented 1 year ago

Just to clarify: when I say "installable package", I'm meaning as in "from PyPI with its own name" (i.e. not being a part of the PyPI packages qiskit or qiskit_sphinx_theme, even if the source code happens to live in the same place), so there wouldn't be any bidirectionality concerns. We can deploy one or more completely separate packages to PyPI from the same repo. I don't mind which repo that is particularly, if that's the way we go, though I think it being in the same repo as the Sphinx theme makes general sense.

Eric-Arellano commented 1 year ago

Superseded by https://github.com/Qiskit/qiskit-terra/pull/9685.