Run mypy on CI, fix or ignore typing issues

[krassowski]

Fixes #985

For now this only runs mypy on jupyter-ai but not yet on jupyter-ai-magics (which we may decide to add in a separate PR).

[dlqqq]

@krassowski Thank you for this significant contribution! This was on our to-do list for a while but we never managed to get around to it.

Since this is a large PR that changes many type hints that pydantic relies on, I do want to allow myself more time to test this thoroughly. I plan to review this tomorrow after I cut a release today.

[krassowski]

@dlqqq If you mean the ellipses, I think that is a straightforward change. Quoting from pydantic documentation (emphasis mine):

To declare a field as required, you may declare it using an annotation, or an annotation in combination with a Field specification. You may also use Ellipsis/... to emphasize that a field is required, especially when using the Field constructor.

[...]

from pydantic import BaseModel, Field


class Model(BaseModel):
    a: int
    b: int = ...
    c: int = Field(..., alias='C')

Here a, b and c are all required. However, this use of b: int = ... does not work properly with mypy, and as of v1.0 should be avoided in most cases.

https://docs.pydantic.dev/latest/concepts/models/#field-ordering

[krassowski]

and as of v1.0 should be avoided in most cases

to be clear, this is verbatim from the latest v2 documentation so should be taken as applying to both v1 and v2 :)

[dlqqq]

There's an unrelated error in CI for the E2E tests workflow:

Error: This request has been automatically failed because it uses a deprecated version of `actions/upload-artifact: v2`. Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

I'm looking into this now and will open a separate PR to address this.

[dlqqq]

This PR looks good to me! However, when I run mypy locally, I get an error regarding the deepmerge package being untyped, even though the same check passes in CI. I'm using mypy 1.11.2, same as the CI env. Not sure what could be causing this 🤔

% mypy packages/jupyter-ai
packages/jupyter-ai/jupyter_ai/config_manager.py:8: error: Skipping analyzing "deepmerge": module is installed, but missing library stubs or py.typed marker  [import-untyped]
packages/jupyter-ai/jupyter_ai/config_manager.py:8: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
packages/jupyter-ai/jupyter_ai/handlers.py:556: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
packages/jupyter-ai/jupyter_ai/extension.py:393: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
packages/jupyter-ai/jupyter_ai/extension.py:420: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
Found 1 error in 1 file (checked 27 source files)

% mypy --version
mypy 1.11.2 (compiled: yes)

[krassowski]

You need to update deepmerge to newer version - this is why there is no error on CI.

[krassowski]

See toumorokoshi/deepmerge#30 - this was merged 8 months ago. I also had an old version in my dev env :)

[dlqqq]

@krassowski Awesome, thanks for finding that reference so quickly!

One remaining question: Should we bump the version specifier on deepmerge and require deepmerge>=2.0.0? I went through the changelog and it looks safe to me, but I'd like your take on this as well.

[krassowski]

My thinking was that an update was not needed because end users do not care about mypy checks on CI, and developers will not get it auto-updated when they pull changes either (unless they reinstall the environment). I wonder if it should have an upper bound on <3 just to ensure a future release does not break installation.

But if you think it is worth pinning the floor, then let's do that!

[dlqqq]

Thank you for your contribution & your suggestions! I've bumped the version floor and added a version ceiling in the latest commit. Everything looks good. Will merge pending green CI.