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

Tech debt: Inconsistent use of from __future__ import annotations #4607

Open
1 of 2 tasks
ericbn opened this issue Jun 22, 2024 · 9 comments
Open
1 of 2 tasks

Tech debt: Inconsistent use of from __future__ import annotations #4607

ericbn opened this issue Jun 22, 2024 · 9 comments
Labels
help wanted Could use a second pair of eyes/hands tech-debt Technical Debt tasks typing Static typing definition related issues (mypy, pyright, etc.)

Comments

@ericbn
Copy link
Contributor

ericbn commented Jun 22, 2024

Why is this needed?

Source files that have from __future__ import annotations are not taking full advantage of the import. Namely, this import enables PEP 563 (Postponed Evaluation of Annotations), which allows:

  1. usage of if typing.TYPE_CHECKING: for types to be used only by the type checker and no need to quote type names (unless being used in forward references)
  2. use of PEP 585 (Type Hinting Generics In Standard Collections) in Python < 3.9
  3. use of PEP 604 (Allow writing union types as X | Y) in Python < 3.10

Looking at the files with this import we see that not all of them have if typing.TYPE_CHECKING:, which could have been the initial motivation to add the import, and they have mixed usages of PEP 585 and PEP 604.

Some other files do have if typing.TYPE_CHECKING: without the import and are quoting the type names instead.

Also, I see some if typing.TYPE_CHECKING: is being used for the types defined in the mypy_boto3_* packages, which are dev dependencies and in these cases this makes sense.

Which area does this relate to?

Static typing

Suggestion

We already have a constraint:

  1. The import of types from the mypy_boto3_* packages must be inside if typing.TYPE_CHECKING:.

And here are suggestions (see https://stackoverflow.com/a/53455562/2654518):

  1. Use from __future__ import annotations instead of quoting types imported inside if typing.TYPE_CHECKING:.
  2. Using both from __future__ import annotations and if typing.TYPE_CHECKING: can improve the startup performance since less packages are imported during runtime. Although optimizing performance really only makes sense if we have benchmarks first.

Also:

  1. The from __future__ import annotations enables the use of PEP 585 and PEP 604 in Python < 3.10.

Looks like the current code does not have a clear direction on how to follow these suggestions. I'd say it's more consistent and cleaner to have a guideline. So, here are some decisions that could be made:

  1. Do we use from __future__ import annotations instead of quoting types that have been imported inside if typing.TYPE_CHECKING:? I'd say YES since we're already using the import in some files.
  2. Do we use from __future__ import annotations and if typing.TYPE_CHECKING: in all files?
    1. Even if we choose YES, should we at least skip the example files?
  3. Do we use PEP 585 and PEP 604 in files using from __future__ import annotations? I'd say definitely YES if we choose YES for question 2.

Acknowledgment

@ericbn ericbn added tech-debt Technical Debt tasks triage Pending triage from maintainers labels Jun 22, 2024
@heitorlessa
Copy link
Contributor

THANK YOU @ericbn for taking the time, this has been a pet peeve of mine for a long while but we never had the bandwidth to kick off a discussion to tackle it. Most was added before the Python's addition of being able to use list over List, hence why the inconsistency -- we could enable this linting rule to have a quick plan of action.

The two issues I remember clearly were:

  1. Forward reference having to be resolved to extract details in metaprogramming wise. I think it's a non-issue nowadays with Pydantic resolving them, and this is pretty much internal codebase rather than customer unresolved type reference.
  2. Certain expressions like a new type / type alias didn't support PEP 585 (exception rather than the role)

Adding help wanted as we're dealing with v3 now with multi-version Python Lambda Layer (compiled deps).

My thoughts on tackling this as of now:

  • Enforce non-PEP585 ruff rule to catch areas to fix
  • Group PR per feature to ease review and limit blast radius (e.g., idempotency, event_handler)
  • Skip examples file for now as there's a major feature we're missing from mkdocs-material - this would take weeks/months of efforts otherwise
  • Go with your 1st suggestion, fallback to 2 if we lose typing on dev dependencies like mypy_boto3 - VSCode or PyCharm resolving would be enough for us.

@heitorlessa heitorlessa added typing Static typing definition related issues (mypy, pyright, etc.) help wanted Could use a second pair of eyes/hands and removed triage Pending triage from maintainers labels Jun 26, 2024
@ericbn
Copy link
Contributor Author

ericbn commented Jun 26, 2024

Oi @heitorlessa.

I've edited the description of the ticket a few times before you've posted your comment. Sorry for the confusion. I'm not sure exactly what you mean by "Go with your 1st suggestion". I'm assuming it's "Only keep from __future__ import annotations in files where if typing.TYPE_CHECKING: is being used" from the original description. In the new description I've broken the decisions into 4 questions (1, 2, 2i, 3) :

  1. Do we use from __future__ import annotations instead of quoting types that have been imported inside if typing.TYPE_CHECKING:? I'd say YES since we're already using the import in some files.
  2. Do we use from __future__ import annotations and if typing.TYPE_CHECKING: in all files?
    i. Even if we choose YES, should we at least skip the example files?
  3. Do we use PEP 585 and PEP 604 in files using from __future__ import annotations? I'd say definitely YES if we choose YES for question 2.

I see you mean "YES" for 2i.

Removing unneeded quoted type annotations can be reported by ruff's quoted-annotation (UP037), hopefully regardless of the presence of from __future__ import annotations.

Is your overall goal to keep the the occurrences of from __future__ import annotations to a minimum?

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Jun 27, 2024

I have another point here that is not directly the problem described here, but is affected by it. We don't have a standard when importing types, sometimes we import from types, sometimes from https://github.com/aws-powertools/powertools-lambda-python/blob/develop/aws_lambda_powertools/shared/types.py and because of this we need to add from __future__ import annotations or we may need to check the Python version inside the files to see if the type is supported.

We can take this opportunity to improve the use of from __future__ import annotations, but also create a standard of every type must be imported from https://github.com/aws-powertools/powertools-lambda-python/blob/develop/aws_lambda_powertools/shared/types.py.

I would like to hear what do you think.

@heitorlessa
Copy link
Contributor

hey @ericbn @leandrodamascena thanks for patience!! I think we've got several subjects here and the formatting isn't helping, causing confusion -- I broke into categories so it's easier to see if we're missing or if we vehemently disagree on one of them.

Let me know if I missed anything, and whether you agree with the direction (then any implementation help is appreciated!).

Summary

Area Example Decision
Lazy type evaluation (source code) from __future__ import annotations Yes, everywhere
Lazy type evaluation (examples) from __future__ import annotations Yes, everywhere
Forward reference types "BaseModel" Needs testing; generic names like Client may not be resolved properly
Backport new typing features from typing_extensions import ParamSpec Yes, everywhere; use typing_extensions over if py version.

Lazy type evaluation in source code

We should use from __future__ import annotations until Python 3.9 is deprecated (2025 Oct).

As @ericbn called out, this will help us reduce typing imports and where possible get cleaner type annotations like str | int | None over Optional[Union[str, int]].

Lazy type evaluation in examples

Since we don't know what Python versions our customers will use (there's plenty still on Py3.7/3.8), it's easier to add from __future__ import annotations consistently across all examples. It helps everyone.

Dev type checking

For types we control and less ambiguous type names, we should use forward references and get rid of if TYPE_CHECKING. This is only possible with from __future__ import annotations.

Example: [Event Handler](https://github.com/aws-powertools/powertools-lambda-python/blob/be7a4cc653e0893815cc50cf8d82f855702c33a5/, aws_lambda_powertools/event_handler/api_gateway.py#L81)

However, for types we do not control that have ambiguous type name, we need to test it. Dependencies like mypy_boto3 might have more than one Client, or Table might get resolved to another dev dependency installed. If we can't confidently test, I'd suggest we keep mypy_boto3 as the first exception to the rule in keeping if TYPE_CHECKING`.

Example: Table type, Mypy Client vs 3P Client

Typing new features backport

We should use from typing_extensions import <Type> for any type that might start by Python version X. Since we add typing_extensions as a required dependency, we can get rid of types.py and any branching logic to import a given type to an older Python version.

Typing extensions handle the case where a type is already supported in a given Python version and return from typing import <X> instead, so we don't have to -- ParamSpec example in 3.10+.

@ericbn
Copy link
Contributor Author

ericbn commented Jul 1, 2024

Hey @heitorlessa.

Makes perfect sense and good callout on using typing_extensions instead of a custom solution.

What do you think about the following plan to tackle this?

  1. Review the use of mypy_boto3_* types, and try to keep their imports to a minimum, to only when those types cannot be discovered automatically by mypy.
  2. Remove usages of aws_lambda_powertools.shared.types imports and replace them by either types from the standard library that are already compatible with Python >= 3.8 or types from typing_extensions. Remove the aws_lambda_powertools.shared.types package too.
  3. Add from __future__ import annotations to all files, including examples.
  4. Enable all ruff TCH rules and fix all reported issues. These will detect all typing-only imports that can be moved to a if TYPE_CHECKING blocks. Expect a significant number of imports to fall into this category specially after adding from __future__ import annotations to the files.
  5. Enable all ruff UP rules and fix all reported issues. We're particularly interested in
    UP006 (non-pep585-annotation, UP007 (non-pep604-annotation) and UP037 (quoted-annotation), but I see there are other small changes other rules are reporting too that we can fix, like UP004 (useless-object-inheritance), UP008 (super-call-with-parameters) and UP009 (utf8-encoding-declaration).
  6. Re-enable the FA100 ruff rule. This should not yield any new issues after the above has been done.

I think 1 and 2 can be done in one PR each first. Then the remaining steps can be done after those and #4606 are merged, with separate PRs per feature as proposed before.

@heitorlessa
Copy link
Contributor

looks great, one minor change to avoid breaking changes.

Remove usages of aws_lambda_powertools.shared.types imports and replace them by either types from the standard library that are already compatible with Python >= 3.8 or types from typing_extensions. Remove the aws_lambda_powertools.shared.types package too.

this ^ is fine if we target v3 branch that @leandrodamascena and @sthulb are working on. Otherwise, we should cautiously keep aws_lambda_powertools.shared.types file, and replace all types with typing_extensions. Often times we don't know what customers took a dependency on, despite the warnings -- a major version gives us more flexibility.

Exciting!!!!

@ericbn
Copy link
Contributor Author

ericbn commented Jul 1, 2024

Oh, I was planning all this for v3. Should have mentioned that. I can start working on it if you agree.

@heitorlessa
Copy link
Contributor

heitorlessa commented Jul 1, 2024 via email

@ericbn
Copy link
Contributor Author

ericbn commented Jul 10, 2024

I've opened the PR #4692 regarding step 1 of the plan above.

  1. Review the use of mypy_boto3_* types, and try to keep their imports to a minimum, to only when those types cannot be discovered automatically by mypy.

This task ended up having a different outcome that I initially though -- the code didn't have too many unnecessary hardcoded mypy_boto3_* types, which I've removed in the PR, but it was not taking advantage of implicit type annotations. I've enabled that, and it allowed mypy to report a few errors in the code or examples, which I've fixed. I've left one reported error there, as I'm not sure if it's a false-positive or need to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Could use a second pair of eyes/hands tech-debt Technical Debt tasks typing Static typing definition related issues (mypy, pyright, etc.)
Projects
Status: Backlog
Development

No branches or pull requests

3 participants