Skip to content

Consistent usage of annotation syntax#5836

Merged
wyli merged 11 commits intoProject-MONAI:devfrom
f-schnabel:feature_use_new_annotation_syntax
Jan 10, 2023
Merged

Consistent usage of annotation syntax#5836
wyli merged 11 commits intoProject-MONAI:devfrom
f-schnabel:feature_use_new_annotation_syntax

Conversation

@f-schnabel
Copy link
Copy Markdown
Contributor

@f-schnabel f-schnabel commented Jan 9, 2023 •
edited
Loading

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
Copy Markdown
Contributor Author

f-schnabel commented Jan 9, 2023

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
Copy Markdown
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
Copy Markdown
Member

bhashemian commented Jan 10, 2023

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
@f-schnabel
Use new syntax for annotations (via `from __future__ import annotatio…
0d9981d 
…ns`) and don't use deprecated typing aliases (instead use `collections.abc`)

Signed-off-by: Felix Schnabel <[email protected]>
@f-schnabel
Fix "TypeError: 'ABCMeta' object is not subscriptable"
8f15cf9 
Signed-off-by: Felix Schnabel <[email protected]>
@f-schnabel
Fix TypeError
8c3da63 
Signed-off-by: Felix Schnabel <[email protected]>
@f-schnabel
Formatting
88d484e 
Signed-off-by: Felix Schnabel <[email protected]>
@f-schnabel
Fix TypeErrors
2321d24 
Signed-off-by: Felix Schnabel <[email protected]>
@bhashemian @f-schnabel
Remove depracated Iterable with cast
3db198d 
Signed-off-by: Behrooz <[email protected]>
@pre-commit-ci @f-schnabel
[pre-commit.ci] auto fixes from pre-commit.com hooks
9f4c805 
for more information, see https://pre-commit.ci
@f-schnabel f-schnabel force-pushed the feature_use_new_annotation_syntax branch from 24a9196 to 9f4c805 Compare January 10, 2023 11:13
f-schnabel
f-schnabel commented Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@f-schnabel f-schnabel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

f-schnabel
f-schnabel commented Jan 10, 2023
View reviewed changes
@wyli
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor Author

f-schnabel 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

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
Copy Markdown
Contributor

wyli commented Jan 10, 2023 •
edited
Loading

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

@monai-bot
[MONAI] code formatting
b08bdc9 
Signed-off-by: monai-bot <[email protected]>
@wyli wyli enabled auto-merge (squash) January 10, 2023 17:30
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Jan 10, 2023

/build

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

bhashemian commented Jan 10, 2023

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.

@f-schnabel f-schnabel deleted the feature_use_new_annotation_syntax branch January 23, 2023 12:45
@Warvito Warvito mentioned this pull request May 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consistent usage of annotation syntax

4 participants

@f-schnabel @wyli @bhashemian @monai-bot