Run typing on orm/{comments,computers}.py and orm/implementation/storage_backend.py #6704

danielhollas · 2025-01-14T15:03:18Z

Run typing on orm/comments.py, orm/computers.py and orm/implementation/storage_backend.py. These files were mostly typed already, but not checked by mypy.

~~Depends on #6703~~

codecov · 2025-01-14T15:20:48Z

Codecov Report

❌ Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.27%. Comparing base (ebc9d4a) to head (8e5f4d3).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/orm/comments.py	80.00%	1 Missing ⚠️
src/aiida/orm/computers.py	87.50%	1 Missing ⚠️
src/aiida/orm/entities.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6704      +/-   ##
==========================================
- Coverage   79.27%   79.27%   -0.00%     
==========================================
  Files         566      566              
  Lines       43779    43782       +3     
==========================================
+ Hits        34700    34702       +2     
- Misses       9079     9080       +1

☔ 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.

danielhollas · 2025-01-14T16:15:58Z

src/aiida/orm/computers.py

    def list_labels(self) -> List[str]:
        """Return a list with all the labels of the computers in the DB."""
-        return self._backend.computers.list_names()
+        return self._backend.computers.list_names()  # type: ignore[attr-defined]


I am not sure why mypy complains here

src/aiida/orm/computers.py:57: error: "BackendComputerCollection" has no attribute "list_names" [attr-defined]

danielhollas · 2025-01-14T16:17:30Z

src/aiida/orm/computers.py

        checking for a valid scheduler.
        """
-        if not isinstance(mpirun_cmd, (tuple, list)) or not all(isinstance(i, str) for i in mpirun_cmd):
+        if not isinstance(mpirun_cmd, (tuple, list)) or not all(isinstance(i, str) for i in mpirun_cmd):  # type: ignore[redundant-expr]


Mypy complains that the isinstance check should be always true since the mpirun_cmd is typed at list or tuple, but we obviously want to preserve the runtime check since we don't control all the callers.

Or I think the better change would be we move the mpirun_cmd runtime check when set up the computer? But it seems out the scope of this PR.

This check is in the _mpirun_command_validator which is called by the validate function which is in turn called before storing the Computer instance in DB (in function store()). So I think the code here is in the correct place.

src/aiida/orm/implementation/storage_backend.py

src/aiida/transports/transport.py

src/aiida/orm/comments.py

unkcpz · 2025-01-14T16:42:52Z

would be good to get reviewed by someone familiar with the orm implementation.

Then my review will not count ;p

Just out of curious, the goal of the PR is to add more mypy checking to code base (I am totally in it, thanks! I was thinking maybe we can even have it as a project for this year's GSoC to find someone can working on it ) or you are targeting to bring some new features that may benefit from adding type?

danielhollas · 2025-01-14T16:45:23Z

Then my review will not count ;p

Haha, fair. I am mostly confused about this code:

class Comment(entities.Entity['BackendComment', CommentCollection]):

Mypy complains about the missing BackendComment type, but when I import it, I get an error from ruff that the import is unused. I think it might be a limitation of ruff so I just added a noqa comment.

danielhollas · 2025-01-14T16:47:31Z

Just out of curious, the goal of the PR is to add more mypy checking to code base (I am totally in it, thanks! I was thinking maybe we can even have it as a project for this year's GSoC to find someone can working on it ) or you are targeting to bring some new features that may benefit from adding type?

Just adding type checking. These files just seemed like low-hanging fruit, they were already mostly typed!

What I really don't like is that we have a list of ignored types in pre-commit config file. It's very easy to miss, and to mess with types in files that are not even checked.

I think it would be better to have type: ignore comments at the top of the ignored files, which makes it super clear that they are not checked.

EDIT: What originally prompted me was the other PR which add checking for the orm/caching file since I was looking at it while working on aiida-test-cache.

unkcpz · 2025-01-14T17:10:12Z

Mypy complains about the missing BackendComment type, but when I import it, I get an error from ruff that the import is unused.

How you import the BackendComment? Since in the typing annotation, you use single-quotes you need to import:

if typing.TYPE_CHECKING:
    from aiida.xxx import BackendComment

unkcpz · 2025-01-14T17:21:42Z

I think it would be better to have type: ignore comments at the top of the ignored files, which makes it super clear that they are not checked.

I agree, but it is not easy to make typing correct in python. No actually compiler will help us (yes, I am comparing it with Rust 🙈 ). But I am also trying to bring typing to engine part of the code base module by module (like this one #6641).

danielhollas · 2025-01-14T17:33:39Z

How you import the BackendComment? Since in the typing annotation, you use single-quotes you need to import:

Yes, that's what I am doing in this PR.

unkcpz

I gave my go on this PR, but it would be nice that someone more familiar with orm can give it a look. I pin @edan-bainglass because your name is in the storage part of team responsibilities.

src/aiida/orm/computers.py

src/aiida/orm/comments.py

danielhollas · 2025-05-06T02:46:30Z

@edan-bainglass after the pydantic PR was merged, I needed to add a bunch of extra type: ignore comments here. Could you please take a look and see if they can be solved (and that they don't point out real bugs)?

c3bca2c

This reverts commit 88c1a11.

src/aiida/orm/comments.py

edan-bainglass · 2025-09-29T04:44:23Z

src/aiida/orm/comments.py

            is_attribute=False,
            orm_class='core.node',
-            orm_to_model=lambda comment, _: comment.node.pk,
+            orm_to_model=lambda comment, _: cast('Comment', comment).node.pk,


For context if unclear, orm_to_model is used when converting an ORM object to its model via .to_model(). The model should be kept serializable, so AiiDA entities are often converted to their pk value. In this case, that would be the pk of the Node associated with the Comment instance of this Model. The Model does not have a reference of its owning Comment instance, so a validator cannot be used. Hence (I presume), it motivates the custom orm_to_model feature.

The lambda function doesn't know what comment is (or whatever the entity is in any use), hence the need for casting.

Regarding the second argument (which is, I believe, ignored in general), we pass a repository path. I vaguely recall @agoscinski implementing this. If so, @agoscinski could you please briefly explain why, or reference to an existing explanation? 🙏

@edan-bainglass thank you for the context, that is helpful. 🙏

edan-bainglass · 2025-09-29T05:11:43Z

src/aiida/orm/implementation/comments.py

    def create(  # type: ignore[override]
-        self, node: 'BackendNode', user: 'BackendUser', content: Optional[str] = None, **kwargs
-    ):
+        self, node: 'BackendNode', user: 'BackendUser', content: Optional[str] = None, **kwargs: Any
+    ) -> BackendComment:


Technically breaks Liskov substitution principle.

One solution would be

@abc.abstractmethod def create(self, node: 'BackendNode' | None = None, user: 'BackendUser' | None = None, content: Optional[str] = None, **kwargs: Any ) -> BackendComment: ...

and explicitly check that node and user exist, complaining otherwise. It will appease Liskov (no required kwargs, as stated in the abstract class), and mypy by extension (hopefully). But it may confuse the API user - we state they are optional... they are not!

@edan-bainglass yeah thanks for pointing this out. I think I prefer to ignore the mypy error here instead of complicating the function signature (at least for this PR). Perhaps a more holistic fix is needed for this.

btw: Here's the mypy error

src/aiida/orm/implementation/comments.py:79: error: Signature of "create" incompatible with supertype "aiida.orm.implementation.entities.BackendCollection" [override] src/aiida/orm/implementation/comments.py:79: note: Superclass: src/aiida/orm/implementation/comments.py:79: note: def create(self, **kwargs: Any) -> BackendComment src/aiida/orm/implementation/comments.py:79: note: Subclass: src/aiida/orm/implementation/comments.py:79: note: def create(self, node: BackendNode, user: BackendUser, content: str | None = ..., **kwargs: Any) -> BackendComment

Yep, that's an LSP violation. We can have a look at a "better" solution later as you say

edan-bainglass · 2025-09-29T08:25:59Z

src/aiida/orm/entities.py


    _CLS_COLLECTION: Type[CollectionType] = Collection  # type: ignore[assignment]
-    _logger = log.AIIDA_LOGGER.getChild('orm.entities')
+    _logger: Logger = log.AIIDA_LOGGER.getChild('orm.entities')


Don't you need to do 'Logger' if imported under TYPE_CHECKING?

There is from __future__ import annotations at the top so all types are implicitly converted to strings.

I think perhaps we are inconsistent about this throughout the code. Likely, from __future__ import annotations is added to a module at some point but 'SomeType' is never reduced to SomeType. Maybe ruff can auto-fix?

Relevant ruff issue:
astral-sh/ruff#18502

Thanks. Regarding the inconsistency, I think already in this PR they exist - see above change to comments.py (the casting in orm_to_model lambdas).

edan-bainglass

Added a few comments. In general, I'm happy with this, but maybe we can have a call to understand the final goal (the typing mission), and order of operations 🙂

danielhollas · 2025-09-29T12:08:00Z

@edan-bainglass thank you for the review. I think I addresses all your comments, PTAL.

but maybe we can have a call to understand the final goal (the typing mission), and order of operations

Sure, maybe we can talk on Thursdays aiida-core meeting (or after?).

tl;dr on my side, I was adding typing to aiida-core just as a side-project for me to learn more about Python typing system. Besides merging the existing PRs I don't have any immediate plans to work on this further in the near future (this has already consumed too much time I should have been spending elsewhere 😅)

FWIW: I think the biggest priority (and what I have been doing) is the fact that many files are skipped altogether from type checking (see the list in .pre-commit-config.yaml) which compromises the typing of the whole codebase and is super confusing since many of the skipped files actually contains some type annotations, which may therefore be wrong since they are not checked).

edan-bainglass · 2025-09-29T12:44:34Z

@edan-bainglass thank you for the review. I think I addresses all your comments, PTAL.

but maybe we can have a call to understand the final goal (the typing mission), and order of operations

Sure, maybe we can talk on Thursdays aiida-core meeting (or after?).

Let's do that, just to sync up.

tl;dr on my side, I was adding typing to aiida-core just as a side-project for me to learn more about Python typing system. Besides merging the existing PRs I don't have any immediate plans to work on this further in the near future (this has already consumed too much time I should have been spending elsewhere 😅)

No worries. I will pick it up, but likely limited to the ORM.

FWIW: I think the biggest priority (and what I have been doing) is the fact that many files are skipped altogether from type checking (see the list in .pre-commit-config.yaml) which compromises the typing of the whole codebase and is super confusing since many of the skipped files actually contains some type annotations, which may therefore be wrong since they are not checked).

Indeed, needs to be addressed.

edan-bainglass

Thanks @danielhollas for the replies. Good on my end. Let's go!

danielhollas force-pushed the comments-typing branch from cf1cf48 to 8d0fa45 Compare January 14, 2025 16:13

danielhollas marked this pull request as ready for review January 14, 2025 16:13

danielhollas commented Jan 14, 2025

View reviewed changes

src/aiida/orm/implementation/storage_backend.py Show resolved Hide resolved

danielhollas commented Jan 14, 2025

View reviewed changes

src/aiida/transports/transport.py Show resolved Hide resolved

danielhollas commented Jan 14, 2025

View reviewed changes

src/aiida/orm/comments.py Outdated Show resolved Hide resolved

danielhollas requested a review from unkcpz January 14, 2025 16:47

danielhollas mentioned this pull request Jan 14, 2025

Add typing to aiida/common/{utils,extendeddicts}.py #6706

Merged

danielhollas force-pushed the comments-typing branch 2 times, most recently from 9bbfb06 to 042e513 Compare January 16, 2025 12:35

unkcpz reviewed Jan 16, 2025

View reviewed changes

danielhollas force-pushed the comments-typing branch from 042e513 to 21db8da Compare January 22, 2025 02:45

danielhollas requested a review from edan-bainglass February 28, 2025 13:20

danielhollas mentioned this pull request Apr 23, 2025

Adding Type Annotation #6810

Closed

danielhollas force-pushed the comments-typing branch from f70d2a6 to c3bca2c Compare May 1, 2025 08:55

danielhollas requested a review from agoscinski as a code owner May 6, 2025 02:25

danielhollas commented May 6, 2025

View reviewed changes

src/aiida/orm/computers.py Outdated Show resolved Hide resolved

danielhollas commented May 6, 2025

View reviewed changes

src/aiida/orm/comments.py Outdated Show resolved Hide resolved

danielhollas mentioned this pull request May 8, 2025

Bump mypy to 1.16.0 #6858

Merged

danielhollas added the topic/typing label May 25, 2025

danielhollas force-pushed the comments-typing branch from 0820342 to 3b39b38 Compare June 21, 2025 13:27

danielhollas added 14 commits September 28, 2025 19:56

Run typing on orm/implementation/storage_backend.py

dccb820

Silence new mypy errors

6855c26

Remove unused type ignore

85d03e4

cast

eaac429

Remove ignore

d342c58

Use builtins for typing

3b62633

Remove incorrect return type

8ef26f6

Fix regression test

610fc27

Try fixing Comment.set_user

ecb2585

Revert "Use builtins for typing"

d82191c

This reverts commit 88c1a11.

Add list_names abstract method to BackendComputerCollection

1316800

More accurate type

93921e4

More types + clever overload

295f5a4

Try fixing the new test

ffd23ec

danielhollas force-pushed the comments-typing branch from c25d975 to ffd23ec Compare September 28, 2025 18:56

Mark test as failing

c89431b

danielhollas marked this pull request as ready for review September 28, 2025 19:01

danielhollas commented Sep 28, 2025

View reviewed changes

src/aiida/orm/comments.py Outdated Show resolved Hide resolved

danielhollas requested a review from unkcpz September 28, 2025 19:27

edan-bainglass reviewed Sep 29, 2025

View reviewed changes

danielhollas added 2 commits September 29, 2025 12:46

Remove overloads for get_or_create

392b513

Merge branch 'main' into comments-typing

8e5f4d3

edan-bainglass approved these changes Sep 29, 2025

View reviewed changes

danielhollas merged commit f6cdb91 into aiidateam:main Sep 29, 2025
16 checks passed

danielhollas deleted the comments-typing branch September 29, 2025 13:33

Run typing on orm/{comments,computers}.py and orm/implementation/storage_backend.py #6704

Run typing on orm/{comments,computers}.py and orm/implementation/storage_backend.py #6704

Uh oh!

Conversation

danielhollas commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

danielhollas commented Jan 14, 2025

Uh oh!

danielhollas commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unkcpz commented Jan 14, 2025

Uh oh!

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

Uh oh!

Uh oh!

danielhollas commented Jan 14, 2025

Uh oh!

unkcpz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danielhollas commented May 6, 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!

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!

edan-bainglass Sep 30, 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!

edan-bainglass left a comment

Choose a reason for hiding this comment

Uh oh!

danielhollas commented Sep 29, 2025

Uh oh!

danielhollas commented Jan 14, 2025 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

unkcpz commented Jan 14, 2025 •

edited

Loading

danielhollas commented Jan 14, 2025 •

edited

Loading

unkcpz commented Jan 14, 2025 •

edited

Loading

edan-bainglass Sep 30, 2025 •

edited

Loading