Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Aug 29, 2023

This PR mainly split mypy check for the source package and dev files.

mypy tends to have issues with checking source files inside a src/ directory. There are tricks1 but it seems like the only way to do it is to run mypy on . using MYPYPATH=src, which will still check the whole . directory (include whole virtualenvs if there are any), which is not what we want. For now we've been using -p to check for packages instead of files, which seemed to have done the trick, but we need to pass directories that don't really have a package structure (like benchmarks or examples or tests) as packages, which is also not great.

We try a different approach, we check the source code as a package separately, and then check the development files (tests, examples, etc.), which are passed as simple paths. This also opens up the door to using more relaxed rules for these development files if needed in the future.

To achieve this we also move the packages specification to the pyproject.toml file, so now mypy can be called without any arguments to check the source code package, making it easier to call it without relaying in nox.

We also move all the options to the pyproject.toml file, so mypy can be run consistently also outside of nox, and the stub dependencies now need to be specified manually in the pyproject.toml file, so they can be pinned.

This is also a step towards removing nox (#71).

Footnotes

  1. https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules

@llucax llucax self-assigned this Aug 29, 2023
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Aug 29, 2023
@llucax llucax added this to the v0.6.0 milestone Aug 29, 2023
@Marenz
Copy link
Contributor

Marenz commented Aug 29, 2023

First commit has a weird phrasing "Most options can be specified in the pyproject.toml file and this have some advantages."

"mypy == 1.5.1",
"types-setuptools >= 67.6.0, < 68", # Should match the global dependency
"types-setuptools == 68.1.0.0", # Should match the build dependency
"types-Markdown == 3.4.2.10",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this updated when the mypy version changes? Like, who keeps it in sync? Dependabot would just pick the latest versions no? And if those don't match what mypy deps need?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mypy is not in charge of these dependencies, they usually need to match the python package, like types-markdown should have the sameish version as the markdown package that we are using.

We are actually not pinning transitional dependencies, which at some point could be problematic (this is a problem to think about in the future, there are many optional package managers that take care of this, like pipenv). For example, in this case we don't depend directly on the markdown package, so it is a bit strange that we need the stub.

All that mypy does is somehow looking for packages without type info and look for them in https://github.com/python/typeshed to see if there is a stub package providing it and then installing that via pip.

@Marenz
Copy link
Contributor

Marenz commented Aug 29, 2023

two comments, otherwise LGTM

@llucax
Copy link
Contributor Author

llucax commented Aug 29, 2023

Rephrased the commit, squashed the fixup commits and added a few more commits, most notably release notes and the no_incremental option. Testing via frequenz-floss/frequenz-sdk-python#587.

@llucax
Copy link
Contributor Author

llucax commented Aug 29, 2023

Not tested in frequenz-floss/frequenz-sdk-python#587 anymore because all the pydoclint tests are too disruptive. Waiting to test it after that PR (which will include an ad-hoc hack) is merged.

@llucax
Copy link
Contributor Author

llucax commented Aug 29, 2023

The fix for mypy weird behavior is to disable caching, which makes it slower (although not by a LOT). I think all the other changes here are still worth it, so I would keep them, but I'm not sure if we should disable caching by default or not, as it is a very awful problem to debug but probably not affecting other simpler project (specially ones not using grpc stuff), so maybe slow everything down just in case is not justified. I'm considering adding the option to the template but commented out, with a comment explaining why is it there.

@llucax llucax force-pushed the mypy branch 2 times, most recently from b7de292 to ac96b0b Compare August 30, 2023 10:24
@llucax
Copy link
Contributor Author

llucax commented Aug 30, 2023

New PR for testing this: frequenz-floss/frequenz-sdk-python#620

@llucax llucax marked this pull request as ready for review August 30, 2023 18:56
@llucax llucax requested a review from a team as a code owner August 30, 2023 18:56
@llucax llucax requested a review from Marenz August 30, 2023 18:56
@llucax
Copy link
Contributor Author

llucax commented Aug 30, 2023

The test PR seems to be working well, so this is ready for the final review and merge (and release).

@llucax llucax enabled auto-merge August 30, 2023 18:56
@llucax
Copy link
Contributor Author

llucax commented Aug 30, 2023

Enabling auto-merge.

llucax added 9 commits August 30, 2023 21:27
This new release will not require a function `yield`ing `None` to have
a `Yields:` section.

Signed-off-by: Leandro Lucarella <[email protected]>
Flake8 is faster, so we prefer it.

Signed-off-by: Leandro Lucarella <[email protected]>
We want to run the checks from the fatests to the slowest, so we get
errors as early as possible.

Signed-off-by: Leandro Lucarella <[email protected]>
It is possible to define most options within the `pyproject.toml` file,
which offers several advantages: the tool can be used from the command
line directly (bypassing the need for `nox`), without having to remember
the options. It also facilitates other integrations that need to invoke
mypy directly.

Signed-off-by: Leandro Lucarella <[email protected]>
`mypy` tends to have issues with checking source files inside a `src/`
directory. There are tricks[1] but it seems like the only way to do it
is to run `mypy` on `.` using `MYPYPATH=src`, which will still check the
whole `.` directory (include whole virtualenvs if there are any), which
is not what we want. For now we've been using `-p` to check for packages
instead of files, which seemed to have done the trick, but we need to
pass directories that don't really have a package structure (like
benchmarks or examples or tests) as packages, which is also not great.

This commit tries a different approach, we check the source code as a
package separately, and then check the development files (tests,
examples, etc.), which are passed as simple paths. This also opens up
the door to using more relaxed rules for these development files if
needed in the future.

To achieve this we also move the `packages` specification to the
`pyproject.toml` file, so now `mypy` can be called without any arguments
to check the source code package, making it easier to call it without
relaying in `nox`.

[1] https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules

Signed-off-by: Leandro Lucarella <[email protected]>
This was used by the `mypy` `nox` session.

Signed-off-by: Leandro Lucarella <[email protected]>
When installing stubs automatically with `mypy` we can't pin the
dependencies, and we make it harder to just run `mypy` without going
through `nox`.

From now on, dependencies must be explicitly declared in the
`pyproject.toml` file, in the optional `dev-mypy` dependencies.

Signed-off-by: Leandro Lucarella <[email protected]>
This option avoids using a cache. It is slower but prevents some issues
with `mypy` giving different results on different runs (typically
complaining about some `type: ignore[issue]` not being used but when
removing it complaining about an `issue` again.

It is included so it has more visibility to users in the hope that they
can spend less time debugging if they hit this `mypy` issue.

Signed-off-by: Leandro Lucarella <[email protected]>
llucax added 3 commits August 30, 2023 21:27
Signed-off-by: Leandro Lucarella <[email protected]>
This is to prepare for the release.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax added this pull request to the merge queue Aug 31, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit 2e084dc Aug 31, 2023
@llucax llucax deleted the mypy branch August 31, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

Development

Successfully merging this pull request may close these issues.

2 participants