Fix local mypy workflow

[KapJI]

Proposed change

My workflow

script/setup
tox -e typing

Actual result

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

homeassistant/components/knx/__init__.py:410: error: unused 'type: ignore' comment
homeassistant/components/knx/light.py:68: error: Returning Any from function declared to return "Optional[int]"  [no-any-return]
homeassistant/components/knx/fan.py:80: error: Returning Any from function declared to return "Optional[int]"  [no-any-return]
homeassistant/components/knx/fan.py:113: error: Returning Any from function declared to return "Optional[bool]"  [no-any-return]
homeassistant/components/knx/cover.py:68: error: Returning Any from function declared to return "Optional[str]"  [no-any-return]
homeassistant/components/knx/cover.py:104: error: Returning Any from function declared to return "Optional[bool]"  [no-any-return]
homeassistant/components/knx/cover.py:109: error: Returning Any from function declared to return "bool"  [no-any-return]
homeassistant/components/knx/cover.py:114: error: Returning Any from function declared to return "bool"  [no-any-return]
homeassistant/components/knx/binary_sensor.py:43: error: Returning Any from function declared to return "Optional[str]"  [no-any-return]
homeassistant/components/knx/binary_sensor.py:49: error: Returning Any from function declared to return "bool"  [no-any-return]
homeassistant/components/knx/binary_sensor.py:66: error: Returning Any from function declared to return "bool"  [no-any-return]
homeassistant/components/knx/weather.py:43: error: Returning Any from function declared to return "Optional[float]"  [no-any-return]
homeassistant/components/knx/weather.py:63: error: Returning Any from function declared to return "str"  [no-any-return]
homeassistant/components/knx/weather.py:68: error: Returning Any from function declared to return "Optional[float]"  [no-any-return]
homeassistant/components/knx/weather.py:73: error: Returning Any from function declared to return "Optional[int]"  [no-any-return]
homeassistant/components/knx/sensor.py:42: error: Returning Any from function declared to return "Union[None, str, int, float]"  [no-any-return]
homeassistant/components/knx/sensor.py:47: error: Returning Any from function declared to return "Optional[str]"  [no-any-return]
homeassistant/components/knx/sensor.py:54: error: Returning Any from function declared to return "Optional[str]"  [no-any-return]
homeassistant/components/knx/sensor.py:65: error: Returning Any from function declared to return "bool"  [no-any-return]
Found 19 errors in 7 files (checked 2855 source files)
homeassistant/components/knx/knx_entity.py:22: error: Returning Any from function declared to return "str"  [no-any-return]
homeassistant/components/knx/climate.py:72: error: Returning Any from function declared to return "Optional[float]"  [no-any-return]
homeassistant/components/knx/climate.py:77: error: Returning Any from function declared to return "float"  [no-any-return]
homeassistant/components/knx/climate.py:82: error: Returning Any from function declared to return "Optional[float]"  [no-any-return]
homeassistant/components/knx/climate.py:88: error: Returning Any from function declared to return "float"  [no-any-return]
homeassistant/components/knx/climate.py:94: error: Returning Any from function declared to return "float"  [no-any-return]
Found 6 errors in 2 files (checked 967 source files)

Expected result

Mypy finishes successfully.

Problem

xknx package with typing information is not installed.

Explanation

pre-commit is not configured to run mypy from tox venv: pre-commit/pre-commit#1309
So let's run it directly as pytest. Another option could be changing pre-commit config to use tox venv but then you won't be able to use pre-commit before tox environments are setup.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[frenck]

Its weird a bit, as the xknx package is part of our testing integration dependencies... So, I'm not sure what could cause this.

Anyways, this workaround should not be merged for the below given reasons.

[KapJI]

Well, I'm pointing out at the problem and explaining why this happens.
You're telling this shouldn't be fixed like this.

I agree it's likely not the best way to fix it.
But what should we do? Should we keep it broken? Or can you suggest a better way to fix this?

Mypy won't work without this typing information. Another option I see is install all packages from requirements_all.txt or requirements_test_all.txt in bootstrap script.

Or create requirements_test_typing.txt for packages which are required for mypy.

[frenck]

I think that typing needs indeed requirements_testing_all in this case:

https://github.com/home-assistant/core/blob/48c0cfb25c0d403506ac5fd0c68251712228e71f/tox.ini#L43

[KapJI]

It will only work if we also add pre-commit to requirements_test.txt so tox will run pre-commit from its own virtualenv.

Right now tox installs all packages from deps to typing virtualenv and then runs pre-commit from outer core virtualenv which can't see any packages installed in tox's typing virtualenv.

Are you ok with adding pre-commit in requirements_test.txt?

Edit: It's actually already there, not sure why it didn't work before. Let me try again.

[KapJI]

Ok, I got it. The problem is not in pre-commit not being isntalled, it's in how pre-commit runs tools.

It doesn't run tools from tox environment: pre-commit/pre-commit#1309

So let's run mypy directly as tox runs pytest.

Note that flake8, bandit and codespell are also not run from tox venv. I can fix them as well if you want to. Otherwise they are run from the main venv and may miss some dependencies, although right now they work fine.

After that:

Success: no issues found in 3822 source files
___________________________________________________________________________________________________ summary ____________________________________________________________________________________________________
  typing: commands succeeded
  congratulations :)

[farmio]

Hi 👋
Why didn't this error come up on CI before?

[KapJI]

@farmio CI has very different setup without using tox or pre-commit. It runs mypy directly from a special devcontainer where all packages are installed. Defined here.

So this will also make tox configuration closer to CI configuration.

[frenck]

Ok, I got it. The problem is not in pre-commit not being isntalled, it's in how pre-commit runs tools.

That seems incorrect. requirements_test_all.txt includes requirements_test.txt, which includes requirements_test_pre_commit.txt.

As shown in your initial screenshot, pre-commit was just working:

Please keep pre-commit in place.

[KapJI]

@frenck I think you didn't get my explanation. Let me try again.

tox installs dependencies in its own venv and runs pre-commit from there. To this point everything is correct, pre-commit is installed and working.

But pre-commit doesn't run mypy from tox venv. Instead it runs it from parent venv which doesn't have dependencies installed by tox. This causes the errors.

pre-commit can be configured to run mypy installed by tox. For this its config should be changed to run .tox/typing/bin/mypy instead of current script/run-in-env.sh mypy.

But this will result that pre-commit can't be used before tox installs typing venv (which resides in .tox/typing/).

So IMO it's better to run mypy directly in tox to not escape venv configured by tox.

[frenck]

Thanks for explaining 👍

LGTM