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

Make type hinting mypy-clean with relaxed settings #6905

Open
levbishop opened this issue Aug 13, 2021 · 9 comments
Open

Make type hinting mypy-clean with relaxed settings #6905

levbishop opened this issue Aug 13, 2021 · 9 comments
Labels
short project type: enhancement It's working, but needs polishing type: qa Issues and PRs that relate to testing and code quality

Comments

@levbishop
Copy link
Member

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.

@levbishop levbishop added type: enhancement It's working, but needs polishing type: qa Issues and PRs that relate to testing and code quality labels Aug 13, 2021
@levbishop levbishop assigned levbishop and unassigned levbishop Dec 4, 2021
@jakelishman
Copy link
Member

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
Copy link
Member

kdk commented May 17, 2022

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
Copy link
Member

jakelishman commented May 17, 2022

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.

@1ucian0 1ucian0 added unitaryhack-bounty Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/ short project labels May 18, 2022
@kdk
Copy link
Member

kdk commented May 18, 2022

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
Copy link
Contributor

Hi, I think work on this is blocked by python/mypy#12943?

@jakelishman
Copy link
Member

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
Copy link
Contributor

Randl commented Jun 16, 2022

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
Copy link
Member

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
Copy link
Contributor

Randl commented Jun 17, 2022

  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"
  1. 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]"
  1. ok, I'll take a look
  2. I noted that some are already ignored by pylint, I'll see how much aren't

@1ucian0 1ucian0 removed the unitaryhack-bounty Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/ label Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
short project type: enhancement It's working, but needs polishing type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

No branches or pull requests

6 participants