Bump mypy to 1.16.0 #6858

danielhollas · 2025-05-06T01:47:50Z

This is a nice bump since we can remove a lot of type: ignore comments!

.pre-commit-config.yaml

danielhollas · 2025-05-06T01:50:14Z

src/aiida/cmdline/commands/cmd_profile.py

@@ -122,17 +122,17 @@ def profile_setup():
    """Set up a new profile."""


-@verdi_profile.command('configure-rabbitmq')  # type: ignore[arg-type]
+@verdi_profile.command('configure-rabbitmq')
+@click.pass_context


Not quite sure why, but moving pass_context here fixed the following error:

src/aiida/cmdline/commands/cmd_profile.py:135: error: Argument 1 to "pass_context" has incompatible type "Callable[[Any, Any, Any, Any, KwArg(Any)], Any]"; expected "Callable[[Context, Any, Any, Any, KwArg(Any)], Any]" [arg-type] src/aiida/cmdline/commands/cmd_profile.py:135: note: This is likely because "profile_configure_rabbitmq" has named arguments: "ctx". Consider marking them positional-only

I guess @verdi_profile.command() change the function signature and make @click.pass_context lose track on what need to wrap.
~~If switch works and not break verdi profile tests then should be okay.~~

UPDATE: sorry, I didn't see the full change, I thought the @click.pass_context was moved as the first layer of wrapper.

I'd suggest not change the order of decorators, the @click.pass_context should be the decorator that directly applied to the function to get the ctx. If there are other decorators in the middle that mutate ctx I assume it may cause unexpected behavior.

You forgot this one. Maybe not change the order, change the order of decorator change the behavior, correct?

Ah, yes, thanks for the remainder. As you noticed, in the end I just read the error message more carefuly and marked ctx as a position-only argument. I am not sure why it is necessary here, and not in the other places? Perhaps because we have the **kwargs here. But in any case, this feels like a safe change.

danielhollas · 2025-05-06T01:53:28Z

src/aiida/engine/processes/functions.py

-        except Exception as exception:
-            LOGGER.warning(f'function `{func.__name__}` has a docstring that could not be parsed: {exception}')
-            param_help_string = {}
+        if func.__doc__ is None:


This fixes:

src/aiida/engine/processes/functions.py:377: error: Argument 1 to "parse" has incompatible type "str | None"; expected "str" [arg-type]

Github is very confused, the diff here is actually not that bad, we're just handling the func.__doc__ is None case pre-emptively. (We could also log a warning if a calcfunction is missing a docstring, but that seems overly pedantic)

We could also log a warning if a calcfunction is missing a docstring, but that seems overly pedantic

Yes, I think that is a bit too much, I won't throw warning for that.

danielhollas · 2025-05-06T01:54:28Z

src/aiida/engine/processes/functions.py

@@ -435,7 +439,7 @@ def define(cls, spec):
                    def indirect_default(value=default):
                        return to_aiida_type(value)
                else:
-                    indirect_default = default
+                    indirect_default = default  # type: ignore[assignment]


Not super clear how to solve this one, since indirect_default can be basically anything

src/aiida/engine/processes/functions.py:438: error: Incompatible types in assignment (expression has type "Any | tuple[()]", variable has type "Callable[[Any], Any]") [assignment]

If it can be anything then as error message suggest use Callable[[Any], Any] ?

I am not sure what are you suggesting here? the callable type is inferred because indirect_default is defined as a function in the if branch above, but in the else branch in can be anything. I think this we could squash this by inverting the logic and put the else branch first, but I don't really want to touch the logic.

Okay, I misunderstood the change here.

I think the mypy complain because the typechecker resolve the expression in if..else.. branch and want to make sure the type of expression conform with each other.

Then the correct way to solve this might be for the else branch do:

def indirect_default(value=default): return lambda _: value

(I am not sure this is correct, but I think it is better to just ignore the type error here to not touch the logic. so if you don't want to try please ignore my comment.)

src/aiida/engine/processes/process.py

codecov · 2025-05-06T02:05:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.15%. Comparing base (a392f5c) to head (3734865).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6858      +/-   ##
==========================================
+ Coverage   79.14%   79.15%   +0.01%     
==========================================
  Files         565      565              
  Lines       43391    43395       +4     
==========================================
+ Hits        34339    34343       +4     
  Misses       9052     9052

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

agoscinski · 2025-05-06T06:33:56Z

Note to me: There seems to be also mypy fixes here #6641 Need to check this too. I think we first merge this PR and then rebase the other

agoscinski · 2025-05-06T06:40:45Z

I approve the changes. Just waiting till we release v2.7.0 before merge

danielhollas · 2025-05-08T16:38:29Z

I think we first merge this PR and then rebase the other

Yes, I think merging this first makes sense since clearly mypy got better at inferring some types. I also have three typing PRs in progress, #6833 #6707 and #6704, which I will rebase once this is merged.

.pre-commit-config.yaml

danielhollas · 2025-06-03T00:11:21Z

src/aiida/cmdline/groups/dynamic.py

@@ -137,7 +137,7 @@ def apply_options(func):
            shared_options.reverse()

            for option in shared_options:
-                func = option(func)
+                func = option(func)  # type: ignore[operator]


I don't understand why this is needed all of a sudden, especially given that we have basically identical code couple lines above that apparently is fine. mypy error:

src/aiida/cmdline/groups/dynamic.py:140: error: "Option" not callable [operator]

It'll be easy to check if you have inlayhint in your IDE to inspect the type of options_list and shared_options.

Took me a while to understand the code, but in the end the mypy error was caused by a wrong type annotation of shared_options in the class constructor, see commit 05df184

danielhollas · 2025-06-03T00:12:54Z

src/aiida/orm/nodes/data/code/legacy.py

@@ -231,7 +231,7 @@ def get_code_helper(cls, label, machinename=None, backend=None):
            return result[0]

    @classmethod
-    def get(cls, pk=None, label=None, machinename=None):
+    def get(cls, pk=None, label=None, machinename=None):  # type: ignore[override]


I don't understand why mypy complains here:

src/aiida/orm/nodes/data/code/legacy.py:234: error: Signature of "get" incompatible with supertype "Entity" [override] def get(cls, pk=None, label=None, machinename=None): ^ src/aiida/orm/nodes/data/code/legacy.py:234: note: Superclass: src/aiida/orm/nodes/data/code/legacy.py:234: note: @classmethod src/aiida/orm/nodes/data/code/legacy.py:234: note: def get(cls, **kwargs: Any) -> Any src/aiida/orm/nodes/data/code/legacy.py:234: note: Subclass: src/aiida/orm/nodes/data/code/legacy.py:234: note: @classmethod src/aiida/orm/nodes/data/code/legacy.py:234: note: def get(cls, pk: Any = ..., label: Any = ..., machinename: Any = ...) -> Any

Since you asked, my answer is because we did bad design by keep on doing too much bad inheritance.
I'd just add an ignore, otherwise need to add an other function signature dispatch or change the signature of parent function which can cause unexpect things.

src/aiida/tools/archive/imports.py

danielhollas · 2025-06-25T13:57:40Z

@unkcpz can you take a look? Now that 2.7.0 is out it would be great to get this merged to unblock other typing PRs.

unkcpz

@danielhollas thanks. I give my go, just some minor requests.

unkcpz · 2025-07-01T07:36:23Z

src/aiida/cmdline/groups/dynamic.py

@@ -137,7 +137,7 @@ def apply_options(func):
            shared_options.reverse()

            for option in shared_options:
-                func = option(func)
+                func = option(func)  # type: ignore[operator]


It'll be easy to check if you have inlayhint in your IDE to inspect the type of options_list and shared_options.

unkcpz · 2025-07-01T07:39:13Z

src/aiida/engine/processes/functions.py

@@ -435,7 +439,7 @@ def define(cls, spec):
                    def indirect_default(value=default):
                        return to_aiida_type(value)
                else:
-                    indirect_default = default
+                    indirect_default = default  # type: ignore[assignment]


If it can be anything then as error message suggest use Callable[[Any], Any] ?

unkcpz · 2025-07-01T07:45:14Z

src/aiida/orm/nodes/data/code/installed.py

@@ -47,7 +47,7 @@ class Model(AbstractCode.Model):
            ...,
            title='Computer',
            description='The remote computer on which the executable resides.',
-            orm_to_model=lambda node, _: node.computer.label,
+            orm_to_model=lambda node, _: node.computer.label,  # type: ignore[attr-defined]


If we know what type of label or type of node.computer should be, in principle can use cast?

lambda node, _: cast(orm.Computer, node.computer).label?

It looks like the following works

Suggested change

orm_to_model=lambda node, _: node.computer.label, # type: ignore[attr-defined]

orm_to_model=lambda node, _: cast('InstalledCode', node).computer.label,

unkcpz · 2025-07-01T07:49:04Z

src/aiida/orm/nodes/data/code/legacy.py

@@ -231,7 +231,7 @@ def get_code_helper(cls, label, machinename=None, backend=None):
            return result[0]

    @classmethod
-    def get(cls, pk=None, label=None, machinename=None):
+    def get(cls, pk=None, label=None, machinename=None):  # type: ignore[override]


Since you asked, my answer is because we did bad design by keep on doing too much bad inheritance.
I'd just add an ignore, otherwise need to add an other function signature dispatch or change the signature of parent function which can cause unexpect things.

danielhollas

Thanks for the review @unkcpz ! I believe I addressed all your comments.

unkcpz

Thanks @danielhollas . Just one comment about the order of where to put@click.pass_context that you may overlooked? I'd suggest not touch the order, if you are not 100% confident that will not break anything.

unkcpz · 2025-07-03T15:28:20Z

src/aiida/cmdline/commands/cmd_profile.py

@@ -122,17 +122,17 @@ def profile_setup():
    """Set up a new profile."""


-@verdi_profile.command('configure-rabbitmq')  # type: ignore[arg-type]
+@verdi_profile.command('configure-rabbitmq')
+@click.pass_context


You forgot this one. Maybe not change the order, change the order of decorator change the behavior, correct?

unkcpz · 2025-07-03T15:31:01Z

src/aiida/engine/processes/functions.py

-        except Exception as exception:
-            LOGGER.warning(f'function `{func.__name__}` has a docstring that could not be parsed: {exception}')
-            param_help_string = {}
+        if func.__doc__ is None:


We could also log a warning if a calcfunction is missing a docstring, but that seems overly pedantic

Yes, I think that is a bit too much, I won't throw warning for that.

unkcpz · 2025-07-03T15:42:02Z

src/aiida/engine/processes/functions.py

@@ -435,7 +439,7 @@ def define(cls, spec):
                    def indirect_default(value=default):
                        return to_aiida_type(value)
                else:
-                    indirect_default = default
+                    indirect_default = default  # type: ignore[assignment]


Okay, I misunderstood the change here.

I think the mypy complain because the typechecker resolve the expression in if..else.. branch and want to make sure the type of expression conform with each other.

Then the correct way to solve this might be for the else branch do:

def indirect_default(value=default): return lambda _: value

(I am not sure this is correct, but I think it is better to just ignore the type error here to not touch the logic. so if you don't want to try please ignore my comment.)

unkcpz

@danielhollas thanks!

danielhollas · 2025-07-04T15:45:04Z

@unkcpz thanks for the thorough review!

@agoscinski I am planning to merge this once the tests are finished, since you gave this a green light already, but let me know if you have any concerns. (also let me know if you don't want to merge any PRs :-) but this one is mostly just typing so relatively low risk)

danielhollas requested review from unkcpz and agoscinski as code owners May 6, 2025 01:47

danielhollas commented May 6, 2025

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

danielhollas commented May 6, 2025

View reviewed changes

src/aiida/engine/processes/process.py Show resolved Hide resolved

agoscinski added this to aiida-core v2.7.1 May 6, 2025

agoscinski moved this to In review in aiida-core v2.7.1 May 6, 2025

danielhollas force-pushed the update-mypy branch from 6404b5b to f557493 Compare May 8, 2025 16:39

agoscinski added the topic/devops label May 21, 2025

danielhollas added the topic/typing label May 25, 2025

danielhollas force-pushed the update-mypy branch from f557493 to 732dd0f Compare June 2, 2025 23:24

danielhollas changed the title ~~Bump mypy to 1.15.0~~ Bump mypy to 1.16.0 Jun 3, 2025

danielhollas commented Jun 3, 2025

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

danielhollas commented Jun 3, 2025

View reviewed changes

src/aiida/tools/archive/imports.py Show resolved Hide resolved

danielhollas added 8 commits June 21, 2025 13:42

Bump mypy to 1.16.0

78f5521

mypy: use --pretty for nicer error messages!

f99dda6

More type:ignore removals!

6673061

Fix bug in existing archive import validation!

260d358

add new type:ignores

4454d04

One more strange type:ignore

7c9f776

Remove extra annotations

6ad39d5

Update mypy to 1.16.1

ee2e443

danielhollas force-pushed the update-mypy branch from 4db11f8 to ee2e443 Compare June 21, 2025 12:46

unkcpz requested changes Jul 1, 2025

View reviewed changes

danielhollas added 3 commits July 2, 2025 10:07

Use cast

a175d9d

Merge branch 'main' into update-mypy

ad8a3a7

Fix typing of shared_options in src/aiida/cmdline/groups/dynamic.py

05df184

danielhollas commented Jul 2, 2025

View reviewed changes

danielhollas requested a review from unkcpz July 2, 2025 10:46

unkcpz requested changes Jul 3, 2025

View reviewed changes

danielhollas added 2 commits July 4, 2025 11:43

Mark ctx positional-only in profile_configure_rabbitmq

59edc1d

Merge branch 'main' into update-mypy

f8b2464

unkcpz approved these changes Jul 4, 2025

View reviewed changes

Reduce diff

3734865

danielhollas merged commit 2106844 into aiidateam:main Jul 4, 2025
32 checks passed

github-project-automation bot moved this from In review to Done in aiida-core v2.7.1 Jul 4, 2025

danielhollas deleted the update-mypy branch July 4, 2025 17:00

	orm_to_model=lambda node, _: node.computer.label, # type: ignore[attr-defined]
	orm_to_model=lambda node, _: cast('InstalledCode', node).computer.label,

Bump mypy to 1.16.0 #6858

Bump mypy to 1.16.0 #6858

Uh oh!

Conversation

danielhollas commented May 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unkcpz Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

agoscinski commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agoscinski commented May 6, 2025

Uh oh!

danielhollas commented May 8, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielhollas commented Jun 25, 2025

Uh oh!

unkcpz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

unkcpz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unkcpz Jul 1, 2025 •

edited

Loading

codecov bot commented May 6, 2025 •

edited

Loading

agoscinski commented May 6, 2025 •

edited

Loading