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

Bug: Revert changes that impacts v3 #5029

Closed
leandrodamascena opened this issue Aug 20, 2024 · 7 comments · Fixed by #5088
Closed

Bug: Revert changes that impacts v3 #5029

leandrodamascena opened this issue Aug 20, 2024 · 7 comments · Fixed by #5088
Assignees
Labels
bug Something isn't working v3 Features that will be included in Powertools v3.

Comments

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Aug 20, 2024

Expected Behaviour

Some utilities should work without any extra dependency.

Current Behaviour

The V3 branch does not yet have nox installed to perform dependency testing and some of our changes have created some unexpected cross-dependency breaking changes.

Here is the list of breaking changes detected so far:

1 - We can't import BaseModel outside of TYPE_CHECKING because it will force to bring Pydantic at Runtime level even not using data_validation - https://github.com/aws-powertools/powertools-lambda-python/blob/v3/aws_lambda_powertools/event_handler/openapi/types.py#L7

2 - We can't import ConfigDict because we will force to bring Pydantic event not using data_validation - https://github.com/aws-powertools/powertools-lambda-python/blob/v3/aws_lambda_powertools/event_handler/openapi/constants.py#L1

3 - We need to restore get_header.. and get_querystring.. functions removed by this PR - https://github.com/aws-powertools/powertools-lambda-python/pull/4606/files

4 - We need to add more e2e tests.

Code snippet

No code snippet

Possible Solution

No response

Steps to Reproduce

Install and run

Powertools for AWS Lambda (Python) version

v3

AWS Lambda function runtime

3.11

Packaging format used

PyPi

Debugging logs

No response

@leandrodamascena leandrodamascena added bug Something isn't working triage Pending triage from maintainers labels Aug 20, 2024
@leandrodamascena leandrodamascena self-assigned this Aug 20, 2024
@leandrodamascena leandrodamascena moved this from Triage to Working on it in Powertools for AWS Lambda (Python) Aug 20, 2024
@leandrodamascena leandrodamascena added v3 Features that will be included in Powertools v3. and removed triage Pending triage from maintainers labels Aug 20, 2024
@leandrodamascena leandrodamascena changed the title Bug: Revert changes that is impacting v3 Bug: Revert changes that impacts v3 Aug 21, 2024
@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Aug 27, 2024
@ericbn
Copy link
Contributor

ericbn commented Aug 28, 2024

@leandrodamascena, regarding point 1, as far as I can see, code has always been importing BaseModel outside TYPE_CHECKING. This makes sense, because otherwise it would break Pydantic.

Looks like for both points 1 and 2, Pydantic-specific code will need to be under some runtime check depending on each case. For example instead of:

        if isinstance(res, BaseModel):
            return _model_dump(
                res,
                by_alias=True,
                exclude_unset=exclude_unset,
                exclude_defaults=exclude_defaults,
                exclude_none=exclude_none,
            )

we could use a duck-typing-"Look Before You Leap" (LBYL) style like:

        if hasattr(res, "model_dump"):
            return _model_dump(
                res,
                by_alias=True,
                exclude_unset=exclude_unset,
                exclude_defaults=exclude_defaults,
                exclude_none=exclude_none,
            )

or an arguably preferred (because under the hood, hasattr is actually checking for an AttributeError exception being raised by getattr) "Easier to Ask Forgiveness Than Permission" (EAFP) style like:

        try:
            return _model_dump(
                res,
                by_alias=True,
                exclude_unset=exclude_unset,
                exclude_defaults=exclude_defaults,
                exclude_none=exclude_none,
            )
        except AttributeError:
            ...

I'd say it goes on depending on each code that should only optionally work with Pydantic, only if it's available.

@leandrodamascena
Copy link
Contributor Author

@leandrodamascena, regarding point 1, as far as I can see, code has always been importing BaseModel outside TYPE_CHECKING. This makes sense, because otherwise it would break Pydantic.

I don't think @ericbn! We are importing this inside the TYPE_CHECKING and using quotes to reference it and avoid forward reference.

https://github.com/aws-powertools/powertools-lambda-python/blob/develop/aws_lambda_powertools/event_handler/openapi/types.py#L8

if TYPE_CHECKING:
    from pydantic import BaseModel  # noqa: F401

CacheKey = Optional[Callable[..., Any]]
IncEx = Union[Set[int], Set[str], Dict[int, Any], Dict[str, Any]]
ModelNameMap = Dict[Union[Type["BaseModel"], Type[Enum]], str]
TypeModelOrEnum = Union[Type["BaseModel"], Type[Enum]]
UnionType = getattr(types, "UnionType", Union)

@ericbn
Copy link
Contributor

ericbn commented Aug 28, 2024

Oi @leandrodamascena. Sorry, I now realize there are at least 3 different scenarios and I was not properly distinguishing between them:

  1. pydantic imports used only for type checking: I was not aware of this. I see this was broken in commit 689072f (refactor(openapi): add from __future__ import annotations #4990) and you fixed it. My bad I've missed this before. I've double checked that this was the only regression introduced as part of the recent changes regarding Tech debt: Inconsistent use of from __future__ import annotations #4607, and I've created refactor(typing): move more types into TYPE_CHECKING #5088 as I was double checking this.
  2. pydantic optionally used only when the package is installed, that should not try to import the package otherwise: I'm not familiar with this scenario, but I see some logic under different checks like "pydantic" in sys.modules or PYDANTIC_V2.
  3. pydantic BaseModel classes being defined, which require the pydantic imports and the types used in the BaseModel definitions to be outside TYPE_CHECKING: We've tried to make sure we didn't break this as part of the changes regarding Tech debt: Inconsistent use of from __future__ import annotations #4607.

TL;DR My last comment above is a mess mixing up independent scenarios where pydantic is being used in different ways. :- )

@leandrodamascena
Copy link
Contributor Author

TL;DR My last comment above is a mess mixing up independent scenarios where pydantic is being used in different ways. :- )

This is completely fine and important because you define exactly how we use Pydantic in different scenarios. I appreciate that.

2. pydantic optionally used only when the package is installed, that should not try to import the package otherwise: I'm not familiar with this scenario, but I see some logic under different checks like "pydantic" in sys.modules or PYDANTIC_V2.

Wow, this made me search for PYDANTIC_V2 in the v3 branch and I found a reference that we should not have. I'll fix that. Thanks for bringing this to light 😄

@leandrodamascena leandrodamascena linked a pull request Aug 29, 2024 that will close this issue
7 tasks
@leandrodamascena
Copy link
Contributor Author

Closing this as completed.

@github-project-automation github-project-automation bot moved this from Working on it to Coming soon in Powertools for AWS Lambda (Python) Sep 9, 2024
Copy link
Contributor

github-actions bot commented Sep 9, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

Copy link
Contributor

This is now released under 3.0.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v3 Features that will be included in Powertools v3.
Projects
Status: Coming soon
Development

Successfully merging a pull request may close this issue.

2 participants