Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.24k stars 2.36k forks source link

Make type hinting mypy-clean with relaxed settings #6905

Open levbishop opened 3 years ago

levbishop commented 3 years ago

It's not clear that we want to enforce strict type hinting for qiskit and/or include mypy in our CI, but I do think that for the places where type hinting exists, there those hints should aim to be correct. For many modules they mostly already are, but in others there are a lot of errors. I'd even be fine with removing the incorrect type hints if its too much trouble to correct them.

The current situation makes it harder for contributors who care to use mypy since they need to ignore all the errors that don't relate to their PR.

jakelishman commented 2 years ago

At the moment, mypy crashes when running on Terra. I think I managed to isolate the problem down to a simple reproducer, though I've absolutely no idea what's causing the failure, because it seems to need quite a few moving parts. Tracked in python/mypy#11686.

kdk commented 2 years ago

At the moment, mypy crashes when running on Terra

It looks like this may depend on python version. I still see this crash with python 3.7.12 and mypy 0.950, but I know @levbishop was able to get results with the same mypy python 3.9.12.

jakelishman commented 2 years ago

I think it's because Lev used some super relaxed options, that result in the mypy bug I linked above not getting triggered. Probably --no-strict-optionals or --allow-redefinitions was enough to get through that bug; as best I remember from before I stopped procrastinating on my thesis, it's only triggered when mypy's in a "complete typing" mode, which is why some of the unrelated type annotations are necessary in my minimal reproducer.

With no options, I can still reproduce the error on both Python 3.7 and 3.9 and mypy 0.950.

kdk commented 2 years ago

Interesting, but the relaxed options I think provide a good place to start. Transcribing @levbishop 's results

Found 1416 errors in 227 files (checked 1002 source files)

FWIW, for me mypy crashed out on 3.7 even with the same relaxed options @levbishop was using

mypy --ignore-missing-imports --no-strict-optional  --allow-untyped-globals --allow-redefinition -p qiskit
prakharb10 commented 2 years ago

Hi, I think work on this is blocked by https://github.com/python/mypy/pull/12943?

jakelishman commented 2 years ago

It's not necessarily blocked, but it'll definitely be harder to do while mypy crashes on Terra. The code mypy currently crashes on is something that could be worked around with more complete typing - if all partial types are resolved at the end of BlueprintCircuit.__init__ and QuantumCircuit.__init__ (which can be done with type hinting), I think mypy will probably make it through.

Randl commented 2 years ago

Relatively frequent errors that I'm unsure how to fix

  1. Incompatible np.floating and float. Possible fix is to use typing.SupportsFloat
  2. ParameterExpression type appearing in operations on numbers
  3. Union of List for different types (e.g. qubit and clbit). Ideally should type check, but probably won't.
  4. Signatures of functions incompatible with superclass signatures.
jakelishman commented 2 years ago

Some potential answers:

  1. I don't know where exactly you're thinking, but typing.SupportsFloat may well be fine. I suspect the type checker will be a little too accepting with that annotation (e.g. fractions.Fraction would match that, but I expect a decent amount of our floating-point code would blow up if passed one), but that's not a big deal.
  2. Cases where ParameterExpression is also valid should be typed as such. There's an alias ParameterValueType in qiskit.circuit.quantumcircuit that defines this.
  3. Depending on where it's used, the two operands can be typed as List[Bit] instead, which will type check. Don't do that if the same variables are used elsewhere in type-specific contexts, though.
  4. I'm assuming you're talking about cases where there's a typing mismatch rather than a parameter name / meaning mismatch, because pylint should have caught those. It's hard to say without examples, but some of these might be programmer error and some of them might be better changing the typing of the base class.

Part of the reason for doing this is to find cases where there are programming bugs caused by type mis-use, so there's bound to be some places in the code that are incorrect. We need to fix those as well (but not as part of a typing PR - that would be near-impossible to review).

Randl commented 2 years ago
  1. numpy returns type incompatible with float as scalar, e.g.
    qiskit/opflow/gradients/natural_gradient.py:267: error: No overload variant of "max" matches argument types "floating[Any]", "float"
  2. For example
    qiskit/transpiler/passes/scheduling/alignments/pulse_gate_validation.py:82: note: Left operand is of type "Union[int, ParameterExpression]"
    qiskit/transpiler/passes/scheduling/alignments/pulse_gate_validation.py:90: error: Unsupported operand types for > ("int" and "ParameterExpression")
    qiskit/transpiler/passes/scheduling/alignments/pulse_gate_validation.py:90: note: Left operand is of type "Union[int, ParameterExpression]"
  3. ok, I'll take a look
  4. I noted that some are already ignored by pylint, I'll see how much aren't