Enable mypy and isort in pre-commit & CI

[ml-evs]

This PR fixes many broken/dodgy type hints in the code, and a few associated bugs/undefined behaviours.

• Enable mypy in CI
• Enable and apply isort
• Remove simplejson (not really needed since we dropped 3.7 support)
• Update pre-commit
• Add linters and fixers to dev requirements

[codecov]

Codecov Report

Merging #1346 (01504c9) into master (62b83fb) will decrease coverage by 0.08%.
The diff coverage is 95.44%.

❗ Current head 01504c9 differs from pull request most recent head 91acd47. Consider uploading reports for the commit 91acd47 to get more accurate results

@@            Coverage Diff             @@
##           master    #1346      +/-   ##
==========================================
- Coverage   91.42%   91.33%   -0.09%     
==========================================
  Files          72       72              
  Lines        4372     4363       -9     
==========================================
- Hits         3997     3985      -12     
- Misses        375      378       +3     

Flag	Coverage Δ
project	91.33% <95.44%> (-0.09%)	⬇️
validator	91.43% <98.02%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/adapters/references/__init__.py	100.00% <ø> (ø)
optimade/adapters/structures/__init__.py	100.00% <ø> (ø)
optimade/server/mappers/__init__.py	100.00% <ø> (ø)
optimade/server/routers/references.py	100.00% <ø> (ø)
optimade/utils.py	58.20% <44.44%> (-3.56%)	⬇️
optimade/adapters/structures/utils.py	83.84% <64.28%> (ø)
optimade/filtertransformers/elasticsearch.py	85.94% <83.33%> (ø)
optimade/client/client.py	79.01% <87.50%> (-0.26%)	⬇️
optimade/server/middleware.py	94.70% <95.23%> (-0.17%)	⬇️
optimade/validator/validator.py	83.45% <95.23%> (-0.09%)	⬇️
... and 53 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[JPBergsma]

In principle, this is a good idea.
I however think that we should solve all the warnings mypy gives (or silence them with # type: ignore if they are false alarms) before we merge this.
Now it gives hundreds of warnings, I won't notice if my PR will add one extra warning, so I do not think it would be very useful at this point.

[ml-evs]

Sorry for the kinda monstrous PR @JPBergsma, I think I will split it into two PRs with isort and then mypy. I didn't have to change any tests from my mypy changes so hopefully it should all just work...

[JPBergsma]

For me, it is not necessary to split in two. I think both changes are quite orthogonal, so I can review both of them at the same time.

[ml-evs]

For me, it is not necessary to split in two. I think both changes are quite orthogonal, so I can review both of them at the same time.

Okay great, I guess my suggestion is to ignore the changes from 4782f5e and just check mypy first. The isort changes do not really need reviewing.

Once you are happy with it I will rebase this PR into a few commits so that we have the separation going forward.

[ml-evs]

(We can also merge your other PRs before this one!)

[JPBergsma]

I am still having some problems with PR#1304.

The docker image setup seems to fail because of yaml missing.
which is strange as this PR did not change anything about the dependancies.

And the tests still fail for the elastic search backend.
So now I am trying to set up elastic locally to see why the tests are failing.

I tried to set up the docker container according to the installation instructions, but then I get an error that optimade can't connect to elastic search: "ERROR tests/server/middleware/test_api_hint.py - elasticsearch.exceptions.ConnectionError: ConnectionError(<urllib3.connection.HTTPConnection object at 0x7f4e5cdb62b0>: Failed to establish a new connection: [Errno 111] Connection ..."

So I guess I still have some more work to do before my pr is merge ready.

ps. after executing docker network create elastic the tests do start, but I still receive many errors.

[ml-evs]

I am still having some problems with PR#1304.

The docker image setup seems to fail because of yaml missing. which is strange as this PR did not change anything about the dependancies.

And the tests still fail for the elastic search backend. So now I am trying to set up elastic locally to see why the tests are failing.

I tried to set up the docker container according to the installation instructions, but then I get an error that optimade can't connect to elastic search: "ERROR tests/server/middleware/test_api_hint.py - elasticsearch.exceptions.ConnectionError: ConnectionError(<urllib3.connection.HTTPConnection object at 0x7f4e5cdb62b0>: Failed to establish a new connection: [Errno 111] Connection ..."

So I guess I still have some more work to do before my pr is merge ready.

ps. after executing docker network create elastic the tests do start, but I still receive many errors.

I also ran into this (#1377), not sure if the docker oneliner was tested before we upgrade to ES v7. Will try to address this in #1373

[ml-evs]

Just had to do a bit of a messy rebase after we merged that last PR, hope that doesn't mess up your review, but at least now the commits should each contain logical diffs

[JPBergsma]

When I run mypy on my machine, I still get two errors.

1. optimade/adapters/structures/utils.py:223: error: Need type annotation for "Y" [var-annotated]
   Replacing line 223 with Y: np.ndarray = np.cross(Z, X) should solve this warning.

2. optimade/server/exception_handlers.py:231: error: List item 3 has incompatible type "Tuple[Type[VisitError], Callable[[Any, VisitError], JSONAPIResponse]]"; expected "Tuple[Type[Exception], Callable[[Any, Exception], JSONAPIResponse]]" [list-item]

   I think black caused this line to be broken up so you need a second: # type: ignore[list-item] # not entirely sure why this entry triggers mypy

[ml-evs]

Thanks for the really thorough review @JPBergsma -- I think for the sake of our time, we can consider this PR ready... unless there's any objections I'll merge later today.

[JPBergsma]

If you want, you can merge it. I still have not finished reviewing all the files yet, so I may still suggest some changes in a later PR.

[ml-evs]

If you want, you can merge it. I still have not finished reviewing all the files yet, so I may still suggest some changes in a later PR.

Okay, happy to hold off if you want to keep going.

[ml-evs]

I've just tried adding the --warn_unused_ignores config option to mypy and it doesn't even seem self-consistent. When I remove the ignores in the base adapters (after it says they are not needed), the same invocation then fails. Is this the same way you have been finding which ignores are not needed?

[JPBergsma]

I mostly try to understand why the ignore statement is needed.
If it is not clear, I remove it to see what kind of error mypy gives.
Sometimes I do not get an error, and in that case I make a remark here that the ignore statement is not needed.

[JPBergsma]

I have finished the review. So there should not be any new comments.

[ml-evs]

Thanks @JPBergsma. I've addressed your final comments and will merge once the tests pass.

[JPBergsma]

I am not so happy with this line. The value of failure_type will be overwritten later on, so this assignment looks rather strange.
Is it such a problem to define the type on what is now line 377?

[ml-evs]

Not a problem, but not of any importance, I don't think. I'd rather have the default value outside of the clause rather than a type-hint inside the clause. Not everything has to be explicitly typed (and we shouldn't let mypy dictate how we code beyond any real errors, especially as it changes between versions).

[JPBergsma]

LGTM