Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0ce39c
Run mypy on orm/{comments,computer}.py
danielhollas Jan 14, 2025
042e513
Run typing on orm/implementation/storage_backend.py
danielhollas Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,6 @@ repos:
src/aiida/manage/external/rmq/launcher.py|
src/aiida/manage/tests/main.py|
src/aiida/manage/tests/pytest_fixtures.py|
src/aiida/orm/comments.py|
src/aiida/orm/computers.py|
src/aiida/orm/implementation/storage_backend.py|
src/aiida/orm/nodes/comments.py|
src/aiida/orm/nodes/data/array/bands.py|
src/aiida/orm/nodes/data/array/trajectory.py|
src/aiida/orm/nodes/data/cif.py|
Expand Down
7 changes: 4 additions & 3 deletions src/aiida/orm/comments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

if TYPE_CHECKING:
from aiida.orm import Node, User
from aiida.orm.implementation import StorageBackend
from aiida.orm.implementation import BackendComment, BackendNode, StorageBackend # noqa: F401

Check warning on line 21 in src/aiida/orm/comments.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/comments.py#L21 

Added line #L21 was not covered by tests
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I don't import BackendComment I get the following error:

src/aiida/orm/comments.py:63: error: Name "BackendComment" is not defined  [name-defined]

From this code:

aiida-core/src/aiida/orm/comments.py

Line 63 in 199a027

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The noqa is needed because ruff complains about an unused import. I think this is a bug in ruff, I'll file an issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In seems a bit strange the type in the square of Entity is mixed, did you try entities.Entity['BackendComment', 'CommentCollection']? I remember my basedpyright gave something that can not use string and class for typing at same time. But doesn't matter, let's use noqa for the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just look at the type annotation, I am wondering if it can be just have a generic type to hold both backendComment and CommentCollection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is it even technically a type annotation? I don't know how the entities.Entity is even supposed to work, since this is somehow specifying the parent class for Comment.

I don't think I'd want to touch this in this PR in any case tbh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, this typing annotation is quite confuse. But let's keep as it is. We can come back and check all noqa. I'd say, the complex typing usually come from improper design, but it requires way more effort to touch some core parts of storage.


__all__ = ('Comment',)

Expand Down Expand Up @@ -146,15 +146,16 @@
return self._backend_entity.set_mtime(value)

@property
def node(self) -> 'Node':
def node(self) -> 'BackendNode':
return self._backend_entity.node

@property
def user(self) -> 'User':
return entities.from_backend_entity(users.User, self._backend_entity.user)

def set_user(self, value: 'User') -> None:
self._backend_entity.user = value.backend_entity
# ignoring mypy error: Property "user" defined in "BackendComment" is read-only
self._backend_entity.user = value.backend_entity # type: ignore[misc]

Check warning on line 158 in src/aiida/orm/comments.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/comments.py#L158 

Added line #L158 was not covered by tests

@property
def content(self) -> str:
Expand Down
12 changes: 6 additions & 6 deletions src/aiida/orm/computers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

if TYPE_CHECKING:
from aiida.orm import AuthInfo, User
from aiida.orm.implementation import StorageBackend
from aiida.orm.implementation import BackendComputer, StorageBackend # noqa: F401

Check warning on line 24 in src/aiida/orm/computers.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/computers.py#L24 

Added line #L24 was not covered by tests
from aiida.schedulers import Scheduler
from aiida.transports import Transport

Expand Down Expand Up @@ -54,7 +54,7 @@

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]

Check warning on line 57 in src/aiida/orm/computers.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/computers.py#L57 

Added line #L57 was not covered by tests
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why mypy complains here


def delete(self, pk: int) -> None:
"""Delete the computer with the given id"""
Expand Down Expand Up @@ -224,7 +224,7 @@
"""Validates the mpirun_command variable. MUST be called after properly
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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

raise exceptions.ValidationError('the mpirun_command must be a list of strings')

try:
Expand Down Expand Up @@ -278,7 +278,7 @@
if def_cpus_per_machine is None:
return

if not isinstance(def_cpus_per_machine, int) or def_cpus_per_machine <= 0:
if not isinstance(def_cpus_per_machine, int) or def_cpus_per_machine <= 0: # type: ignore[redundant-expr]

Check warning on line 281 in src/aiida/orm/computers.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/orm/computers.py#L281 

Added line #L281 was not covered by tests
raise exceptions.ValidationError(
'Invalid value for default_mpiprocs_per_machine, must be a positive integer, or an empty string if you '
'do not want to provide a default value.'
Expand All @@ -290,7 +290,7 @@
if def_memory_per_machine is None:
return

if not isinstance(def_memory_per_machine, int) or def_memory_per_machine <= 0:
if not isinstance(def_memory_per_machine, int) or def_memory_per_machine <= 0: # type: ignore[redundant-expr]
raise exceptions.ValidationError(
f'Invalid value for def_memory_per_machine, must be a positive int, got: {def_memory_per_machine}'
)
Expand Down Expand Up @@ -487,7 +487,7 @@
"""Set the mpirun command. It must be a list of strings (you can use
string.split() if you have a single, space-separated string).
"""
if not isinstance(val, (tuple, list)) or not all(isinstance(i, str) for i in val):
if not isinstance(val, (tuple, list)) or not all(isinstance(i, str) for i in val): # type: ignore[redundant-expr]
raise TypeError('the mpirun_command must be a list of strings')
self.set_property('mpirun_command', val)

Expand Down
2 changes: 1 addition & 1 deletion src/aiida/orm/implementation/storage_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ def get_orm_entities(self, detailed: bool = False) -> dict:
"""
from aiida.orm import Comment, Computer, Group, Log, Node, QueryBuilder, User

data = {}
data: dict[str, Any] = {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change I get these errors:

src/aiida/orm/implementation/storage_backend.py:465: error: Incompatible types in assignment (expression has type "list[Any]", target has type "int")  [assignment]
src/aiida/orm/implementation/storage_backend.py:470: error: Incompatible types in assignment (expression has type "list[Any]", target has type "int")  [assignment]
src/aiida/orm/implementation/storage_backend.py:478: error: Incompatible types in assignment (expression has type "list[Any]", target has type "int")  [assignment]
src/aiida/orm/implementation/storage_backend.py:486: error: Incompatible types in assignment (expression has type "list[Any]", target has type "int")  [assignment]
src/aiida/orm/implementation/storage_backend.py:491: error: Incompatible types in assignment (expression has type "list[Any]", target has type "int")  [assignment]

I don't know if there is a better solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do the same.


query_user = QueryBuilder(self).append(User, project=['email'])
data['Users'] = {'count': query_user.count()}
Expand Down
2 changes: 1 addition & 1 deletion src/aiida/transports/transport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class Transport(abc.ABC):
"""Abstract class for a generic transport (ssh, local, ...) contains the set of minimal methods."""

# This will be used for ``Computer.get_minimum_job_poll_interval``
DEFAULT_MINIMUM_JOB_POLL_INTERVAL = 10
DEFAULT_MINIMUM_JOB_POLL_INTERVAL = 10.0
unkcpz marked this conversation as resolved.
Show resolved Hide resolved

# This is used as a global default in case subclasses don't redefine this,
# but this should be redefined in plugins where appropriate
Expand Down
Loading