Skip to content

Conversation

@f-schnabel
Copy link
Contributor

@f-schnabel f-schnabel commented Jan 9, 2023

Fixes #5835.

Description

Use from __future__ import annotations in all source files using annotations, so we have consistent usage (and not only in ~5 files).
The new annotations are also more readable e.g. Optional[Union[List[str], str]] vs. list[str] | str | None.

Secondly, this issue includes changing from typing import Callable, Sequence, ... to from collections.abc import Callable, Sequence, ... since the ones in the typing module are deprecated. They are interchangeable even for isinstance checks (see also this stackoverflow thread).

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.

@f-schnabel f-schnabel changed the title Consistent usage of annotation syntax [WIP] Consistent usage of annotation syntax Jan 9, 2023
@f-schnabel f-schnabel changed the title [WIP] Consistent usage of annotation syntax Consistent usage of annotation syntax Jan 9, 2023
@f-schnabel
Copy link
Contributor Author

There are some special cases, since these subscriptable types are only allowed as annotations but not inside casts. Therefore, cast(Iterable[str], x) fails with collections.abc.Iterable but works for typing.Iterable. For those cases I change it back to typing.Iterable.

@f-schnabel f-schnabel marked this pull request as ready for review January 9, 2023 22:51
@wyli
Copy link
Contributor

wyli commented Jan 9, 2023

thanks, could you please also review the pyupgrade configs
https://github.com/Project-MONAI/MONAI/blob/dev/.pre-commit-config.yaml#L33
maybe there are some options that could be enabled after this PR
https://github.com/asottile/pyupgrade#pep-585-typing-rewrites

@bhashemian
Copy link
Member

There are some special cases, since these subscriptable types are only allowed as annotations but not inside casts. Therefore, cast(Iterable[str], x) fails with collections.abc.Iterable but works for typing.Iterable. For those cases I change it back to typing.Iterable.

I see! I believe it is better to ignore those typings instead of keeping a deprecated type.

f-schnabel and others added 7 commits January 10, 2023 12:13
…ns`) and don't use deprecated typing aliases (instead use `collections.abc`)

Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@f-schnabel f-schnabel force-pushed the feature_use_new_annotation_syntax branch from 24a9196 to 9f4c805 Compare January 10, 2023 11:13
Copy link
Contributor Author

@f-schnabel f-schnabel left a comment

Choose a reason for hiding this comment

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

@wyli
Copy link
Contributor

wyli commented Jan 10, 2023

looks like it's feasible to ensure the "from __future__ import annotations" via isort config,

MONAI/setup.cfg

Line 157 in 6060a47

[isort]

could you please add this:

add_imports = ["from __future__ import annotations"]
append_only = true

f-schnabel and others added 3 commits January 10, 2023 18:00
@f-schnabel
Copy link
Contributor Author

looks like it's feasible to ensure the "from __future__ import annotations" via isort config,

MONAI/setup.cfg

Line 157 in 6060a47

[isort]

could you please add this:

add_imports = ["from __future__ import annotations"]
append_only = true

This now also adds the imports to __init__ and test files.
Should I try to exclude them from isort? But then no import sorting would be happening to those files...

@wyli
Copy link
Contributor

wyli commented Jan 10, 2023

/black
thanks, I think it's good as long as the tools are configured to always enforce the style consistently

Signed-off-by: monai-bot <monai.miccai2019@gmail.com>
@wyli wyli enabled auto-merge (squash) January 10, 2023 17:30
@wyli
Copy link
Contributor

wyli commented Jan 10, 2023

/build

@wyli wyli merged commit fdd07f3 into Project-MONAI:dev Jan 10, 2023
@bhashemian
Copy link
Member

I think we still need the deprecated types here, since the new ones only work inside annotations, but this is creating a type alias. https://github.com/Shadow-Devil/MONAI/blob/9f4c8059491af22c7d89f02f8d8ed804206868e3/monai/transforms/spatial/array.py#L111

Also here: https://github.com/Shadow-Devil/MONAI/blob/feature_use_new_annotation_syntax/monai/config/type_definitions.py

We have until 2025 to update this!😉 This behavior is accepted in 3.10 where we can define numeric = int | float for instance. Maybe by 2025 we can drop support for <3.10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consistent usage of annotation syntax

4 participants