Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated mypy and pytest-mypy #304

Closed
wants to merge 5 commits into from
Closed

Conversation

jacobcook1995
Copy link
Collaborator

Description

On my local setup (macOS python 3.11) the mypy part of the unit testing takes ages to run. When I deleted the mypy cache files the unit test took 87.84s to run. When I updated mypy and pytest-mypy this same test dropped to 25ish seconds.

I felt like this was a big enough change to be worth updating the dependancies for (even if it only affected my setup), provided that others agree.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist

  • [N/A] Make sure you've run the pre-commit checks: $ pre-commit run -a
  • [N/A] All tests pass: $ poetry run pytest

Further checks

  • [N/A] Code is commented, particularly in hard-to-understand areas
  • [N/A] Tests added that prove fix is effective or that feature works

@codecov-commenter
Copy link

Codecov Report

Merging #304 (35a6fb8) into develop (1e0ca4e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #304   +/-   ##
========================================
  Coverage    97.03%   97.03%           
========================================
  Files           51       51           
  Lines         2293     2293           
========================================
  Hits          2225     2225           
  Misses          68       68           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jacobcook1995 jacobcook1995 marked this pull request as draft September 5, 2023 14:13
@jacobcook1995
Copy link
Collaborator Author

Made this a draft as the newest version of mypy does not seem to like the constants registry. I'm going to test if upgrading to 1.0.1 improves the performance, and if not I'll just have to accept that mypy runs very slowly on my machine

@jacobcook1995
Copy link
Collaborator Author

Updating mypy to v1.0.1 increases the speed by less (55ish s), but doesn't break the pre-commit hook, so might be worth doing?

@davidorme
Copy link
Collaborator

What is the issue with the constants registry?

@jacobcook1995
Copy link
Collaborator Author

This was the error message virtual_rainforest/core/constants.py:79: error: Argument 1 to "fields" has incompatible type "Callable[..., Any]"; expected "Union[DataclassInstance, type[DataclassInstance]]" [arg-type].

Previously, mypy was happy with the constants registry being typed as follows:

CONSTANTS_REGISTRY: dict[str, dict[str, Callable]] = {}

But now it infers that the type of the object added to the constants registry is DataclassInstance. I can import this (from _typeshed) and use it as a type hint, but then this causes another mypy error as DataclassInstance doesn't have the attribute .__name__

@alexdewar
Copy link
Collaborator

I think upgrading mypy is a good idea.

One other thing you could do is telling mypy to exclude the .venv folder. We do it on other projects, though I've never actually checked whether it is actually needed and speeds things up.

@jacobcook1995
Copy link
Collaborator Author

@alexdewar given we are using poetry I don't think we have a .venv folder? Or at I can't see a folder called that within the virtual_rainforest folder on my local machine.

Is it worth working out how to exclude poetry equivalents of this folder, or is safe to assume that they will be excluded anyway?

@alexdewar
Copy link
Collaborator

@jacobcook1995 Ah, well if that's the case, then that won't be why you're seeing slowdowns. Not sure why that is. It might be worth seeing if someone's raised an issue in the mypy repo and, if not, raising one of your own.

(I actually do have a .venv folder in my repo because I've set the virtualenvs.in-project option for poetry.)

@davidorme
Copy link
Collaborator

I don't think we're in general using virtualenvs.in-project = true, so shouldn't have any Python environments floating around inside the project directory.

@davidorme
Copy link
Collaborator

Superseded by changes in #323

@davidorme davidorme closed this Oct 16, 2023
@jacobcook1995 jacobcook1995 deleted the feature/update_mypy branch November 28, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants