[WAIT TO MERGE] Add extension to add deprecated directives #1680
Conversation
Eric-Arellano
requested review from
mtreinish,
jakelishman,
kevinhartman,
HuangJunye and
javabster
as code owners
There was a problem hiding this comment.
Thanks for getting started on this. I left some comments in line but there's one more that's pretty over-arching that might need some thought: this adds additional complexity to the docs build process, including the possibility of invalid/incomplete metadata in the subrepositories causing a docs build failure. That alone is fine for an extension, but the problem is that we won't be testing against that in CI, which means we may well not detect problems until it's time to release the metapackage.
I think we might need to rethink the organisation of things a bit here, possibly even moving this extension into qiskit_sphinx_theme rather than being here. It's suboptimal that at the moment we don't attempt a full metapackage docs build on all CI, but we're already in the process of disentangling that mess. I think we need these more complex extensions to be more widely available, so they can be pulled in during component CI, but also because putting them here means that anything that deploys its documentation separately to the metapackage (which is most things) won't be able to access this. Perhaps the best strategy is to either put them with qiskit_sphinx_theme (despite the name) or to start a new package qiskit_sphinx_extensions that the theme / all docs builds can depend on?
| if what not in {"function", "method"} or not hasattr(obj, "__qiskit_deprecations__"): | ||
| return |
There was a problem hiding this comment.
More of a note than a change request: this won't catch places where an undecorated method overrides a deprecated method in a base class, but that's the same situation with our existing decorators (we require subclasses explicitly deprecate their methods if appropriate too), so probably doesn't need a change here.
docs/custom_extensions.py
Outdated
| meta_index = next( | ||
| ( | ||
| i for i, line | ||
| in enumerate(lines) | ||
| if any(line.startswith(prefix) for prefix in meta_prefixes) | ||
| ), | ||
| None, | ||
| ) | ||
| if meta_index and not (meta_index >= 1 and lines[meta_index - 1] == ""): |
There was a problem hiding this comment.
I'm a bit confused about meta_index here: I think it's initially about whether the meta fields are found, and if so, where, but we only really use it to reason about the insertion point. However, the meta_index being found at position 0 and not found at all are handled the same throughout this logic, which I'm not super sure was intended, especially because there's a superfluous test (if meta_index, then meta_index >= 1 is always true, since meta_index can only be 0, None or positive).
I think the logic throughout this might be clearer if we replace meta_index with an integer that's always the insertion point, and always include a blank line at the end of new_lines if it's non-empty - there's no harm in having extra full-blank lines (I'm relatively sure 2+ full blanks are treated equally to 1 full blank). Similarly, I think we can always just stick a blank line in front of it on insertion - you can keep your test below to check for edge cases, but on actual insertion this is probably simpler.
I think there's maybe a bug in the logic currently: if the doc string is exactly 1 line long with no fields, then I think the .. deprecated will get inserted immediately adjacent to the summary line, which is either a parse error or semantically wrong depending on whether it goes before or after (though I didn't fully attempt it, so I could be wrong).
There was a problem hiding this comment.
To expand on the last paragraph - we can super easily get into that situation with something like
def clear(self):
"""Remove all internal objects from this container."""There was a problem hiding this comment.
However, the meta_index being found at position 0
Based on my testing in ibmq-quantum-provider, this is not possible. If there are only meta lines, there will at least be a single "" before the meta line. So, meta_index cannot be 0.
I think there's maybe a bug in the logic currently: if the doc string is exactly 1 line long with no fields, then I think the .. deprecated will get inserted immediately adjacent to the summary line
In this case, the docstring will have a line, followed by "" after it. The code will add the deprecation to the very end. No bug.
--
I think the logic throughout this might be clearer if we replace meta_index with an integer that's always the insertion point, and always include a blank line at the end of new_lines if it's non-empty
Yeah, I think your intuition is right. I agree this is too intricate, and it's fine if the output has extra blank lines, so long as it renders the same to end users. I will confirm this and simplify - thank you!
There was a problem hiding this comment.
For the "bug": in my example for it, there's no trailing blank line, so I think relying on one getting inserted somewhere by napoleon is fragile, because there's not one required by reST.
I think it might be a good idea to rework the tests somehow / add integration tests to invoke the entire docstring processing on them, rather than us providing what we expect the normalised output to look like.
| # This code is part of Qiskit. | ||
| # |
There was a problem hiding this comment.
The location of this file is an unusual structure for Qiskit projects, and I think it'd be better to use the same style as them: we use a separate test tree, rather than interspersing the test code with the driver code.
(I'm not making any particular judgement about which is better were we starting afresh, just saying we probably ought to be consistent.)
There was a problem hiding this comment.
Ah fair. I'm used to tests living right next to the file, which makes it really easy to see what has tests and doesn't. But, I agree that it's more important to follow convention.
|
|
||
| from docs.custom_extensions import add_qiskit_deprecation |
There was a problem hiding this comment.
I probably ought to have spotted this before, but this import makes me nervous. docs isn't an actual Python package (nor should it be), so this import only works because of the implicit namespace package behaviour, and because we only run the test runners / whatever from working directories where docs is in . and Python implicitly adds that to sys.path.
We can maybe address this in a follow-up, but I think we maybe want to formalise this into a proper Python package, especially as the logic for our extensions gets longer. (See also top review comment.)
docs/custom_extensions_test.py
Outdated
| # Check that we correctly insert the directive in-between the function description | ||
| # and metadata args. See | ||
| # https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists. | ||
| for metadata_line in [ | ||
| ":param foo:", | ||
| ":parameter foo:", | ||
| ":arg foo:", | ||
| ":argument foo:", | ||
| ":key foo:", | ||
| ":keyword foo:", | ||
| ":type foo:", | ||
| ":raise ValueError:", | ||
| ":raises ValueError:", | ||
| ":except ValueError:", | ||
| ":exception ValueError:", | ||
| ":return: blah", | ||
| ":returns: blah", | ||
| ":rtype: blah", | ||
| ]: |
There was a problem hiding this comment.
You maybe want to use ddt to parametrise a separate test case for these guys. (It's not as powerful as pytest's built-in parametrisation handling, but works just fine with unittest.)
| python -m unittest -v # Runs on the `test` folder. | ||
| python -m unittest -v docs/custom_extensions_test.py |
There was a problem hiding this comment.
Mentioned above, but normal Qiskit structure would be to put these tests into test/docs (or similar).
There was a problem hiding this comment.
Thank you for the thoughtful review!
Perhaps the best strategy is to either put them with qiskit_sphinx_theme (despite the name) or to start a new package qiskit_sphinx_extensions that the theme / all docs builds can depend on?
Putting it in qiskit_sphinx_extensions sounds reasonable to me. That's originally where I was planning on adding this, but Luciano pointed me to this repo since it's where all extensions are currently.
I come from a monorepo background, so I'm +1 for using qiskit_sphinx_theme rather than Yet Another Repo, which requires copy and pasting all of our config files etc.
docs/custom_extensions.py
Outdated
| meta_index = next( | ||
| ( | ||
| i for i, line | ||
| in enumerate(lines) | ||
| if any(line.startswith(prefix) for prefix in meta_prefixes) | ||
| ), | ||
| None, | ||
| ) | ||
| if meta_index and not (meta_index >= 1 and lines[meta_index - 1] == ""): |
There was a problem hiding this comment.
However, the meta_index being found at position 0
Based on my testing in ibmq-quantum-provider, this is not possible. If there are only meta lines, there will at least be a single "" before the meta line. So, meta_index cannot be 0.
I think there's maybe a bug in the logic currently: if the doc string is exactly 1 line long with no fields, then I think the .. deprecated will get inserted immediately adjacent to the summary line
In this case, the docstring will have a line, followed by "" after it. The code will add the deprecation to the very end. No bug.
--
I think the logic throughout this might be clearer if we replace meta_index with an integer that's always the insertion point, and always include a blank line at the end of new_lines if it's non-empty
Yeah, I think your intuition is right. I agree this is too intricate, and it's fine if the output has extra blank lines, so long as it renders the same to end users. I will confirm this and simplify - thank you!
| # This code is part of Qiskit. | ||
| # |
There was a problem hiding this comment.
Ah fair. I'm used to tests living right next to the file, which makes it really easy to see what has tests and doesn't. But, I agree that it's more important to follow convention.
|
@jakelishman in response to your top level comment I'd be hesitant to add this extension to the 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 |
|
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 |
My vague understanding is that people still must explicitly opt into our extension in their
This is something I'm interested in. Today, I'm trying to improve Terra's
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 I don't have any major time crunches, so I'd bias towards me doing the more correct thing of adding to the Theme. |
|
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 |
Eric-Arellano
changed the title
|
Superseded by Qiskit/qiskit#9685. |
Summary
Rework of Qiskit/qiskit#8600.
Qiskit repos can add the attribute
__qiskit_deprecations__to any function object, which is done automatically by the@deprecateddecorators (Qiskit/qiskit#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):
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:
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:
(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: