Add tests for type annotations with mypy #1042

stefsmeets · 2021-03-12T10:58:38Z

Description

Hi everyone, this PR adds mypy as an additional test in pytest.
It also fixes all the type annotations where possible.

Closes Add mypy checks to pytest #993
Link to documentation: https://esmvaltool--1042.org.readthedocs.build/projects/ESMValCore/en/1042/contributing.html#python

Before you get started

☝ Create an issue to discuss what you are going to do

Checklist

PR has a descriptive title for the changelog
Labels are assigned so they can be used in the changelog
Code follows the style guide
Documentation is available for new functionality
YAML files pass pre-commit or yamllint checks
Circle/CI tests pass
Codacy code quality checks pass
Documentation builds successfully on readthedocs
Unit tests are available

To help with the number pull requests:

🙏 We kindly ask you to review two other open pull requests in this repository

This reverts commit 48ca75b.

stefsmeets · 2021-03-15T14:47:21Z

Hi @nielsdrost @bouweandela , this PR is ready to be reviewed. Could one of you have a look?

bouweandela · 2021-03-15T15:12:43Z

Sure, I'd be happy to review!

Did you try if it works with Python 3.6? Because I would expect the worst support for type annotations there. Up until now, we weren't using any new Python features, but I believe that type annotations are a more recent addition to the language.

bouweandela

Very nice contribution!

Did you also try the mypy plugin for prospector?

esmvalcore/_task.py

esmvalcore/cmor/table.py

bouweandela · 2021-03-15T15:57:52Z

tests/integration/test_data_finder.py

+CFG_DEVELOPER = esmvalcore._config.read_config_developer_file()
+esmvalcore._config._config.CFG = CFG_DEVELOPER
 # Initialize CMOR tables
-read_cmor_tables(esmvalcore._config.CFG)
+read_cmor_tables(CFG_DEVELOPER)


This is unsafe code, because it changes a loaded module (esmvalcore._config._config.CFG) so it can potentially mess up that module. Line 14-17 can be replaced with a safer:

esmvalcore._config.load_config_developer()

I also don't like that code, but that is how it is at the moment in master and should be fixed in a separate PR.

You're changing the code in this pull request, so you might as well improve it while you're at it

I'd love to improve it, but that is not the goal of this PR. I only fixed what was necessary to pass the type checks with mypy👍 If this needs more fixing, then let's do it in a separate issue/PR.

bouweandela · 2021-03-15T16:15:05Z

tests/unit/experimental/test_config.py

@@ -224,7 +225,7 @@ def test_config_class():
    assert isinstance(cfg['output_dir'], Path)
    assert isinstance(cfg['auxiliary_data_dir'], Path)

-    from esmvalcore._config import CFG as CFG_DEV
+    from esmvalcore._config._config import CFG as CFG_DEV


Has the location of this import moved in this pull request?

No, as you mentioned in the comment above, this code is unsafe. This test depended on the config developer to be set as esmvalcore._config.CFG in test_data_finder.py. The location of _config was changed in #939.

But why are the tests passing with the old import, if this was changed 2 months ago?

Because test_data_finder sets the variable:
test_data_finder.py: esmvalcore._config.CFG = esmvalcore._config.read_config_developer_file()

and then test_config imports it:
test_config.py: from esmvalcore._config import CFG as CFG_DEV

When I changed one, I had to change the other. I don't know the reason why it was done this way, but it seemed like I missed this one in #939, so I updated it.

bouweandela · 2021-03-16T09:01:43Z

While trying out this pull request, I noticed that sometimes when I run mypy it automatically adds the entry /.mypy_cache/ to my .gitignore file, which then shows as changed. It would be nice to add that entry in this pull request, so other people aren't bothered with it.

stefsmeets · 2021-03-16T09:54:11Z

Did you try if it works with Python 3.6?

I have not tried Python 3.6. Is that part of our test specification? The tests on CircleCI are only with 3.7 afaik. I'm not sure it makes sense to test it with Python3.6, because we will never see the errors.

stefsmeets · 2021-03-16T09:55:09Z

Did you also try the mypy plugin for prospector?

I have not. I will add it to prospector / pre-commit, that may be nice. The check is very fast anyway 👍

Update: seems to work as intended. 😁

bouweandela · 2021-03-16T13:23:35Z

I have not tried Python 3.6. Is that part of our test specification?

Yes, ESMValTool and ESMValCore support Python 3.6 and up

The tests on CircleCI are only with 3.7 afaik.

They are run with a recent Python version, Python 3.9 at the moment, see e.g. here

I'm not sure it makes sense to test it with Python3.6, because we will never see the errors.

It makes sense to test, because if we say we support Python 3.6 and up, we should be sure it works. There is already a nightly GitHub Action run on the master branch that tests all supported Python versions on all supported platforms, see e.g. here. So if there are any errors, we will start seeing them there as soon as this pull request is merged into master.

Depending on how easy it is to mess up because type hints are less well supported by older Python versions, it would probably make sense to add a test job that runs on every pull request with the lowest Python version we support.

bouweandela · 2021-03-16T13:34:40Z

If it makes life easier, I think we could drop Python 3.6 support because it's almost end of life anyway. It looks like Python 3.7 and up have better support.

stefsmeets · 2021-03-16T13:44:40Z

If it makes life easier, I think we could drop Python 3.6 support because it's almost end of life anyway. It looks like Python 3.7 and up have better support.

Either works for me, but if we are supporting 3.6 then I think it would be best if we run our CI tests in the PR on 3.6 to ensure that people do not add unsupported code.

bouweandela · 2021-03-17T10:26:33Z

It would be best to run them with both Python 3.9 (to see that things work the Python version that most people will use and that we're not using too old features that have been removed) and Python 3.6 (to check that we're not using features that are too new and not available yet). Would it be OK with you if I added an extra CircleCI test for Python 3.6 in this pull request? Or would you like to do this yourself?

stefsmeets · 2021-03-18T10:43:21Z

It would be best to run them with both Python 3.9 (to see that things work the Python version that most people will use and that we're not using too old features that have been removed) and Python 3.6 (to check that we're not using features that are too new and not available yet). Would it be OK with you if I added an extra CircleCI test for Python 3.6 in this pull request? Or would you like to do this yourself?

I think it is more transparent to add a new CircleCI test for python 3.6 in a separate PR. And then merge that here.

EDIT: made an issue here #1055

bouweandela · 2021-03-22T15:36:18Z

@stefsmeets Could you please fix the issues reported by Codacy and the failing CircleCI tests?

stefsmeets · 2021-03-22T16:14:34Z

@stefsmeets Could you please fix the issues reported by Codacy and the failing CircleCI tests?

Done. The other codacy issue was already there, because the function arguments need to match the original one.

CircleCI fails are because of the conda_build related to fiona, and a segmentation fault 🤷‍♂️

bouweandela · 2021-03-23T08:44:50Z

The other codacy issue was already there, because the function arguments need to match the original one.

I think you are referring to the warnings from file esmvalcore/experimental/_warnings.py here, right?

What about

esmvalcore/experimental/config/_validated_config.py
Method '__delitem__' is abstract in class 'MutableMapping' but is not overridden (abstract-method)

class ValidatedConfig(MutableMapping, dict):

and

esmvalcore/experimental/recipe_output.py
No value for argument 'files' in constructor call

return cls(name=task.name, output=product_attributes)

Unexpected keyword argument 'output' in constructor call

return cls(name=task.name, output=product_attributes)

? These look like things that would need to be solved.

CircleCI fails are because of the conda_build related to fiona, and a segmentation fault man_shrugging

The fiona test problem has been fixed #1046, so you can solve it by merging master into this branch. The segmentation fault can be usually be avoided by clicking the re-run button on CircleCI, see #644.

stefsmeets · 2021-03-23T10:11:55Z

Sorry @bouweandela , but I don't see those issues in Codacy. Afaik I already fixed those a long time ago.

stefsmeets · 2021-03-23T10:13:25Z

I already re-ran the tests, and now test passes, but conda_build fails with OSError: [Errno 28] No space left on device: '/opt/conda/conda-bld/linux-64/repodata_from_packages.json.7a93cac9-76ed-4f08-88f8-57fe3851d589' 🤷‍♂️

bouweandela · 2021-03-23T13:49:03Z

I already re-ran the tests, and now test passes, but conda_build fails with OSError: [Errno 28] No space left on device: '/opt/conda/conda-bld/linux-64/repodata_from_packages.json.7a93cac9-76ed-4f08-88f8-57fe3851d589' man_shrugging

The tests pass now, so that should be fine, thanks!

stefsmeets added 17 commits March 12, 2021 11:54

Add mypy to pytest

68cb662

Fix _warnings_formatter arguments

d8624c6

Fix regex type

c498c62

Add/fix type hints in config

d644dc4

Fix MRO

48ca75b

Fix reassignment

b350bdb

Fix type hints in experimental.recipe

0c9a344

Fix mypy not being able to infer type of self._engine

59b1ccb

Fix type annotation for log_to_dir

6edaa21

Add type annotation for CMOR_TABLES

4721151

Fix typing in _logging submodule

7c453d0

Fix type annotations in _run_parallel

0e41b25

Avoid re-using variable name

ecc4451

Revert "Fix MRO"

f9b5130

This reverts commit 48ca75b.

Add type hints

5b67c2c

Fix mypy issues in tests

54f9254

Fix typing issues (MRO) with config object

a26099c

stefsmeets added the testing label Mar 15, 2021

stefsmeets added 5 commits March 15, 2021 14:56

Fix misc typing issues

e65672e

Add mypy to setup.cfg

e6b34d0

Add documentation

bc84012

Fix type hint instead of removing it

107a548

Add esmvalcore to mypy files config

623c667

stefsmeets marked this pull request as ready for review March 15, 2021 14:46

stefsmeets requested a review from jvegreg as a code owner March 15, 2021 14:46

stefsmeets requested a review from nielsdrost March 15, 2021 14:46

stefsmeets requested a review from bouweandela March 15, 2021 14:47

bouweandela requested changes Mar 15, 2021

View reviewed changes

stefsmeets added 4 commits March 16, 2021 10:24

Add .mypy_cache to .gitignore

8f325bb

Address review comments

02dc7b5

Fix forward reference

64a1e47

Fix future type

baa1f63

stefsmeets and others added 2 commits March 16, 2021 09:55

Merge branch 'master' into mypy_test

40902e7

Add mypy to prospector / pre-commit

cb700ac

stefsmeets mentioned this pull request Mar 18, 2021

Should we test every PR with Python 3.6? #1055

Closed

Fix py36 edge cases with mypy

ce46fc6

Fix Codacy issue

6834246

Merge branch 'master' into mypy_test

9ef73bd

bouweandela approved these changes Mar 23, 2021

View reviewed changes

bouweandela merged commit 09ff7c9 into master Mar 23, 2021

bouweandela deleted the mypy_test branch March 23, 2021 13:50

Add tests for type annotations with mypy #1042

Add tests for type annotations with mypy #1042

Uh oh!

Conversation

stefsmeets commented Mar 12, 2021 • edited by bouweandela Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Before you get started

Checklist

Uh oh!

stefsmeets commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bouweandela commented Mar 15, 2021

Uh oh!

bouweandela left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bouweandela Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

stefsmeets Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

bouweandela Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

stefsmeets Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

bouweandela Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

stefsmeets Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bouweandela Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

stefsmeets Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bouweandela commented Mar 16, 2021

Uh oh!

stefsmeets commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefsmeets commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bouweandela commented Mar 16, 2021

Uh oh!

bouweandela commented Mar 16, 2021

Uh oh!

stefsmeets commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bouweandela commented Mar 17, 2021

Uh oh!

stefsmeets commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bouweandela commented Mar 22, 2021

Uh oh!

stefsmeets commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bouweandela commented Mar 23, 2021

Uh oh!

stefsmeets commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefsmeets commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bouweandela commented Mar 23, 2021

stefsmeets commented Mar 12, 2021 •

edited by bouweandela

Loading

stefsmeets commented Mar 15, 2021 •

edited

Loading

stefsmeets Mar 16, 2021 •

edited

Loading

stefsmeets Mar 16, 2021 •

edited

Loading

stefsmeets commented Mar 16, 2021 •

edited

Loading

stefsmeets commented Mar 16, 2021 •

edited

Loading

stefsmeets commented Mar 16, 2021 •

edited

Loading

stefsmeets commented Mar 18, 2021 •

edited

Loading

stefsmeets commented Mar 22, 2021 •

edited

Loading

stefsmeets commented Mar 23, 2021 •

edited

Loading

stefsmeets commented Mar 23, 2021 •

edited

Loading