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

[unitaryhack] Start working on mypy errors #8187

Closed
wants to merge 38 commits into from
Closed

Conversation

Randl
Copy link
Contributor

@Randl Randl commented Jun 16, 2022

Summary

I've started fixing some of the mypy errors (#6905). There are some that require deeper dive into the code, and some that are just time-consuming.

@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Randl Randl changed the title Start working on mypy errors [unitaryhack] Start working on mypy errors Jun 16, 2022
@Randl Randl requested a review from jyu00 as a code owner June 16, 2022 20:27
@Randl
Copy link
Contributor Author

Randl commented Jun 17, 2022

Currently I got
Found 1277 errors in 218 files (checked 1003 source files)

@1ucian0
Copy link
Member

1ucian0 commented Jun 17, 2022

That's 139 errors less! Thanks @Randl !

@coveralls
Copy link

coveralls commented Jun 17, 2022

Pull Request Test Coverage Report for Build 2542669289

  • 435 of 491 (88.59%) changed or added relevant lines in 139 files are covered.
  • 41 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 84.289%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/algorithms/optimizers/gradient_descent.py 2 3 66.67%
qiskit/pulse/channels.py 3 4 75.0%
qiskit/pulse/library/pulse.py 3 4 75.0%
qiskit/transpiler/preset_passmanagers/level0.py 2 3 66.67%
qiskit/transpiler/preset_passmanagers/level2.py 2 3 66.67%
qiskit/transpiler/preset_passmanagers/level3.py 5 6 83.33%
qiskit/utils/measurement_error_mitigation.py 1 2 50.0%
qiskit/utils/mitigation/_filters.py 7 8 87.5%
qiskit/utils/mitigation/fitters.py 5 6 83.33%
qiskit/visualization/pulse/interpolation.py 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/algorithms/amplitude_estimators/fae.py 1 97.39%
qiskit/pulse/library/parametric_pulses.py 1 43.03%
qiskit/compiler/transpiler.py 39 89.95%
Totals Coverage Status
Change from base Build 2539968332: -0.02%
Covered Lines: 55007
Relevant Lines: 65260

💛 - Coveralls

@Randl
Copy link
Contributor Author

Randl commented Jun 18, 2022

Found 1071 errors in 200 files (checked 1319 source files)

Note that while this PR appears very large, it doesn't actually makes changes to code itself (except for a couple of very minor changes), only the type annotations.

@Randl
Copy link
Contributor Author

Randl commented Jun 18, 2022

I've got it down below 1000:
Found 987 errors in 188 files (checked 1374 source files)
I think this is a good point to pause, both to not make this PR way too large (hope it's not yet) and to think of how to fix part of the errors. I think I've dealt with most of low-hanging fruits (e.g., annotation of containers/optionals), but out of thousand errors, some should've been missed.

@HuangJunye HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 21, 2022
@Randl
Copy link
Contributor Author

Randl commented Jun 23, 2022

Is something required from me at that point (except from keeping the PR up-to-date)?

@ikkoham
Copy link
Contributor

ikkoham commented Jun 23, 2022

Thank you for your contribution. I have a quick question. Which options are you using?
My local option for Terra is

[mypy]
warn_unused_configs = True
ignore_missing_imports = True
strict_optional = False
no_implicit_optional = True
warn_redundant_casts = True
warn_unused_ignores = True

@Randl
Copy link
Contributor Author

Randl commented Jun 24, 2022

I also would like to add that I strongly believe that whatever is not forced by CI doesn't hold, so typing module-by-module thoroughly, slowly and carefully, not only fixing things that do not typecheck, but annotating everything (which I felt was a spirit of your message) is far harder because
a) we don't enforce any type safety as for now and we more or less can't because existing code doesn't typecheck. We may find ourselves in infinite loop of fixing newly introduced errors (e.g., one of the rebases I did introduced ~50 new type errors, not sure exactly how)
b) it's just much more job.

I think the most feasible way to proceed is to get mypy typecheck with minimal possible effort (yet large, but probably doable in reasonable time) and force at least the relaxed version of mypy checks to CI. That would "checkpoint" type safety efforts and everyone will be able to benefit from type annotation. Then it can be further improved at any pace, and possible some relaxation flags can be cancelled after better annotation is available.
If we don't want to add mypy to CI, someone would need to fix newly introduced errors manually instead, which sounds suboptimal.

The fear that suboptimal annotation wouldn't be further improved is reasonable but feels like a lesser evil then being stuck trying doing everything perfectly from a single shot on a relatively fast-changing code

@Randl
Copy link
Contributor Author

Randl commented Jun 24, 2022

As for splitting, do you think splitting by top-level subfolders of qiskit is good enough granularity? That would mean splitting in 17 PRs 🫠

@Randl
Copy link
Contributor Author

Randl commented Jun 26, 2022

Here is approximate breakdown of errors by qiskit module (acquired by just grepping the mypy output) after this PR:

qiskit/algorithms: 244
qiskit/assembler: 10
qiskit/circuit: 130
qiskit/compiler: 21
qiskit/dagcircuit: 0
qiskit/extensions: 0
qiskit/opflow: 214
qiskit/primitives: 23
qiskit/providers: 46
qiskit/pulse: 139
qiskit/qpy: 0
qiskit/quantum_info: 8
qiskit/results: 0
qiskit/scheduler: 2
qiskit/transpiler: 89
qiskit/utils: 39
qiskit/visualization: 238

Comment on lines -394 to +392
self._y_measurements = None
self._max_probability = None
self._num_evaluation_qubits: Optional[int] = None
self._mle: Optional[float] = None
self._mle_processed: Optional[float] = None
self._samples: Optional[Dict[float, float]] = None
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a common change here. Both algorithms and opflow used to run against mypy in Aqua before they were migrated to Terra here. Since then mypy has not been run here and some code has been added so I would have expected some issues. But mypy always internally figured the type of instance variables based on its usage so unless things were more complicated around the types, where it locked in one aspect and it was used with a different type later, in general this was not needed. @manoelmarques since you dealt more directly with mypy in this area any comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with error messages, somehow mypy couldn't figure out that it's Optional[int] rather than NoneType, and thus assignment of int to this variable is leading to type error

Copy link
Member

Choose a reason for hiding this comment

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

What version of mypy are you running with? I know mypy has put out new versions since we ran the code when it was in Aqua. Here for instance is a mypy ini file, for code that was migrated from Aqua, where in the applications repos we run mypy, and is updated from what we used to do in Aqua. For instance we have strict_optional set False, which allows None to be any type. What settings are you running this with. I assume you are doing this all locally since I do not see requirements-dev.txt nor CI updated to run this here yet. Maybe its too soon.

Copy link
Contributor Author

@Randl Randl Jun 26, 2022

Choose a reason for hiding this comment

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

I'm running mypy 0.970 with following command dmypy run -- --ignore-missing-imports --no-strict-optional --allow-untyped-globals --allow-redefinition --show-error-codes qiskit taken from #6905 (comment)
I'm indeed doing it locally, since there are still ~1000 mypy errors after this PR (and ~1400 before). Fixing everything in one PR is too much, and even this PR will probably be broken into a couple of smaller ones, as discussed a bit earlier here.

@1ucian0
Copy link
Member

1ucian0 commented Jun 28, 2022

As for splitting, do you think splitting by top-level subfolders of qiskit is good enough granularity? That would mean splitting in 17 PRs 🫠

Splitting by module makes sense to me, as they probably require a more targeted review.

@jakelishman
Copy link
Member

Some little comments:

I strongly believe that whatever is not forced by CI doesn't hold

I totally agree with this, it's just that we're not currently prepared to put mypy in CI - we may at some point in the future, but it's not an immediate goal, even if we temporarily get to zero mypy errors. There's two conflicting desires: enforced type safety is one way to catch some classes of programming bugs, but too onerous contributing requirements strongly discourage external contributions. Terra is already complicated to develop for, and a lot of our contributors are scientists first and programmers second. There are also a few modules which use quite a bit of duck typing, which is (naturally) quite faffy to get right with mypy, and bugs can often be caught by well-written unit tests instead.

Right now there are so many errors that reducing them down will make mypy usable enough that we can do the usefulness evaluation, and then we can make another PR squashing the few more bugs that have popped up since, if we decide to go for it. There doesn't need to be zero errors for it to be useful.

first make mypy usable, then improve it [about over/under-specific types]

This is true, and some of it is a bit of a judgement call. In practice, my experience is that the type hints are very unlikely to get changed once they're actually in, and people will look at them and think "this choice is by design", which prevents progress. This is why I'm in favour of targetting small modules one-at-a-time, doing the fixes right rather than just good enough, and then moving on.

[on the subject of type: ignore]

The places I saw were in opflow, and while I may well be missing some context, those pylint suppressions look wrong to me, and if I had been the reviewer, I most likely wouldn't have accepted the code with them in place. They're an example of the paragraph above: they've been in a long time now, but they've never been fixed, because the errors are suppressed. Since us getting mypy working is new and we're not imminently about to gate CI on it, I would rather we let it continue to complain about opflow.


Thanks for opening all the separate PRs: it should be much easier to review. Unfortunately, I'm going to be away from work quite a lot in the next month or two, so I won't be able to do much review myself, but there are others on the maintainer and devrel teams who may.

@jakelishman
Copy link
Member

Since the new PRs are all opened now, I'll close this one to avoid confusion for other reviewers.

@Randl
Copy link
Contributor Author

Randl commented Jun 28, 2022

Re: CI and zero error, I think we are more or less on the same page. I don't think adding relaxed mypy would add much burden to contributors, but we are far from there anyway.

re:ignores, I'm adding a short justification of choices now to PRs, including ignores, which would make the discussion easier. There are some errors which definitely suggest design changes, but that's up to code owners. That's a good chance to consider those changes.

I've started from smaller modules/modules with fewer errors, and I'm trying to classify the remaining errors so their fixes can be discussed in the same pr. I'll try to add the remaining modules PRs tomorrow

@Randl
Copy link
Contributor Author

Randl commented Aug 1, 2022

Hmm, who can I ping for a review of the PRs? Some are really small and can be easily merged, others require some discussion

@Randl
Copy link
Contributor Author

Randl commented Sep 12, 2022

Ping @jakelishman @1ucian0

@1ucian0 1ucian0 added the unitaryhack Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/ label May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo unitaryhack Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants