Bump mypy version to ~=1.13.0 #6630

unkcpz · 2024-11-22T01:09:37Z

Bump mypy version to ~=1.13.0
fixes all failed mypy checks.

codecov · 2024-11-22T01:55:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.90%. Comparing base (ef60b66) to head (7b18eab).
Report is 137 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6630      +/-   ##
==========================================
+ Coverage   77.51%   77.90%   +0.40%     
==========================================
  Files         560      567       +7     
  Lines       41444    42147     +703     
==========================================
+ Hits        32120    32831     +711     
+ Misses       9324     9316       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

danielhollas · 2024-11-22T03:15:31Z

I suspect that the sudden pre-commit failures are due to a new pydantic version 2.10 which was released yesterday.

https://docs.pydantic.dev/latest/changelog/

unkcpz · 2024-11-22T11:24:37Z

I suspect that the sudden pre-commit failures are due to a new pydantic version 2.10 which was released yesterday.

Right, but the pydantic type errors didn't show up with the new mypy, so I think we just bump mypy and solve the new issues here.

danielhollas · 2024-11-22T11:33:44Z

It's not because of mypy, they actually released a patch version 2.10.1 that appear to fix some of the issues. But I agree that we might as well bump mypy.

unkcpz · 2024-11-22T15:59:45Z

Here are all errors fixed:

src/aiida/common/lang.py:99: error: Argument 1 has incompatible type "SelfType@__get__"; expected "SelfType@__init__"  [arg-type]
src/aiida/common/pydantic.py:45: error: Item "None" of "Any | None" has no attribute "metadata"  [union-attr]
src/aiida/manage/manager.py:433: error: Argument 1 to "Runner" has incompatible type "**dict[str, float | Any]"; expected "AbstractEventLoop | None"  [arg-type]
src/aiida/manage/manager.py:433: error: Argument 1 to "Runner" has incompatible type "**dict[str, float | Any]"; expected "bool"  [arg-type]
src/aiida/manage/manager.py:433: error: Argument 1 to "Runner" has incompatible type "**dict[str, float | Any]"; expected "Persister | None"  [arg-type]
src/aiida/plugins/utils.py:67: error: Subclass of "str" and "FunctionType" cannot exist: "FunctionType" is final  [unreachable]
src/aiida/plugins/utils.py:67: error: Subclass of "type" and "FunctionType" cannot exist: "FunctionType" is final  [unreachable]
src/aiida/orm/nodes/data/singlefile.py:74: error: Unused "type: ignore" comment  [unused-ignore]
src/aiida/orm/nodes/data/list.py:84: error: Signature of "pop" incompatible with supertype "MutableSequence"  [override]
src/aiida/orm/nodes/data/list.py:84: note:      Superclass:
src/aiida/orm/nodes/data/list.py:84: note:          def pop(self, index: int = ...) -> Any
src/aiida/orm/nodes/data/list.py:84: note:      Subclass:
src/aiida/orm/nodes/data/list.py:84: note:          def pop(self, **kwargs: Any) -> Any
src/aiida/orm/nodes/data/list.py:92: error: Signature of "index" incompatible with supertype "Sequence"  [override]
src/aiida/orm/nodes/data/list.py:92: note:      Superclass:
src/aiida/orm/nodes/data/list.py:92: note:          def index(self, value: Any, start: int = ..., stop: int = ...) -> int
src/aiida/orm/nodes/data/list.py:92: note:      Subclass:
src/aiida/orm/nodes/data/list.py:92: note:          def index(self, value: Any) -> Any
src/aiida/orm/nodes/data/array/projection.py:281: error: Signature of "set_orbitals" incompatible with supertype "OrbitalData"  [override]
src/aiida/orm/nodes/data/array/projection.py:281: note:      Superclass:
src/aiida/orm/nodes/data/array/projection.py:281: note:          def set_orbitals(self, orbitals: Any) -> Any
src/aiida/orm/nodes/data/array/projection.py:281: note:      Subclass:
src/aiida/orm/nodes/data/array/projection.py:281: note:          def set_orbitals(self, **kwargs: Any) -> Any
src/aiida/engine/processes/workchains/restart.py:160: error: Argument 1 to "validate_handler_overrides" has incompatible type "type[BaseRestartWorkChain]"; expected "BaseRestartWorkChain"  [arg-type]
src/aiida/engine/processes/calcjobs/calcjob.py:1065: error: Argument 1 to "asdict" has incompatible type "DataclassInstance | type[DataclassInstance]"; expected "DataclassInstance"  [arg-type]
tests/cmdline/groups/test_dynamic.py:20: error: Incompatible types in assignment (expression has type "bool", variable has type "str")  [assignment]
src/aiida/tools/pytest_fixtures/daemon.py:130: error: Right operand of "and" is never evaluated  [unreachable]
src/aiida/tools/pytest_fixtures/daemon.py:131: error: Statement is unreachable  [unreachable]

unkcpz · 2024-11-22T16:38:31Z

It seems last commit change fail the test.

.readthedocs.yml

unkcpz · 2024-11-22T17:10:12Z

src/aiida/common/pydantic.py

@@ -41,7 +41,7 @@ class Model(BaseModel):
    field_info = Field(default, **kwargs)

    for key, value in (('priority', priority), ('short_name', short_name), ('option_cls', option_cls)):
-        if value is not None:
+        if value is not None and field_info is not None:


Field seems can return None, if so the field_info won't have metadata attribute.

Nice fix. cc @edan-bainglass for visibility

unkcpz · 2024-11-22T17:12:42Z

src/aiida/engine/processes/calcjobs/calcjob.py

@@ -1062,7 +1062,7 @@ def presubmit(self, folder: Folder) -> CalcInfo:

        def encoder(obj):
            if dataclasses.is_dataclass(obj):
-                return dataclasses.asdict(obj)
+                return dataclasses.asdict(obj)  # type: ignore[arg-type]


Not sure I should cast the type here, for the readability, I ignore the error.

Would try to solve this in a subsequent PR

unkcpz · 2024-11-22T17:13:55Z

tests/cmdline/groups/test_dynamic.py

@@ -17,7 +17,7 @@ class Model(BaseModel):
        union_type: t.Union[int, float] = Field(title='Union type')
        without_default: str = Field(title='Without default')
        with_default: str = Field(title='With default', default='default')
-        with_default_factory: str = Field(title='With default factory', default_factory=lambda: True)
+        with_default_factory: str = Field(title='With default factory', default_factory=lambda: True)  # type: ignore[assignment]


It is test, so I just ignore it.

Actually, when I run mypy 1.13 on this, the # type: ignore[assignment] is not necessary?

unkcpz · 2024-11-22T17:14:35Z

src/aiida/orm/nodes/data/array/projection.py

@@ -278,7 +278,7 @@ def array_list_checker(array_list, array_name, orb_length):
                raise exceptions.ValidationError('Tags must set a list of strings')
            self.base.attributes.set('tags', tags)

-    def set_orbitals(self, **kwargs):
+    def set_orbitals(self, **kwargs):  # type: ignore[override]


I'll just leave kwargs as it is and ignore type checking.

Can you add a TODO comment with the error? The mypy error suggests that this is an actual error? Or do you think it's a false positive?

projection.py:281: error: Signature of "set_orbitals" incompatible with supertype "OrbitalData" [override] projection.py:281: note: Superclass: projection.py:281: note: def set_orbitals(self, orbitals: Any) -> Any projection.py:281: note: Subclass: projection.py:281: note: def set_orbitals(self, **kwargs: Any) -> Any

Neither, I think it is a design issue to use inheritance everywhere in the code base. Here it makes sense that the ProjectionData is only inheritant from ArrayData. For reduce the code duplication, can move the shared methods to dedicate functions or classes. It requires the actual use case of ProjectionData which I think it is in aiida-quantumespresso.

But since this method is just to override and block the method, so the type in the function call doesn't matter. Just ignore to skip the checker.

Can we just merge it and if this is a problem make an issue out of it?

Yes, I move this change to #6635, so let's continue the discussion there.

May fix doc failed build

src/aiida/engine/processes/workchains/restart.py

danielhollas · 2024-11-23T18:59:01Z

src/aiida/tools/pytest_fixtures/daemon.py

@@ -116,7 +116,7 @@ def test(submit_and_await):
    from aiida.engine import ProcessState

    def factory(
-        submittable: 'Process' | 'ProcessBuilder' | 'ProcessNode',
+        submittable: type[Process] | 'ProcessBuilder' | 'ProcessNode' | t.Any,


Why is t.Any needed here? This makes the type checking weaker, would be nice if we could avoid it.

Because there is an else branch otherwise the branch is unreachable.

Yes, makes sense, sorry I originally missed your comment with the errors.

I think the correct fix here is to add type ignores on those branches, since those essentially represent runtime type checking, which we need to enforce. At the same time we don't want to make static type checking weaker.

I may not fully agree. I think types in the function signature plays two functionalities. On the one hand, it partially is the hint for the function behavior, like in this case in the function body it requires to explicitly deal with three types and raise if it doesn't know what to do with certain type. On the other hand, the type in the function signature is for other function to know what should be passed to the function when it is called.
Ideally the # type: ignore should all be addressed.
If we just rely on runtime type check, then the type should be t.Any. Here I still think it is better to have types as I changed.

Ideally the # type: ignore should all be addressed.

I agree with this sentiment, but introducing Any is just another way to silence type checking. :-) I would argue that in this case Any is worse, since it "turns off" type checking of this parameter for the whole function, whereas the "type: ignore" would only target a specific part of the code. In this case, there is a real tension between static and runtime type checking: the else branches that are "unreachable" to the static type checker, but static types are not enforce at runtime in Python, and we can't enforce users to use mypy (e.g. in plugins that use this fixture), hence the additional runtime check.

If we just rely on runtime type check, then the type should be t.Any

But we don't want to rely on runtime checks only. If we can catch it statically that's certainly better, no?

Sorry if I am being terse, I am teaching now, am happy to write more later today if needed.

I think the correct thing to do here is to NOT change the type annotation, since it was correct.

add type: ignore to the else branch, where the error originates.
The error is not really about type annotation, but about unreachable code. But we already know that branch is unreachable, unless the user screws up and passes a wrong type.

Process -> type[Process] should be a correct change, in this function, it expect both workchain and calcjob which are subclass of Process. ~~If it is just Process, then the issubclass(submittable, Process) part is useless because and shortcut the condition.~~
We can have a zoom chat if you like, can be easier.

I agree that the t.Any is not necessary since we won't expect the function is used besides those three types. But after change to type[Process] which I think it is correct, there is not need to add ignore anywhere to make mypy work.

Again, I personally don't like the way how subclass of Process implemented here. I think this is a place where the duck typing should used. Then with using python protocol, the generic submit function can just expect the class it calls has a submit method.

In mega office, we decide to merge this one and continue the discussion here to not block other PRs. @danielhollas hope you don't mind.

Sure, go ahead

danielhollas

Thanks @unkcpz! Can you revert the changes to src/aiida/orm/nodes/data/list.py and only add type: ignore, so we can merge this, and make a separate PR for those changes? Otherwise LGTM

src/aiida/orm/nodes/data/list.py

GeigerJ2

LGTM. Thanks for taking the time to look into this, @unkcpz!

GeigerJ2 · 2024-11-25T09:48:22Z

tests/cmdline/groups/test_dynamic.py

@@ -17,7 +17,7 @@ class Model(BaseModel):
        union_type: t.Union[int, float] = Field(title='Union type')
        without_default: str = Field(title='Without default')
        with_default: str = Field(title='With default', default='default')
-        with_default_factory: str = Field(title='With default factory', default_factory=lambda: True)
+        with_default_factory: str = Field(title='With default factory', default_factory=lambda: True)  # type: ignore[assignment]


Actually, when I run mypy 1.13 on this, the # type: ignore[assignment] is not necessary?

GeigerJ2

LGTM. Thanks for taking the time to look into this, @unkcpz!

unkcpz · 2024-11-25T10:15:40Z

Actually, when I run mypy 1.13 on this, the # type: ignore[assignment] is not necessary?

This is the one required by the new pydantic. If you update the pydantic to 2.10.1 you can probably see this.

src/aiida/tools/pytest_fixtures/daemon.py

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

agoscinski

Please merge because it is blocking all PRs, we can put another PR to fix the remaining small issues.

agoscinski · 2024-11-25T13:58:02Z

src/aiida/orm/nodes/data/array/projection.py

@@ -278,7 +278,7 @@ def array_list_checker(array_list, array_name, orb_length):
                raise exceptions.ValidationError('Tags must set a list of strings')
            self.base.attributes.set('tags', tags)

-    def set_orbitals(self, **kwargs):
+    def set_orbitals(self, **kwargs):  # type: ignore[override]


Can we just merge it and if this is a problem make an issue out of it?

agoscinski · 2024-11-25T13:58:45Z

src/aiida/engine/processes/calcjobs/calcjob.py

@@ -1062,7 +1062,7 @@ def presubmit(self, folder: Folder) -> CalcInfo:

        def encoder(obj):
            if dataclasses.is_dataclass(obj):
-                return dataclasses.asdict(obj)
+                return dataclasses.asdict(obj)  # type: ignore[arg-type]


Would try to solve this in a subsequent PR

danielhollas · 2024-11-25T22:51:47Z

src/aiida/plugins/utils.py

@@ -38,7 +38,7 @@ def __init__(self):
    def logger(self) -> Logger:
        return self._logger

-    def get_version_info(self, plugin: str | type) -> dict[t.Any, dict[t.Any, t.Any]]:


@unkcpz FYI I think this has a similar issue that we discussed, I don't think adding Any here is correct. Let's discuss on the other PR, but just flagging it here as well so we don't forget.

The original error was:

src/aiida/plugins/utils.py:67: error: Subclass of "str" and "FunctionType" cannot exist: "FunctionType" is final [unreachable]
src/aiida/plugins/utils.py:67: error: Subclass of "type" and "FunctionType" cannot exist: "FunctionType" is final [unreachable]

Still struggling to understand, will need to experiment a bit.

Bump mypy version to ~=1.13.0

3e4f3ef

unkcpz marked this pull request as draft November 22, 2024 01:09

ignore

b6800dc

unkcpz added 3 commits November 22, 2024 16:33

Try fix

f415f55

Pydantic version caused mypy error

0dabade

Merge branch 'main' into mypy/fix/xx

2c0948f

unkcpz marked this pull request as ready for review November 22, 2024 15:56

unkcpz requested review from danielhollas and GeigerJ2 and removed request for danielhollas November 22, 2024 16:00

unkcpz marked this pull request as draft November 22, 2024 16:38

unkcpz force-pushed the mypy/fix/xx branch from 5d6408b to 3a88318 Compare November 22, 2024 16:47

unkcpz commented Nov 22, 2024

View reviewed changes

unkcpz force-pushed the mypy/fix/xx branch 3 times, most recently from ea70514 to ac5473e Compare November 22, 2024 17:29

amend

ac5473e

May fix doc failed build

unkcpz marked this pull request as ready for review November 22, 2024 18:01

danielhollas reviewed Nov 23, 2024

View reviewed changes

src/aiida/engine/processes/workchains/restart.py Show resolved Hide resolved

danielhollas reviewed Nov 23, 2024

View reviewed changes

danielhollas requested changes Nov 23, 2024

View reviewed changes

src/aiida/orm/nodes/data/list.py Outdated Show resolved Hide resolved

unkcpz force-pushed the mypy/fix/xx branch from c2538cd to 7b9e02e Compare November 23, 2024 23:39

rd

7b9e02e

unkcpz mentioned this pull request Nov 23, 2024

List mypy fix #6635

Merged

unkcpz requested a review from danielhollas November 23, 2024 23:44

GeigerJ2 reviewed Nov 25, 2024

View reviewed changes

GeigerJ2 approved these changes Nov 25, 2024

View reviewed changes

unkcpz commented Nov 25, 2024

View reviewed changes

src/aiida/tools/pytest_fixtures/daemon.py Outdated Show resolved Hide resolved

unkcpz force-pushed the mypy/fix/xx branch from 0ee351b to d1cc350 Compare November 25, 2024 10:40

unkcpz added 2 commits November 25, 2024 11:41

Update src/aiida/tools/pytest_fixtures/daemon.py

d1cc350

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Merge branch 'main' into mypy/fix/xx

e636253

unkcpz force-pushed the mypy/fix/xx branch from 704cf17 to 7b18eab Compare November 25, 2024 12:48

More

7b18eab

agoscinski mentioned this pull request Nov 25, 2024

Bump ruff version #6614

Merged

agoscinski approved these changes Nov 25, 2024

View reviewed changes

unkcpz merged commit c93fb4f into aiidateam:main Nov 25, 2024
29 checks passed

unkcpz deleted the mypy/fix/xx branch November 25, 2024 15:12

danielhollas reviewed Nov 25, 2024

View reviewed changes

Bump mypy version to ~=1.13.0 #6630

Bump mypy version to ~=1.13.0 #6630

Conversation

unkcpz commented Nov 22, 2024 • edited Loading

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

danielhollas commented Nov 22, 2024

unkcpz commented Nov 22, 2024 • edited Loading

danielhollas commented Nov 22, 2024

unkcpz commented Nov 22, 2024

unkcpz commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

unkcpz Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas left a comment

Choose a reason for hiding this comment

GeigerJ2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeigerJ2 left a comment

Choose a reason for hiding this comment

unkcpz commented Nov 25, 2024

agoscinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

unkcpz commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

unkcpz commented Nov 22, 2024 •

edited

Loading

danielhollas Nov 23, 2024 •

edited

Loading

unkcpz Nov 23, 2024 •

edited

Loading

unkcpz Nov 23, 2024 •

edited

Loading

unkcpz Nov 25, 2024 •

edited

Loading

danielhollas Nov 25, 2024 •

edited

Loading