-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(lambda-python-alpha): support uv #33880
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33880 +/- ##
==========================================
+ Coverage 82.38% 83.98% +1.60%
==========================================
Files 120 120
Lines 6938 6976 +38
Branches 1170 1178 +8
==========================================
+ Hits 5716 5859 +143
+ Misses 1119 1005 -114
- Partials 103 112 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| dependenciesFile: DependenciesFile.UV, | ||
| // By default, uv creates a virtualenv in `/.venv` | ||
| // At the end, we remove the virtualenv to avoid creating a duplicate copy in the Lambda package. | ||
| exportCommand: `uv sync --frozen --no-managed-python --no-python-downloads && uv pip freeze > ${DependenciesFile.PIP} && rm -rf .venv`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, please could you explain the use of uv sync and pip freeze instead of uv export (like it has in the UV docs https://docs.astral.sh/uv/guides/integration/aws-lambda/)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the reference. I wasn't aware of that doc. Updated according to official guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the comment about the venv still true? You may either need to add the rm command back or remove the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, export command no longer create .venv. Comment removed. Thank you again 💯 I should have noticed that.
|
Random'ish feedback: not sure if having a That being said, I'm not sure if this is the absolute best way or if maybe the uv groups feature would make more sense... or both! I haven't tried groups yet though, EG: can a dependency be in multiple groups (this does work with workspaces)? Right now, leaning more towards workspaces because it's a little more obvious due to the pyproject.toml files and you can detect it during bundling via filesystem. |
|
@polothy your feedback is valuable actually. The reason why workspace was not chosen is simply because I've never try it 😅 . Thank you for the reference. Looking at workspaces doc, there is a section Let me add an option for choosing workspace. Thank you again for your advice and reference repo also. |
|
Hi @phuhung273, Thank you for your contribution to CDK through this PR. However, we're unable to accept it at this time, as we are currently evaluating the future graduation path for this module(lamba-python-alpha), along with alternative approach of using nix to provide customers with greater flexibility in adding package manager functionality and options on their end. You can find more information about the proposed Nix-based solution in this discussion post: #33659. Just like with the current Dockerfile setup, we can define the package manager configuration in the Nix template (default.nix): |
|
If nix is an implementation detail that the CDK team uses behind the scenes to implement If you're expecting customers to have to interact with As a customer I should just be able to provide a standard |
|
Comments on closed issues and PRs are hard for our team to see. |
Abogical
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @phuhung273, Thank you for your contribution!
I don't think it is necessary to wait for nix support in order for uv to be supported. We can go ahead with this feature.
There seems to be a lot of changes related to updating the python versions used in existing integ tests (for poetry and pipenv). Correct me if I'm wrong, but I do not think this is necessary. We can update those versions in a separate PR if needed.
| Payload: '200', | ||
| })); | ||
| }); | ||
| app.synth(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right. I was once told that this should be removed by the team, we also have #33331 to remove it all. But can see it is closed now so we dont need to remove app.synth
packages/@aws-cdk/aws-lambda-python-alpha/test/lambda-handler-poetry/pyproject.toml
Show resolved
Hide resolved
Co-authored-by: A. Abdel-Rahman <[email protected]>
Abogical
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! Just a couple of changes needed and this should be good to go.
| Payload: '200', | ||
| })); | ||
| }); | ||
| app.synth(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct that it is redundant and it should be removed, but for now I just want to limit the PR's scope to only support uv. This is to prevent any possible merge conflicts with other PR's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure, thanks for your understanding. Let me remove it
|
Thanks for your review and also replying to my email @Abogical |
Abogical
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #31238
Closes #32413
Description of changes
LambdaFunctionsupportuv.lockuv.lockdetected, install dependencies withuv pip syncinstead ofpip installfor better performance. But we can further use uv for all locks since export output is alwaysrequirements.txt.Description of how you validated changes
Unit + Integ
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license