Ignore mypy's missing import warning for requests

[datumbox]

Fixes #4009

[AnirudhDagar]

LGTM! Thanks for the fix @datumbox

A quick question: Any specific reasons for ignoring the type check instead of installing the stubs?

[pmeier]

I'm with @AnirudhDagar here: installing the stubs would make the mypy check more reliable. mypy is not a necessary evil that needs to be silenced whenever it turns up something. We opted to use it to catch bugs that are not covered by our tests.

[NicolasHug]

We opted to use it to catch bugs that are not covered by our tests.

I'm curious, in the PRs that were making the code mypy compliant, did you catch actual bugs?

[datumbox]

@pmeier @AnirudhDagar The master is currently failing and when this happens it's high priority to fix it. This PR is not about improving the code, it's about restoring the CI in green state. In situations like this, please avoid blocking such PRs unless you see a bug being introduced and opt for making any suggested improvements on subsequent PRs.

[pmeier]

We opted to use it to catch bugs that are not covered by our tests.

I'm curious, in the PRs that were making the code mypy compliant, did you catch actual bugs?

It has been a while, but I'm fairly certain that the answer was no. IMO that is a testament to good reviews.

While developing, I often find myself in a position where mypy complains about something and it turns out it is something I didn't think through before. Plus, being mypy compliant makes thorough testing a whole lot easier.

[fmassa]

Let's get this merged to get CI green. We can always send a follow-up PR adding it to the stubs.

[NicolasHug]

Stamping, but even though I hate mypy, I agree with @pmeier on this one: it wouldn't hurt much to spend a bit more time to fix the root of the issue.

[pmeier]

@datumbox The fix for this is exactly the same number of lines: add mypy --install-types after

vision/.circleci/config.yml.in

Line 207 in ea6be12

pip install --user --progress-bar off mypy

[AnirudhDagar]

@datumbox did not mean to question or block you. Sorry for that. It is definitely important to get a master to green in such a case.

[NicolasHug]

Thanks @pmeier for the feedback

It has been a while, but I'm fairly certain that the answer was no. IMO that is a testament to good reviews.

And to good tests I think

While developing, I often find myself in a position where mypy complains about something and it turns out it is something I didn't think through before.

Interesting. My own experience was that whatever mypy was flagging was forcing me to re-write the code into something that either doesn't make sense, or is less readable ^^

Plus, being mypy compliant makes thorough testing a whole lot easier.

I disagree with the test part from that talk because it's talking about input validation checks: we can't rely on types for that, because this would assume that our users also use mypy. For libraries such as torchvision, type annotation brings nothing to the table when it comes to input validation of user-facing utilities.

[datumbox]

@pmeier I understand that the fix you have in mind might be trivial in this specific case but changes like this still need time to be tested. IMO restoring the CI in green state when it's a false positive is high priority and buys us time to do any further due diligence on improving the code and resolving the root cause. By restoring the CI, it will unblock contributors such as @AnirudhDagar who currently get the wrong signal.

Happy to review a PR that addresses the issue and improve the codebase.