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

Prepare release notes for Qiskit Terra 0.19 #7329

Merged
merged 40 commits into from
Dec 6, 2021

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Nov 30, 2021

Summary

The main object of this PR is to move the release notes for 0.19 into a separate folder, and to reformat them into a consistent style with correct formatting and working Sphinx cross-references.

In doing so, I also came across some places with missing documentation, and a couple of places where the Sphinx documentation was being generated twice for quite a few objects, which messed with the ability to cross-reference correctly. This PR is then actually in a few commits, which may be viewed separately to see the changes more easily:

  • move the release notes into releasenotes/notes/0.19
  • add objects that were missing from the API docs/module imports completely (that looked like they should have been included - I didn't do it indiscriminately)
  • updated a couple of deprecation notices that incorrectly said 0.18 due to their milestone getting moved
  • reorganised the qiskit.pulse documentation to avoid duplicating definitions of all its objects (this could be split into a separate PR)
  • actually reformatted the release notes

Details and comments

Most of the release notes changes are mechanical - fixing typos, formatting and cross-reference links. In a few cases, there was insufficient context in the release note itself when read in isolation, so those I mostly rewrote. If you want to review the changes only, you may want to filter the diff only to the single commit named "Reformat 0.19 release notes" (its hash may change if I rebase).

The main documentation addition was hooking the approximate quantum compiler into the actual tree. This then necessitated a couple of fixes in its documentation to account for Sphinx errors that had previously gone unnoticed. Adding a few things into the import path created some cyclical imports, so those are moved into runtime imports.

While the changes to the qiskit.pulse documentation look big, in reality, the rendered documentation is almost identical (because it's already good - I didn't want to mess with it!). I just rearranged how it gets generated, and which docstrings are responsible for various parts of it. The main developer-facing change is that now there is only one canonical reference path for every object in the qiskit.pulse namespace. The builder objects, Schedule and ScheduleBlock are in the top-level qiskit.pulse, and most other stuff is in qiskit.pulse.instructions, .library, .transforms, etc (with no further nesting within these). This means that suffix references will now generally work (e.g. :class:`.Gaussian` will now actually resolve, and be equal to :class:`~qiskit.pulse.library.Gaussian`). This should also help users searching for documentation, because there won't be duplication in the results.


This is likely going to be the last PR for 0.19, and the one that gets tagged as the release. On hold til all other PRs are in, at which point I'll rebase it and write a release prelude.

Todo before merge:

  • merge in remaining 0.19 PRs and move release notes
  • rewrite QASM 3 and control-flow release notes to reference the builder interface/promote control-flow more

A few objects added in new features for the 0.19 cycle did not get added
into higher-level `__init__` files, either as imports or documentation
links.  This adds several documentation links in, and fixes some
cyclic imports that were previously hidden due to the objects not being
fully imported.
These deprecation notices were written in a PR that was originally
targetted for the 0.18 release but later got shifted to 0.19.
Previously, autosummary documentation files were being generated for
most objects in `qiskit.pulse` in two namespaces: `qiskit.pulse` and
(e.g.) `qiskit.pulse.library`.  In other cases, the module-level
documentation was not being generated at all.  This moves the
responsibility for defining all the definitions into the respective
submodules, and turns the `qiskit.pulse` docstring into a series of
`.. automodule::` calls.  The relevant module docstrings are merged
with any content that was already in the `qiskit.pulse` docstring, to
ensure that the displayed documentation on the `qiskit.pulse` page is
almost identical to what it was before.

Various Sphinx crossreferences within the `pulse` module (and referring
to it from elsewhere) were previously broken, in part because of the
duplication.  It should be more reliable when creating crossreferences
now.  The abstract base classes are also added in `.. autoclass::` calls
in suitable locations, since they are a common source of failed Sphinx
crossreferences elsewhere.

(It's easy to accidentally get the duplication when using `autosummary`.
If it does need to be used on more than one page, then only the
canonical path should use the `:toctree:` directive.  Pulse also had
more of this than other places because it's generally a bit more
thoroughly documented.)
This reformats the release notes for Terra 0.19, canonicalising the
style and ensuring that all Sphinx crossreferences link correctly.  In
some cases there is no suitable target for documentation (especially in
the case of removals), so these are left as simple monospaced font.
@jakelishman jakelishman added on hold Can not fix yet documentation Something is not clear or an error documentation priority: high labels Nov 30, 2021
@jakelishman jakelishman added this to the 0.19 milestone Nov 30, 2021
@jakelishman jakelishman added this to To do in Documentation via automation Nov 30, 2021
@coveralls
Copy link

coveralls commented Nov 30, 2021

Pull Request Test Coverage Report for Build 1545764687

  • 25 of 25 (100.0%) changed or added relevant lines in 18 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 83.093%

Totals Coverage Status
Change from base Build 1544860931: 0.001%
Covered Lines: 51668
Relevant Lines: 62181

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I just went through the doc changes to start here are some quick comments. I'm gonna take a break before starting the release note meat of this PR to start preparing a fix commit for those 2 broken transpiler passes before we're committed to the API in the release.

docs/apidocs/transpiler_builtin_plugins.rst Outdated Show resolved Hide resolved
docs/apidocs/transpiler_builtin_plugins.rst Show resolved Hide resolved
qiskit/pulse/__init__.py Show resolved Hide resolved
qiskit/pulse/instructions/__init__.py Show resolved Hide resolved
qiskit/transpiler/passes/calibration/builders.py Outdated Show resolved Hide resolved
@jakelishman
Copy link
Member Author

I'm going to leave the changes for now, just to ease the CI - I've got them done locally, but I'll not push til more stuff has merged.

@jakelishman jakelishman closed this Dec 1, 2021
Documentation automation moved this from To do to Done Dec 1, 2021
@jakelishman jakelishman reopened this Dec 1, 2021
@jakelishman
Copy link
Member Author

Well, I was going to leave things, then I accidentally closed the PR rather than just commenting, and re-opening re-triggered the build anyway. So now they're done.

@1ucian0
Copy link
Member

1ucian0 commented Dec 4, 2021

I dont thinks is possible to suggest changes in unmodified moved files. So Ill drop them here:

  • add-backend-v2-ce84c976fb13b038.yaml:33: rest of Qiskit can use today with any backend. Subclasses of the abstract

@jakelishman
Copy link
Member Author

Resolved all Luciano's most recent comments in 1423ebf. The styling issue I think needs fixing in our theme.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Almost done with my final pass (need to step away for a bit right now) so I'll likely have some other comments when I get back

Comment on lines 28 to 35
Along with this change, a schedule was added to
:class:`~qiskit.pulse.InstructionScheduleMap` and is implicitly updated with
a metadata ``"publisher"``. Backend-calibrated gate schedules have a
special publisher kind to avoid overriding circuits with calibrations of
already known schedules. Usually, end-users don't need to take care of this
metadata as it is applied automatically. You can call
:meth:`.InstructionScheduleMap.has_custom_gate` to check if the map has
custom gate calibration.
Copy link
Member

Choose a reason for hiding this comment

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

Which change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a bit of a rewrite in a40c877, but I may have got some of that not 100% correct. Would be good if someone with more knowledge could check.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

We can remove gate-type-hints-b287215190144ceb.yaml it's not really a useful release note and if we did want to document it should be a new feature and not an upgrade note as it's not breaking anyone.

I still need to go through the fix notes, but they're not as important so we can ship it as is if needed.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I quickly skimmed through the fix notes and caught a few small things but for the most part it looked okay. I think once the issues I commented on before and these are fixed we should be good to go on this.

@mtreinish mtreinish removed the on hold Can not fix yet label Dec 6, 2021
@mtreinish
Copy link
Member

mtreinish commented Dec 6, 2021

I've removed the on hold here because all the release PRs are merged. Once we're done with the release notes this is ready to merge and we can tag the release.

@jakelishman
Copy link
Member Author

All changes now addressed (including removing the gate type hints note). I rewrote the PulseGate note a little bit (conversation still unresolved above), so it'd be good for someone with more knowledge to check that.

mtreinish
mtreinish previously approved these changes Dec 6, 2021
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, it looks like you addressed all the comments thanks for the quick turnaround.

For the pulse gate release note if it's not clear we can always update it after the release. I think at this point it's good enough in it's current form.

@mtreinish mtreinish added automerge Changelog: None Do not include in changelog labels Dec 6, 2021
This was made in order to simplify some import paths, but as a side
effect it caused ten `QuantumCircuit`s to be created on import of
`qiskit`, which spoiled the QPY backwards compatibility tests and likely
had import performance implications.

The import of this likely should not need to create circuits, but for
now, we revert its import rather than making a change this close to
release.
@mergify mergify bot merged commit d5094ee into Qiskit:main Dec 6, 2021
@jakelishman jakelishman deleted the fix/release-notes-0.19 branch December 6, 2021 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog documentation Something is not clear or an error documentation priority: high
Projects
Documentation
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants