Finish the rest of the type annotations for quil.py #1134

rht · 2019-12-24T13:59:16Z

Description

Continuation of the effort to annotate quil.py.

Checklist

The above description motivates these changes.
There is a unit test that covers these changes.
All new and existing tests pass locally and on Semaphore.
Parameters have type hints with PEP 484 syntax.
Functions and classes have useful sphinx-style docstrings.
(New Feature) The docs have been updated accordingly.
(Bugfix) The associated issue is referenced above using
auto-close keywords.
The changelog is updated,
including author and PR number (@username, gh-xxx).

rht · 2019-12-24T14:04:06Z

While annotating Program, I encountered a method instructions which is not initialized anywhere. But the _instructions method exists. Is instructions actually _instructions?

pyquil/quil.py

karalekas

Couple quick things. Also, you'll probably notice that we've added some additional CI checks (see https://github.com/rigetti/pyquil/blob/master/CONTRIBUTING.md#style-guidelines). Finally, because the goal of this PR is to finish off the quil.py file, it should be added to the list here in the Makefile (https://github.com/rigetti/pyquil/blob/master/Makefile#L71) and thus verified as part of the CI.

karalekas · 2019-12-24T16:09:27Z

CHANGELOG.md

@@ -8,7 +8,7 @@ Changelog

 ### Improvements and Changes

-   Added some type hints to the `Program` class (@rht, gh-1115).
+-   Type hints have been added to the `Program` class (@rht, gh-1115, gh-1134)


Suggested change

- Type hints have been added to the `Program` class (@rht, gh-1115, gh-1134)

- Type hints have been added to the `quil.py` file (@rht, gh-1115, gh-1134).

you're doing the rest of the file right?

Yes, the rest of the file.

But there are 52 mypy errors, some are caused by another files that makes it impossible to reduce in this PR. How should I reword the commit message and the changelog message?

Down to 32 errors.

pyquil/quil.py

appleby · 2019-12-24T16:48:29Z

While annotating Program, I encountered a method instructions which is not initialized anywhere. But the _instructions method exists. Is instructions actually _instructions?

Program.instructions is defined as an @property around line 132.

karalekas · 2019-12-24T17:54:27Z

Rebase off of master now that I've merged #1133 to get the updated CI stuff. Check out the updated contributing guide as well to come up to speed if you're confused :)

rht · 2019-12-25T10:41:25Z

I have reformatted my commit with Black.

rht · 2019-12-26T21:38:49Z

@appleby

pyquil/pyquil/quil.py

Line 372 in 2f61735

ro = self.declare("ro", "BIT", max(qubit_inds) + 1)

has a mypy error of

pyquil/quil.py:383: error: Unsupported operand types for + ("Qubit" and "int")
pyquil/quil.py:383: error: Unsupported operand types for + ("QubitPlaceholder" and "int")

because max(qubit_inds) has a type of QubitDesignator, and there is no + operation for Qubit and QubitPlaceholder. Should it be written that Program.get_qubits() returns Set[int] instead of Set[QubitDesignator]?

rht · 2019-12-27T00:25:21Z

@karalekas I reworded the changelog and commit message by saying "some" so that this PR can be merged. The increased precision in *instructions alone makes this PR worth merging since it will decrease mypy errors in paulis.py.

appleby · 2019-12-27T01:12:07Z

@appleby

pyquil/pyquil/quil.py

Line 372 in 2f61735

ro = self.declare("ro", "BIT", max(qubit_inds) + 1)

has a mypy error of
pyquil/quil.py:383: error: Unsupported operand types for + ("Qubit" and "int")
pyquil/quil.py:383: error: Unsupported operand types for + ("QubitPlaceholder" and "int")
because max(qubit_inds) has a type of QubitDesignator, and there is no + operation for Qubit and QubitPlaceholder. Should it be written that Program.get_qubits() returns Set[int] instead of Set[QubitDesignator]?

I don't think we should change the return type because Program.get_qubits really does return Set[QubitDesignator] as far as I can tell. That is, _extract_qubit_index might return a QubitPlaceholder even when index = True. This is probably an actual bug. For example:

>>> p = Program(H(QubitPlaceholder()))
>>> p.get_qubits(indices=True)
{<QubitPlaceholder 4840608152>}
>>> p.measure_all()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-110-8be3d209b112> in <module>
----> 1 p.measure_all()

~/src/repos/rigetti/pyquil/pyquil/quil.py in measure_all(self, *qubit_reg_pairs)
    370             if len(qubit_inds) == 0:
    371                 return self
--> 372             ro = self.declare("ro", "BIT", max(qubit_inds) + 1)
    373             for qi in qubit_inds:
    374                 self.inst(MEASURE(qi, ro[qi]))

TypeError: unsupported operand type(s) for +: 'QubitPlaceholder' and 'int'

appleby · 2019-12-27T01:15:54Z

Maybe Program.measure_all() should declare an ro register of size len(qubit_inds), rather than max(qubit_inds) + 1, and then loop over enumerate(qubit_inds) instead? But this feels like an issue for a separate PR...

appleby · 2019-12-27T01:19:51Z

I don't think we should change the return type because Program.get_qubits really does return Set[QubitDesignator] as far as I can tell. That is, _extract_qubit_index might return a QubitPlaceholder even when index = True. This is probably an actual bug.

Or maybe the expectation is that you've already called address_qubits on your Program before you call measure_all, idk.

appleby

github won't let me comment on lines without any changes, but the following diff around line 105 of quil.py removed some mypy errors for me:

@@ -105,12 +106,12 @@ class Program(object):
         # labels so the program must first be have its labels instantiated.
         # _synthesized_instructions is simply a cache on the result of the _synthesize()
         # method.  It is marked as None whenever new instructions are added.
-        self._synthesized_instructions = None
+        self._synthesized_instructions: Optional[List[AbstractInstruction]] = None
 
         self.inst(*instructions)
 
         # Filled in with quil_to_native_quil
-        self.native_quil_metadata = None  # type: NativeQuilMetadata
+        self.native_quil_metadata : Optional[NativeQuilMetadata] = None
 
         # default number of shots to loop through
         self.num_shots = 1

pyquil/quil.py

rht · 2019-12-30T16:23:09Z

In the 7th commit, I ignored the type checking for the function address_qubit because mypy can't fully understand the conditionals in that function. Now we are down to only 7 mypy errors!

appleby

Couple more small changes to resolve minor mypy errors.

There are still some errors left, but they require more thought / discussion on how (and whether) to resolve them.

pyquil/quil.py

rht · 2019-12-30T16:30:42Z

@appleby there are 5 errors left as of the 8th commit.

appleby

Thanks for your patience @rht . Please apply the following diff, which gets the number of mypy errors down to zero and adds quil.py to the list of files checked by make check-types. I will ping karalekas to see what he thinks about the changes in measure_all.

diff --git a/Makefile b/Makefile
index 399e231a..77c3e260 100644
--- a/Makefile
+++ b/Makefile
@@ -16,7 +16,8 @@ check-format:
 # The dream is to one day run mypy on the whole tree. For now, limit checks to known-good files.
 .PHONY: check-types
 check-types:
-	mypy pyquil/quilatom.py pyquil/quilbase.py pyquil/gates.py pyquil/noise.py
+	mypy pyquil/quilatom.py pyquil/quilbase.py pyquil/gates.py \
+		pyquil/noise.py pyquil/quil.py
 
 .PHONY: check-style
 check-style:
diff --git a/pyquil/quil.py b/pyquil/quil.py
index 8491f087..06d6f127 100644
--- a/pyquil/quil.py
+++ b/pyquil/quil.py
@@ -19,7 +19,7 @@ Module for creating and defining Quil programs.
 import itertools
 import types
 import warnings
-from collections import OrderedDict, defaultdict
+from collections import defaultdict
 from typing import (
     Any,
     Dict,
@@ -32,6 +32,7 @@ from typing import (
     Set,
     Tuple,
     Union,
+    cast,
     no_type_check,
 )
 
@@ -128,7 +129,8 @@ class Program(object):
         new_prog = Program()
         new_prog._defined_gates = self._defined_gates.copy()
         if self.native_quil_metadata is not None:
-            new_prog.native_quil_metadata = self.native_quil_metadata.copy()
+            # TODO: remove this type: ignore once rpcq._base.Message gets type hints.
+            new_prog.native_quil_metadata = self.native_quil_metadata.copy()  # type: ignore
         new_prog.num_shots = self.num_shots
         return new_prog
 
@@ -390,9 +392,18 @@ class Program(object):
                   MEASURE 2 [3]
         """
         if qubit_reg_pairs == ():
-            qubit_inds = self.get_qubits(indices=True)
-            if len(qubit_inds) == 0:
+            qubits = self.get_qubits(indices=True)
+            if len(qubits) == 0:
                 return self
+            if any(isinstance(q, QubitPlaceholder) for q in qubits):
+                raise ValueError(
+                    "Attempted to call measure_all on a Program that contains QubitPlaceholders. "
+                    "You must either provide the qubit_reg_pairs argument to describe how to map "
+                    "these QubitPlaceholders to memory registers, or else first call "
+                    "pyquil.quil.address_qubits to instantiate the QubitPlaceholders."
+                )
+            # Help mypy determine that qubits does not contain any QubitPlaceholders.
+            qubit_inds = cast(List[int], qubits)
             ro = self.declare("ro", "BIT", max(qubit_inds) + 1)
             for qi in qubit_inds:
                 self.inst(MEASURE(qi, ro[qi]))
@@ -632,9 +643,11 @@ class Program(object):
         if any(not isinstance(instr, Gate) for instr in self._instructions):
             raise ValueError("Program to be daggered must contain only gate applications")
 
-        # This is a bit hacky. Gate.dagger() mutates the gate object,
-        # rather than returning a fresh (and daggered) copy.
-        return Program([instr.dagger() for instr in reversed(Program(self.out())._instructions)])
+        # This is a bit hacky. Gate.dagger() mutates the gate object, rather than returning a fresh
+        # (and daggered) copy. Also, mypy doesn't understand that we already asserted that every
+        # instr in _instructions is a Gate, above, so help mypy out with a cast.
+        surely_gate_instructions = cast(List[Gate], Program(self.out())._instructions)
+        return Program([instr.dagger() for instr in reversed(surely_gate_instructions)])
 
     def _synthesize(self) -> "Program":
         """
@@ -743,7 +756,7 @@ def _what_type_of_qubit_does_it_use(
     # We probably want to index qubits in the order they are encountered in the program
     # so an ordered set would be nice. Python doesn't *have* an ordered set. Use the keys
     # of an ordered dictionary instead
-    qubits = OrderedDict()
+    qubits = {}
 
     for instr in program:
         if isinstance(instr, Gate):
@@ -795,7 +808,9 @@ def get_default_qubit_mapping(program: Program) -> Dict[Union[Qubit, QubitPlaceh
         warnings.warn(
             "This program contains integer qubits, so getting a mapping doesn't make sense."
         )
-        return {q: q for q in qubits}
+        # _what_type_of_qubit_does_it_use ensures that if real_qubits is True, then qubits contains
+        # only real Qubits, not QubitPlaceholders. Help mypy figure this out with cast.
+        return {q: cast(Qubit, q) for q in qubits}
     return {qp: Qubit(i) for i, qp in enumerate(qubits)}

appleby · 2019-12-30T17:26:13Z

pyquil/quil.py

@@ -771,7 +799,10 @@ def get_default_qubit_mapping(program):
    return {qp: Qubit(i) for i, qp in enumerate(qubits)}


-def address_qubits(program, qubit_mapping=None):
+@no_type_check


Personally I think this is fine, but pining @karalekas to make sure he sees it.

I think it's possible to satisfy mypy here by splashing around a bunch of casts, but it's ugly and probably not worth it imo, so I'm fine with just ignoring this function.

rht · 2019-12-30T18:56:38Z

I have applied the diff via 9th commit.

rht · 2019-12-30T18:59:46Z

@appleby I got 2 errors left even with the 9th commit:

pyquil/quil.py:133: error: unused 'type: ignore' comment
pyquil/quil.py:409: error: Invalid index type "Union[Qubit, QubitPlaceholder, int]" for "MemoryReference"; expected type "int"
Found 2 errors in 1 file (checked 1 source file)

appleby

See comments for fixes to the remaining two errors.

pyquil/quil.py

rht · 2019-12-30T19:16:47Z

@appleby I still got this error locally:

pyquil/quil.py:133: error: unused 'type: ignore' comment
Found 1 error in 1 file (checked 1 source file)

appleby · 2019-12-30T19:22:06Z

@appleby I still got this error locally:

pyquil/quil.py:133: error: unused 'type: ignore' comment
Found 1 error in 1 file (checked 1 source file)

Strange, if I remove the # type: ignore comment I get the following error when run locally:

pyquil/quil.py:133: error: Call to untyped function (unknown) in typed context

Semaphore's make check-types seems to be happy. How are you invoking mypy?

rht · 2019-12-30T19:23:42Z

@appleby more detail:

 pyquil (mypy) ✗  mypy pyquil/quil.py 
pyquil/quil.py:133: error: unused 'type: ignore' comment
Found 1 error in 1 file (checked 1 source file)
 pyquil (mypy) ✗  mypy --version                                                                                           1 ↵
mypy 0.761

appleby · 2019-12-30T19:26:55Z

@appleby more detail:

 pyquil (mypy) ✗  mypy pyquil/quil.py 
pyquil/quil.py:133: error: unused 'type: ignore' comment
Found 1 error in 1 file (checked 1 source file)
 pyquil (mypy) ✗  mypy --version                                                                                           1 ↵
mypy 0.761

Interesting. This must be a difference between mypy 0.740 and 0.761. You'll notice we recently pinned the mypy version to 0.740 in requirements.txt. You might want to reinstall mypy.

rht · 2019-12-30T19:27:05Z

Should the pinned version of mypy be updated to 0.761 from 0.740?

appleby · 2019-12-30T19:30:18Z

Should the pinned version of mypy be updated to 0.761 from 0.740?

No, there is a bug in mypy 0.750+, which is why we pinned to 0.740. According to mypy contributors, the bugfix won't be in a release until 0.770. See #1124.

appleby

LGTM. Thanks @rht!

appleby · 2019-12-30T19:45:49Z

pyquil/quil.py

+            qubits = self.get_qubits(indices=True)
+            if len(qubits) == 0:
                return self
+            if any(isinstance(q, QubitPlaceholder) for q in qubits):
+                raise ValueError(
+                    "Attempted to call measure_all on a Program that contains QubitPlaceholders. "
+                    "You must either provide the qubit_reg_pairs argument to describe how to map "
+                    "these QubitPlaceholders to memory registers, or else first call "
+                    "pyquil.quil.address_qubits to instantiate the QubitPlaceholders."
+                )
+            # Help mypy determine that qubits does not contain any QubitPlaceholders.
+            qubit_inds = cast(List[int], qubits)


@karalekas what do you think, does this diff seem reasonable? See also my comment here #1134 (comment)

karalekas · 2019-12-30T20:05:26Z

Thanks for trudging through this stuff @rht, it is greatly appreciated!!

rht requested a review from a team as a code owner December 24, 2019 13:59

rht force-pushed the mypy branch 2 times, most recently from 62b6a1a to 2daff9d Compare December 24, 2019 14:02

rht force-pushed the mypy branch from 2daff9d to 7a21e3e Compare December 24, 2019 15:33

rht commented Dec 24, 2019

View reviewed changes

pyquil/quil.py Outdated Show resolved Hide resolved

rht force-pushed the mypy branch from 7a21e3e to 1839eff Compare December 24, 2019 16:10

karalekas suggested changes Dec 24, 2019

View reviewed changes

karalekas added the quality 🎨 Improve code quality. label Dec 24, 2019

rht force-pushed the mypy branch 4 times, most recently from 04a51b1 to 1c37255 Compare December 25, 2019 20:20

rht force-pushed the mypy branch 2 times, most recently from d4742cb to dddb6e8 Compare December 26, 2019 23:39

rht changed the title ~~Add missing type annotations of quil.py~~ Add some missing type annotations of quil.py Dec 27, 2019

rht force-pushed the mypy branch 2 times, most recently from 0872d7f to d145fed Compare December 27, 2019 22:50

appleby suggested changes Dec 27, 2019

View reviewed changes

rht force-pushed the mypy branch 2 times, most recently from 9043c54 to f30f99a Compare December 28, 2019 11:33

Add some missing type annotations of quil.py

2353ac4

rht force-pushed the mypy branch 3 times, most recently from 156b77b to fa69e56 Compare December 30, 2019 16:04

6th commit

b5c552a

rht force-pushed the mypy branch from fa69e56 to b5c552a Compare December 30, 2019 16:08

7th commit

e71a6eb

appleby suggested changes Dec 30, 2019

View reviewed changes

pyquil/quil.py Show resolved Hide resolved

pyquil/quil.py Outdated Show resolved Hide resolved

pyquil/quil.py Outdated Show resolved Hide resolved

8th commit

25b5806

appleby suggested changes Dec 30, 2019

View reviewed changes

pyquil/quil.py Outdated Show resolved Hide resolved

pyquil/quil.py Show resolved Hide resolved

rht force-pushed the mypy branch from ac302ae to 3c0ce40 Compare December 30, 2019 19:15

9th commit

a4b890b

rht force-pushed the mypy branch from 3c0ce40 to a4b890b Compare December 30, 2019 19:29

appleby approved these changes Dec 30, 2019

View reviewed changes

karalekas added this to the v2.16 milestone Dec 30, 2019

karalekas changed the title ~~Add some missing type annotations of quil.py~~ Finish the rest of the type annotations for quil.py Dec 30, 2019

karalekas approved these changes Dec 30, 2019

View reviewed changes

karalekas merged commit c46a476 into rigetti:master Dec 30, 2019

rht deleted the mypy branch December 30, 2019 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish the rest of the type annotations for quil.py #1134

Finish the rest of the type annotations for quil.py #1134

rht commented Dec 24, 2019 •

edited

Loading

rht commented Dec 24, 2019

karalekas left a comment

karalekas Dec 24, 2019

karalekas Dec 24, 2019

rht Dec 25, 2019

rht Dec 25, 2019

rht Dec 25, 2019 •

edited

Loading

appleby commented Dec 24, 2019

karalekas commented Dec 24, 2019

rht commented Dec 25, 2019

rht commented Dec 26, 2019

rht commented Dec 27, 2019

appleby commented Dec 27, 2019

appleby commented Dec 27, 2019

appleby commented Dec 27, 2019

appleby left a comment

rht commented Dec 30, 2019 •

edited

Loading

appleby left a comment

rht commented Dec 30, 2019

appleby left a comment

appleby Dec 30, 2019

rht commented Dec 30, 2019

rht commented Dec 30, 2019

appleby left a comment

rht commented Dec 30, 2019

appleby commented Dec 30, 2019

rht commented Dec 30, 2019

appleby commented Dec 30, 2019

rht commented Dec 30, 2019

appleby commented Dec 30, 2019

appleby left a comment

appleby Dec 30, 2019

karalekas commented Dec 30, 2019

	- Type hints have been added to the `Program` class (@rht, gh-1115, gh-1134)
	- Type hints have been added to the `quil.py` file (@rht, gh-1115, gh-1134).

Finish the rest of the type annotations for quil.py #1134

Finish the rest of the type annotations for quil.py #1134

Conversation

rht commented Dec 24, 2019 • edited Loading

Description

Checklist

rht commented Dec 24, 2019

karalekas left a comment

Choose a reason for hiding this comment

karalekas Dec 24, 2019

Choose a reason for hiding this comment

karalekas Dec 24, 2019

Choose a reason for hiding this comment

rht Dec 25, 2019

Choose a reason for hiding this comment

rht Dec 25, 2019

Choose a reason for hiding this comment

rht Dec 25, 2019 • edited Loading

Choose a reason for hiding this comment

appleby commented Dec 24, 2019

karalekas commented Dec 24, 2019

rht commented Dec 25, 2019

rht commented Dec 26, 2019

rht commented Dec 27, 2019

appleby commented Dec 27, 2019

appleby commented Dec 27, 2019

appleby commented Dec 27, 2019

appleby left a comment

Choose a reason for hiding this comment

rht commented Dec 30, 2019 • edited Loading

appleby left a comment

Choose a reason for hiding this comment

rht commented Dec 30, 2019

appleby left a comment

Choose a reason for hiding this comment

appleby Dec 30, 2019

Choose a reason for hiding this comment

rht commented Dec 30, 2019

rht commented Dec 30, 2019

appleby left a comment

Choose a reason for hiding this comment

rht commented Dec 30, 2019

appleby commented Dec 30, 2019

rht commented Dec 30, 2019

appleby commented Dec 30, 2019

rht commented Dec 30, 2019

appleby commented Dec 30, 2019

appleby left a comment

Choose a reason for hiding this comment

appleby Dec 30, 2019

Choose a reason for hiding this comment

karalekas commented Dec 30, 2019

rht commented Dec 24, 2019 •

edited

Loading

rht Dec 25, 2019 •

edited

Loading

rht commented Dec 30, 2019 •

edited

Loading