ci: fix lint failure of mypy due to modern typed libraries #305
Conversation
maartenbreddels
force-pushed
the
fix_ci_lint_traitlets
branch
from
d5ba3fd to
9c734d7
Compare
maartenbreddels
changed the title
maartenbreddels
force-pushed
the
fix_ci_lint_traitlets
branch
from
9c734d7 to
05680db
Compare
|
Also matplotlib 3.8 seems to introduce typing which makes mypy linting fail. |
|
This should be tagged as technical debt and, in my opinion, prioritized. |
| @@ -23,7 +23,7 @@ jobs: | |||
| python-version: ${{ matrix.python-version }} | |||
| - name: Install dependencies | |||
| run: | | |||
| pip install ".[dev]" mypy==0.991 black==22.12.0 codespell==2.2.4 "click<8.1.4" | |||
| pip install ".[dev]" mypy==0.991 black==22.12.0 codespell==2.2.4 "click<8.1.4" "traitlets<5.10.0" "matplotlib<3.8.0" | |||
There was a problem hiding this comment.
Versions are currently pinned in the GitHub workflow, but not in the pyproject.toml (optional) dependencies. Doesn't that mean users can install dependency versions that are not supported or tested by solara?
As far as I know, best practice is to define dependencies in pyproject.toml and use those in testing/CI. If you have dependencies only used in testing or dev, add them to an optional dependency list.
There was a problem hiding this comment.
I agree that we should fix this soon, but it's not as high a priority as a runtime failure.
However, I disagree we should do version pinning in solara since that can make environments unsolvable. Pinning should only be done by applications (or application-like libraries, or when you know for sure there is a failure.
There was a problem hiding this comment.
Thanks for getting back. I agree that pinning exact versions is bad practice (for exactly the reasons you said),
Also I can't imagine black and mypy should be required dependencies, those should be optional dependencies (in .[dev] or .[ci] for example), and then regular users won't install them. Thus limiting the version range doesn't affect them, since it's purely for internal use.
Introduced in traitlets 5.10 due to
The correct way would be to fix the errors, but we don't want CI to fail randomly. This also supports our idea of having lockfiles for CI.