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

Maintenance: Remove email-validator dependency for v2 #1607

Closed
1 of 2 tasks
rubenfonseca opened this issue Oct 14, 2022 · 5 comments · Fixed by #1608
Closed
1 of 2 tasks

Maintenance: Remove email-validator dependency for v2 #1607

rubenfonseca opened this issue Oct 14, 2022 · 5 comments · Fixed by #1608
Assignees
Labels
tech-debt Technical Debt tasks v2

Comments

@rubenfonseca
Copy link
Contributor

Summary

We should remove the email-validator dependency from the v2 Lambda Layer and SAR.

Why is this needed?

As we try to reduce the size of our v2 layer, we noticed that the email-validator is only needed if you use the pydantic's EmailStr validator.

Currently, adding email-validator adds ~400KB to our compressed layer size.

Which area does this relate to?

Validation

Solution

No response

Acknowledgment

@rubenfonseca rubenfonseca added triage Pending triage from maintainers internal Maintenance changes v2 labels Oct 14, 2022
@rubenfonseca rubenfonseca self-assigned this Oct 15, 2022
@ran-isenberg
Copy link
Contributor

@rubenfonseca i think this removal will break the parser SES model that expect this depedancy during import.

from pydantic.networks import EmailStr

is 400KB really that big of a deal?

@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Oct 17, 2022
@rubenfonseca
Copy link
Contributor Author

rubenfonseca commented Oct 17, 2022

Hi @ran-isenberg! Thanks for your feedback! We thought about the cost-benefit of adding an extra dependency to validate something that is sent by AWS, and that the user (AFAIK) has little to no control. We believe it's not worth it and we could use a simple string validation instead.

400KB might not seem a very big deal, but currently is twice the size of Powertools :) We could do a lot more with that space!

I also just update the PR to remove the dependency you mentioned during the import, so hopefully no crashes during import :)

@heitorlessa
Copy link
Contributor

linking the original issue by @huonw #998

and linking Pydantic ~330ms overhead at cold start (largely due to reflection; it's expected to get a lot better in Pydantic V2): #1164 (comment)

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Oct 18, 2022
@heitorlessa heitorlessa removed the pending-release Fix or implementation already in dev waiting to be released label Oct 18, 2022
@heitorlessa
Copy link
Contributor

Closing as we're wrap to launch V2

@github-actions
Copy link
Contributor

⚠️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.

@heitorlessa heitorlessa added tech-debt Technical Debt tasks and removed internal Maintenance changes labels Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment