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

feat(lambda-python): bundle dependencies in a lambda layer #9582

Merged
merged 68 commits into from Oct 12, 2020

Conversation

misterjoshua
Copy link
Contributor

@misterjoshua misterjoshua commented Aug 11, 2020

This feature enables PythonFunction to include Pipfile and requirements.txt dependencies in a function and PythonLayerVersion to include the dependencies in a lambda layer. The bundling process uses a Dockerfile build to install Pipenv and to cache the dependencies in the Docker layer cache, improving the dev experience by speeding up bundling.

Closes #9406, #9944


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 11, 2020

@jogold After looking at issue #9406, I suggested to @adamelmore that once #9576 (caching bundler output) is merged, pip installing to a dedicated lambda layer could close the issue. I have opened this draft PR to collect comments.

@misterjoshua misterjoshua changed the title feat(aws-lambda-python): add dependency layer support to the PythonFunction construct feat(aws-lambda-python): add support for putting dependencies in a lambda layer Aug 11, 2020
Copy link
Contributor

@adamdottv adamdottv left a comment

Choose a reason for hiding this comment

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

Take my comments with a grain of 🧂; love adding layer support! 🥂

packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
@SomayaB SomayaB added the pr/work-in-progress This PR is a draft and needs further work. label Aug 11, 2020
@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 14, 2020

I'll resume after we merge #9576. The dependencies pip installs may differ by the runtime, but the asset hashing can't yet tell the difference since it doesn't yet include bundler options.

Edit: Since 2edf744, this is no longer relevant. Using docker build and the layer cache to cache the dependencies has effectively made this detail a non-issue.

@misterjoshua
Copy link
Contributor Author

@adamelmore Hey. I was thinking about this PR this weekend after working on #9728. I figure that if we build the assets in a Docker build, then we can take pretty good advantage of the layer cache. I have a rough implementation up on this PR if you want to take a look.

@misterjoshua misterjoshua marked this pull request as ready for review August 17, 2020 16:46
@misterjoshua misterjoshua changed the title feat(aws-lambda-python): add support for putting dependencies in a lambda layer feat(aws-lambda-python): add support for putting Pipenv and requirements.txt dependencies in a lambda layer Aug 17, 2020
@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 17, 2020

There's been some traction in #9728. If that PR lands first, I can update this PR to use the new features.

@adamdottv
Copy link
Contributor

I'll try to review this tomorrow morning; I'm working on an Elasticsearch L2 PR or I would have reviewed it today!

@misterjoshua
Copy link
Contributor Author

I'll try to review this tomorrow morning; I'm working on an Elasticsearch L2 PR or I would have reviewed it today!

No rush. :)

@eladb eladb changed the title feat(aws-lambda-python): add support for putting Pipenv and requirements.txt dependencies in a lambda layer feat(lambda-python): add support for putting Pipenv and requirements.txt dependencies in a lambda layer Aug 19, 2020
packages/@aws-cdk/aws-lambda-python/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
@eladb eladb changed the title feat(lambda-python): add support for putting Pipenv and requirements.txt dependencies in a lambda layer feat(lambda-python): bundle dependencies in a lambda layer Aug 19, 2020
@eladb
Copy link
Contributor

eladb commented Sep 7, 2020

Is this ready for review?

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Sep 7, 2020

Is this ready for review?

@eladb Yes it is.

@misterjoshua
Copy link
Contributor Author

@eladb This python lambda PR is ready for review when you've got a chance.

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Sep 27, 2020

Merge 8747b6b from master resulted in an unexpected build failure. It seems that recent sam cli build images pip install the chardet 3.0.4 dependency slightly differently than older images.

From the diff below, it seems that there's something changed in the easy_install console script stubs generated by setuptools. I'm uncertain whether this is a one-shot deal or if it will be a recurring problem in the sam cli build images. For now, I've simply updated the expectations. I've left my notes below.

Notes:

  • When manually tested, integ.function.pipenv-inline and integ.function.project run on AWS without any problems
  • It appears that only assets bundled for Python 3.7 are affected. integ.function.pipenv-inline and integ.function.project are the only integration tests targeting 3.7 - all others are unaffected.
  • With recent amazon/aws-sam-cli-build-image-python3.7, the tests also fail in commit d9e0f68 before the merge, so the cause is probably not related to the merge from master.
  • When I re-run with a 6-week-old copy of amazon/aws-sam-cli-build-image-python3.7 (4338c0baedef), the integration tests pass.
  • I'm uncertain whether this will be a recurring problem with the sam cli build images. For now, I've simply updated the expectations.

integ.function.pipenv-inline asset diff between old and new amazon/aws-sam-cli-build-image-python3.7:

diff -ru cdk.out.oldimg/asset.771170a5b339defba72bda907cf9ccb30fce9b91e45bbb3152dd9df29589957d/bin/chardetect cdk.out.newimg/asset.6327fef54763f920a63cab392d9bd4bc9b14917985a3f95700fcd35610b5e83c/bin/chardetect
--- cdk.out.oldimg/asset.771170a5b339defba72bda907cf9ccb30fce9b91e45bbb3152dd9df29589957d/bin/chardetect        2020-09-27 12:25:54.730254541 -0600
+++ cdk.out.newimg/asset.6327fef54763f920a63cab392d9bd4bc9b14917985a3f95700fcd35610b5e83c/bin/chardetect        2020-09-27 12:18:17.686252265 -0600
@@ -2,9 +2,7 @@
 # -*- coding: utf-8 -*-
 import re
 import sys
-
 from chardet.cli.chardetect import main
-
 if __name__ == '__main__':
-    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
+    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
     sys.exit(main())
diff -ru cdk.out.oldimg/asset.771170a5b339defba72bda907cf9ccb30fce9b91e45bbb3152dd9df29589957d/chardet-3.0.4.dist-info/RECORD cdk.out.newimg/asset.6327fef54763f920a63cab392d9bd4bc9b14917985a3f95700fcd35610b5e83c/chardet-3.0.4.dist-info/RECORD
--- cdk.out.oldimg/asset.771170a5b339defba72bda907cf9ccb30fce9b91e45bbb3152dd9df29589957d/chardet-3.0.4.dist-info/RECORD        2020-09-27 12:25:54.734254490 -0600
+++ cdk.out.newimg/asset.6327fef54763f920a63cab392d9bd4bc9b14917985a3f95700fcd35610b5e83c/chardet-3.0.4.dist-info/RECORD        2020-09-27 12:18:17.690252286 -0600
@@ -1,4 +1,4 @@
-../../bin/chardetect,sha256=bDJN3SiDn9YaLlVuQisJzBXSUWH1KDqMdPjZKiY1Pxc,231
+../../bin/chardetect,sha256=cvP0mdvKK8hDplcSQ2LSWTZAe5uUxr8swGu_T3pnmtc,228
 chardet-3.0.4.dist-info/DESCRIPTION.rst,sha256=PQ4sBsMyKFZkjC6QpmbpLn0UtCNyeb-ZqvCGEgyZMGk,2174
 chardet-3.0.4.dist-info/INSTALLER,sha256=zuuue4knoyJ-UwPPXg8fezS7VCrXJQrAP7zeNuwvFQg,4
 chardet-3.0.4.dist-info/METADATA,sha256=RV_2I4B1Z586DL8oVO5Kp7X5bUdQ5EuKAvNoAEF8wSw,3239

integ.function.project asset diff between old and new amazon/aws-sam-cli-build-image-python3.7:

diff -ru cdk.out.oldimg/asset.4931a239511545d5c1e3ab8d27ce860ac9ac33d0501d86ec31749c676d678b27/python/bin/chardetect cdk.out.newimg/asset.7a572697b6cea2f1d84014e9f44093d47abc652f5d0fe9e8857a5ea6cd994bff/python/bin/chardetect
--- cdk.out.oldimg/asset.4931a239511545d5c1e3ab8d27ce860ac9ac33d0501d86ec31749c676d678b27/python/bin/chardetect 2020-09-27 12:26:03.414144722 -0600
+++ cdk.out.newimg/asset.7a572697b6cea2f1d84014e9f44093d47abc652f5d0fe9e8857a5ea6cd994bff/python/bin/chardetect 2020-09-27 12:18:17.778252749 -0600
@@ -2,9 +2,7 @@
 # -*- coding: utf-8 -*-
 import re
 import sys
-
 from chardet.cli.chardetect import main
-
 if __name__ == '__main__':
-    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
+    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
     sys.exit(main())
diff -ru cdk.out.oldimg/asset.4931a239511545d5c1e3ab8d27ce860ac9ac33d0501d86ec31749c676d678b27/python/chardet-3.0.4.dist-info/RECORD cdk.out.newimg/asset.7a572697b6cea2f1d84014e9f44093d47abc652f5d0fe9e8857a5ea6cd994bff/python/chardet-3.0.4.dist-info/RECORD
--- cdk.out.oldimg/asset.4931a239511545d5c1e3ab8d27ce860ac9ac33d0501d86ec31749c676d678b27/python/chardet-3.0.4.dist-info/RECORD 2020-09-27 12:26:03.418144671 -0600
+++ cdk.out.newimg/asset.7a572697b6cea2f1d84014e9f44093d47abc652f5d0fe9e8857a5ea6cd994bff/python/chardet-3.0.4.dist-info/RECORD 2020-09-27 12:18:17.782252769 -0600
@@ -1,4 +1,4 @@
-../../bin/chardetect,sha256=bDJN3SiDn9YaLlVuQisJzBXSUWH1KDqMdPjZKiY1Pxc,231
+../../bin/chardetect,sha256=cvP0mdvKK8hDplcSQ2LSWTZAe5uUxr8swGu_T3pnmtc,228
 chardet-3.0.4.dist-info/DESCRIPTION.rst,sha256=PQ4sBsMyKFZkjC6QpmbpLn0UtCNyeb-ZqvCGEgyZMGk,2174
 chardet-3.0.4.dist-info/INSTALLER,sha256=zuuue4knoyJ-UwPPXg8fezS7VCrXJQrAP7zeNuwvFQg,4
 chardet-3.0.4.dist-info/METADATA,sha256=RV_2I4B1Z586DL8oVO5Kp7X5bUdQ5EuKAvNoAEF8wSw,3239

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

LGTM, is this ready to be merged?

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Oct 11, 2020

LGTM, is this ready to be merged?

@eladb I'll fix the merge conflict and then yep, it'll be ready!

eladb
eladb previously approved these changes Oct 11, 2020
@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed eladb’s stale review October 11, 2020 16:10

Pull request has been modified.

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Oct 11, 2020

@eladb Oh jeeze. I didn't see the merge bot message until after I fixed the conflict via github. Is this going to be a problem?

Edit: I'm going to grab a coffee and figure out what I missed in the conflict resolution. Sorry about this confusion.
Edit2: Ok I've found the problem - I missed removing an extra import declaration. I have the fix in hand in my local repo, but I'm not sure if I should be waiting for mergify to do something first?
Edit3: I'm going to push up the fix for the issue I introduced by merging via github ui.

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Oct 12, 2020

@eladb Hey. It looks like because I accidentally fixed the merge conflict concurrently with the mergify bot action, this PR got stuck and not merged into the main branch. It may need your eyes.

@mergify
Copy link
Contributor

mergify bot commented Oct 12, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@gitpod-io
Copy link

gitpod-io bot commented Oct 12, 2020

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: a82411d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Oct 12, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit aebac92 into aws:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/work-in-progress This PR is a draft and needs further work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@aws-cdk/aws-lambda-python] Support caching of built assets
6 participants