Add type hints, and use mypy to enforce in CI

[karalekas]

Python is a dynamically-typed language (see: duck typing), but Python 3.5 and pep-484 introduced type hints as a way to annotate Python code with types. These are not enforced by the language at runtime, but they can be checked by third-party libraries such as mypy. The impetus for adding type hints and using mypy is that they generally encourage developers to write saner, simpler code (the flexibility of Python is a double-edged sword). This issue is mostly about good code style and therefore is optional -- it doesn't block CI setup (but once it is complete, the CI can be used to do this type checking automatically).

[TripleRD]

Hi @HGSilveri @karalekas, I went through the issue and would like to work on it. I have a few questions, I found this GitHub Action which can be used to implement the type hint ci, so can it be used? or do it need to be implemented using shell script?

[LaurentAjdnik]

Hi @TripleR47, I've done it for the Register class (I will PR #132 tomorrow) but I did it manually 🤨, while adding "Enhanced ErrorValue messages" (#148) at the same time.

[HGSilveri]

Hi @TripleR47, no preference regarding how the mypy checks are occur, as long as they are programmed into the existing workflow, if that's what you're asking. But bear in mind that is not the crux of the PR, but rather going through the existing code and adding the appropriate type hints to each function and method. After that's done, then you can incorporate mypy into the CI routine.
I realize it's quite a bit of work, but that's why there's a bounty on it!

[TripleRD]

Okay, I'll ask in the unitaryHack's server if anyone wants to collaborate on this issue. Can it be assigned to me?

[TripleRD]

Hi @LaurentAjdnik, wanna collaborate on this issue?

[LaurentAjdnik]

Hi @LaurentAjdnik, wanna collaborate on this issue?

👍

[TripleRD]

Hi @LaurentAjdnik, wanna collaborate on this issue?

👍

Are you in unitaryfund's discord server? If you are then DM me on discord, we can discuss further on this.

[HGSilveri]

@TripleR47 @LaurentAjdnik So, can I assign it to you both?

[TripleRD]

Sure

[LaurentAjdnik]

@TripleR47 @LaurentAjdnik So, can I assign it to you both?

Yup!

Your feedback on #155, in which I added type hints, will be insightful for the current issue.

Then, we can sync with @TripleR47.

[TripleRD]

Hi @LaurentAjdnik, wanna collaborate on this issue?

👍

Are you in unitaryfund's discord server? If you are then DM me on discord, we can discuss further on this.

@LaurentAjdnik

[LaurentAjdnik]

@TripleR47: Nope, not on Discord. I'm waiting for the feedback on #155 and we can move on from there.

Adding type hints will be pretty straightforward. Maybe we can dispatch classes between us.

@HGSilveri: Will you be in charge of adding mypy to CI workflow ?

[HGSilveri]

@LaurentAjdnik : I'll review #155 first thing on Monday. And yes, I can add it to the CI no problem. I would advise you to use it to test your own typing as you go. Did you try it already on #155?

[LaurentAjdnik]

Running MyPy on #155 (screenshots below):

• Register class: No errors in my code (or in the signatures I updated)
• Register class: Error in from_coordinates() on qubits = dict(enumerate(coords))
• A few errors in other modules
• Lots of errors on imports with modules having no type hints

With the --ignore-missing-imports option:

Without the --ignore-missing-imports option:

[LaurentAjdnik]

Hi @TripleR47!

Have you been working on it ?

PR #155 has been approved and type-checked with MyPy beforehand. We could use it as an inspiration.

[TripleRD]

Yes, I have been going through mypy and Pulser documentations and have started working on the devices module.

@LaurentAjdnik, let's split up the rest of the modules.

[LaurentAjdnik]

Yes, I have been going through mypy and Pulser documentations and have started working on the devices module.

@LaurentAjdnik, let's split up the rest of the modules.

Great! I'll start working on waveforms.py then.

[LaurentAjdnik]

Hi again @TripleR47!

I just PR-ed #163 on waveforms.py for a feedback from experts, that we can use for further work.

[LaurentAjdnik]

Hi again @TripleR47 and @HGSilveri, I start working on sequence.py.

[TripleRD]

Okay, I'm almost done with the devices module, will start working on parametrized and simulation after that.

[LaurentAjdnik]

Great! I'll start working on _seq_drawer.py, channels.py and pulse.py so that the root directory is done.

[TripleRD]

Okay! I'm done with the parametrized module, start working on simulation next.

[HGSilveri]

Hi @LaurentAjdnik , @TripleR47 ! Glad to see you're making progress.

I just wanted to clarify a few things:

• The order of the PRs should be in order of increased dependency on other modules. This means, for example, that pulse.py should be merged after waveforms.py but before sequence.py. Also, pulser.parametrized should come soon as it is frequently imported but does not import any other module. Inversely, pulser.simulation should be one of the last modules to be merged, as it is not imported but imports Sequence, which in turn imports almost everything else. Thus, the next modules to merge should be:
  1. pulser.channels and pulser.parametrized
  2. pulser.waveforms
  3. pulser.pulse
• You'll find in Setting up mypy #168 that I setup a config file for mypy. Once that PR is merged with develop, you can start type checking locally just by running mypy inside the Pulser folder, but make sure you have numpy >= 1.20 installed beforehand.
• You'll also find that I added some type checks in Setting up mypy #168, mostly to get rid of all the mypy errors. Beware of potential merge conflicts.

Keep up the good work!

[LaurentAjdnik]

• The order of the PRs should be in order of increased dependency on other modules.

Sorry, I started PR-ing without respecting this order...

And the branch I'm working on will include all remaining files in the root directory...

I feel like type-hinting everything will soon be over. Hence, if the corresponding PRs pass the existing checks (without mypy enabled), can we merge them into develop, use mypy locally on a copy of develop, adjust everything that's needed and finally enable mypy in CI ?

[HGSilveri]

I'd rather not merge to develop without mypy in the CI... What might happen is that you find, as the modules are incrementally merged into develop, that conflicts in modules that were already type-hinted will arise. This means that these PRs will need to include modifications to files that should already be dealt with. It will make things a bit messier, but I guess it's too late now to go back. Just try to stick to the order laid out above as much as possible. I will also try to order the merging of the PRs accordingly.

[TripleRD]

Okay @HGSilveri, @LaurentAjdnik if you haven't started working on pulse.py yet, can I take over that module? After that, I can go for pulser.simulation.

[LaurentAjdnik]

if you haven't started working on pulse.py

Well, I've finished working on it. All files in root are done. I might have to split this unique new branch so that @HGSilveri can arrange the merging order.

[TripleRD]

Okay, I have also finished the pulser.simlation. Let's start working on the test module then.

[TripleRD]

Hey @LaurentAjdnik, which test modules are you working on?

[LaurentAjdnik]

Hey @LaurentAjdnik, which test modules are you working on?

Hi @TripleR47, I'm done with all files in the root directory.

I haven't planned working on test files.

@HGSilveri: Does it make sense anyway?

[HGSilveri]

@HGSilveri: Does it make sense anyway?

I would say no, not really.

[TripleRD]

Okay

[HGSilveri]

So, I was convinced that using "parametrized generics" for type-hinting using built-ins (e.g. dict[str, Any] instead of Dict[str, Any]) would only work from Python 3.9, but apparently, according to PEP 585 that's not the case if annotations are imported from __future__, which can be done since Python version 3.7.

I don't like the inconsistency of having both forms, so I guess we should adopt the most modern one throughout, which is to use the built-ins (i.e., dict, list, tuple, ...).

I have been asking you to do the opposite up until now, so it's only fair I make a PR enforcing these changes for all the type-hints that have already been merged. However, it would be good if you could change this on the PRs that are still open.

[TripleRD]

Okay

[HGSilveri]

@TripleR47, bear in mind that you still can only do this in the type annotations. This means, for example, that you can't do cast(list[int], x), you'll have to use List. But you can type a return or a variable as list[int]. Hope that makes sense

[TripleRD]

Okay, and what about the special types like Callable, Union etc. Do we have their alternatives in the built-ins?

[HGSilveri]

No, those stay the same. Refer to this list for the built-in generics that replace those in typing.

[TripleRD]

Okay, thanks!

[HGSilveri]

@TripleR47 : From that list, notice that Callable actually does change (should be imported from collections.abc instead).

[HGSilveri]

Solved by @LaurentAjdnik and @TripleR47 , thank you both for your patience and hard work!

[TripleRD]

@HGSilveri, thanks for your guidance! 😃