Enable `mypy` and `isort` in pre-commit & CI #1346

ml-evs · 2022-10-02T10:55:15Z

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 · 2022-10-02T11:04:26Z

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 · 2022-10-03T10:45:31Z

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 · 2022-11-11T23:32:47Z

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 · 2022-11-14T09:03:58Z

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 · 2022-11-14T10:06:39Z

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 · 2022-11-14T12:30:09Z

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

JPBergsma · 2022-11-14T14:10:18Z

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 · 2022-11-14T17:33:57Z

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

optimade/adapters/structures/aiida.py

requirements-dev.txt

tests/server/test_server_validation.py

ml-evs · 2022-11-18T11:03:30Z

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 · 2022-11-18T12:23:41Z

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

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.
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

optimade/adapters/structures/utils.py

optimade/client/client.py

optimade/filterparser/lark_parser.py

- Fix all implicit optional type hints (PEP484) - Replaces all `a: int = None` with `a: Optional[int] = None` - Automated with `pipx run no_implicit_optional <path>` - Add many mypy ignores - Fix importlib shenanigans

optimade/filterparser/lark_parser.py

optimade/server/middleware.py

ml-evs · 2022-11-26T18:35:22Z

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 · 2022-11-26T19:31:47Z

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 · 2022-11-26T20:40:22Z

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.

optimade/server/routers/utils.py

ml-evs · 2022-11-27T11:42:26Z

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 · 2022-11-27T19:52:08Z

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.

optimade/validator/utils.py

tests/server/utils.py

JPBergsma · 2022-11-28T13:08:06Z

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

Co-authored-by: Johan Bergsma <JPBergsma@users.noreply.github.com>

ml-evs · 2022-11-28T15:58:32Z

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

JPBergsma · 2022-11-28T16:22:23Z

optimade/validator/utils.py

                message = msg.split("\n")
                if validator.verbosity > 1:
                    # ValidationErrors from pydantic already include very detailed errors
                    # that get duplicated in the traceback
                    if not isinstance(result, ValidationError):
                        message += traceback.split("\n")

-                message = "\n".join(message)
-
+                failure_type = None


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?

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

ml-evs force-pushed the ml-evs/enable_mypy branch from b11f8c8 to 0992859 Compare October 5, 2022 10:55

ml-evs force-pushed the ml-evs/enable_mypy branch from 0992859 to 5ba3946 Compare October 25, 2022 14:35

ml-evs changed the title ~~Enable MyPy in pre-commit & CI~~ Enable MyPy and isort in pre-commit & CI Oct 25, 2022

ml-evs force-pushed the ml-evs/enable_mypy branch from 5ba3946 to 818d336 Compare October 25, 2022 16:35

ml-evs force-pushed the ml-evs/enable_mypy branch 3 times, most recently from cbd91b0 to 49851eb Compare November 11, 2022 19:37

ml-evs marked this pull request as ready for review November 11, 2022 21:11

ml-evs requested review from CasperWA, JPBergsma and markus1978 as code owners November 11, 2022 21:11

JPBergsma reviewed Nov 17, 2022

View reviewed changes

optimade/adapters/structures/aiida.py Show resolved Hide resolved