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.

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

Bump mypy version to ~=1.13.0 #6630

Merged
merged 10 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ notebook = [
]
pre-commit = [
'aiida-core[atomic_tools,rest,tests,tui]',
'mypy~=1.10.0',
'mypy~=1.13.0',
'packaging~=23.0',
'pre-commit~=3.5',
'sqlalchemy[mypy]~=2.0',
Expand Down
2 changes: 1 addition & 1 deletion src/aiida/common/lang.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,4 @@ def __init__(self, getter: Callable[[SelfType], ReturnType]) -> None:
self.getter = getter

def __get__(self, instance: Any, owner: SelfType) -> ReturnType:
return self.getter(owner)
return self.getter(owner) # type: ignore[arg-type]
2 changes: 1 addition & 1 deletion src/aiida/common/pydantic.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice fix. cc @edan-bainglass for visibility

field_info.metadata.append({key: value})

return field_info
2 changes: 1 addition & 1 deletion src/aiida/engine/processes/calcjobs/calcjob.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Would try to solve this in a subsequent PR

raise TypeError(f' {obj!r} is not JSON serializable')

subfolder = folder.get_subfolder('.aiida', create=True)
Expand Down
2 changes: 1 addition & 1 deletion src/aiida/engine/processes/workchains/restart.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@


def validate_handler_overrides(
process_class: 'BaseRestartWorkChain', handler_overrides: Optional[orm.Dict], ctx: 'PortNamespace'
process_class: type['BaseRestartWorkChain'], handler_overrides: Optional[orm.Dict], ctx: 'PortNamespace'
danielhollas marked this conversation as resolved.
Show resolved Hide resolved
) -> Optional[str]:
"""Validator for the ``handler_overrides`` input port of the ``BaseRestartWorkChain``.

Expand Down
2 changes: 1 addition & 1 deletion src/aiida/manage/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ def create_runner(self, with_persistence: bool = True, **kwargs: Any) -> 'Runner
if with_persistence and 'persister' not in settings:
settings['persister'] = self.get_persister()

return runners.Runner(**settings)
return runners.Runner(**settings) # type: ignore[arg-type]

def create_daemon_runner(self, loop: Optional['asyncio.AbstractEventLoop'] = None) -> 'Runner':
"""Create and return a new daemon runner.
Expand Down
2 changes: 1 addition & 1 deletion src/aiida/orm/nodes/data/array/projection.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@danielhollas danielhollas Nov 23, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@unkcpz unkcpz Nov 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

"""This method is inherited from OrbitalData, but is blocked here.
If used will raise a NotImplementedError
"""
Expand Down
4 changes: 2 additions & 2 deletions src/aiida/orm/nodes/data/list.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,15 @@ def remove(self, value):
self.set_list(data)
return item

def pop(self, **kwargs):
def pop(self, **kwargs): # type: ignore[override]
"""Remove and return item at index (default last)."""
data = self.get_list()
item = data.pop(**kwargs)
if not self._using_list_reference():
self.set_list(data)
return item

def index(self, value):
def index(self, value): # type: ignore[override]
"""Return first index of value.."""
return self.get_list().index(value)

Expand Down
4 changes: 1 addition & 3 deletions src/aiida/orm/nodes/data/singlefile.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@ def open(self, path: FilePath, mode: t.Literal['rb']) -> t.Iterator[t.BinaryIO]:

@t.overload
@contextlib.contextmanager
def open( # type: ignore[overload-overlap]
self, path: None = None, mode: t.Literal['r'] = ...
) -> t.Iterator[t.TextIO]: ...
def open(self, path: None = None, mode: t.Literal['r'] = ...) -> t.Iterator[t.TextIO]: ...

@t.overload
@contextlib.contextmanager
Expand Down
2 changes: 1 addition & 1 deletion src/aiida/plugins/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@danielhollas danielhollas Nov 25, 2024

Choose a reason for hiding this comment

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

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.

def get_version_info(self, plugin: str | t.Any) -> dict[t.Any, dict[t.Any, t.Any]]:
"""Get the version information for a given plugin.

.. note::
Expand Down
2 changes: 1 addition & 1 deletion src/aiida/tools/pytest_fixtures/daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@unkcpz unkcpz Nov 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@unkcpz unkcpz Nov 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, go ahead

unkcpz marked this conversation as resolved.
Show resolved Hide resolved
state: ProcessState = ProcessState.FINISHED,
timeout: int = 20,
**kwargs,
Expand Down
2 changes: 1 addition & 1 deletion tests/cmdline/groups/test_dynamic.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member Author

Choose a reason for hiding this comment

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

It is test, so I just ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

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



def test_list_options(entry_points):
Expand Down
Loading