-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): support for providing a custom bundling docker image #18082
feat(lambda-python): support for providing a custom bundling docker image #18082
Conversation
91baabe
to
ca10615
Compare
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.
By mistake I have approved
d55c1f6
to
5bcab84
Compare
// Then, write a requirements.txt without the extraneous dependency and synth again | ||
const afterDeps = 'certifi==2020.6.20\nchardet==3.0.4\nidna==2.10\nurllib3==1.26.7\nrequests==2.26.0\nPillow==8.4.0\n'; | ||
fs.writeFileSync(requirementsTxtPath, afterDeps); |
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.
This test kept failing for me, regardless of how I modified the afterDeps
value. In the traceback, I could only see one of the commands being run, which makes me think it has something to do with how synth + caching occurs with the bundling process.
In the previous version, dependency installation was occurring during the Docker build process (compared to Docker image run now), so maybe that has something to do with it? Not sure.
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.
Noticed this in the logs:
### `cdk synthesize`
aws-cdk: Synthesizes the CDK app and produces a cloud assembly to a designated output (defaults to `cdk.out`)
aws-cdk: Typically you don't interact directly with cloud assemblies. They are files that include everything
aws-cdk: needed to deploy your app to a cloud environment. For example, it includes an AWS CloudFormation
aws-cdk: template for each stack in your app, and a copy of any file assets or Docker images that you reference
aws-cdk: in your app.
which to me reads like: app.synth()
in the above referenced stack wouldn't actually create the asset; that would be during cdk deploy
.
@corymhall : Given all tests, including integration tests are succeeding, I feel good about this PR. I'm happy to start documenting this into the readme and rebase to |
Sounds good. |
5bcab84
to
0a43b76
Compare
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.
@corymhall : This is complete and ready for review.
```ts | ||
new lambda.PythonFunction(this, 'MyFunction', { | ||
entry: '/path/to/my/function', | ||
runtime: Runtime.PYTHON_3_6, | ||
layers: [ | ||
new lambda.PythonLayerVersion(this, 'MyLayer', { | ||
entry: '/path/to/my/layer', // point this to your library's directory | ||
}), | ||
], | ||
}); | ||
``` |
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.
Moved this up.
```ts | ||
new lambda.PythonLayerVersion(this, 'MyLayer', { | ||
entry: '/path/to/my/layer', // point this to your library's directory | ||
}) |
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.
Added this example.
Packaging is executed using the `Packaging` class, which: | ||
|
||
1. Infers the packaging type based on the files present. | ||
2. If it sees a `Pipfile` or a `poetry.lock` file, it exports it to a compatible `requirements.txt` file with credentials (if they're available in the source files or in the bundling container). | ||
3. Installs dependencies using `pip`. | ||
4. Copies the dependencies into an asset that is bundled for the Lambda package. |
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.
Added some details on how bundling occurs.
## Custom Bundling | ||
|
||
Custom bundling can be performed by using custom Docker images for bundling dependencies. If using a custom Docker image for bundling, the dependencies are installed with `pip`, `pipenv` or `poetry` by using the `Packaging` class. | ||
|
||
A different bundling Docker image that is in the same directory as the function can be specified as: | ||
|
||
```ts | ||
const entry = '/path/to/function'; | ||
const image = DockerImage.fromBuild(entry); | ||
|
||
new PythonFunction(stack, 'function', { | ||
entry, | ||
bundling: { image }, | ||
}); |
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.
Added example that shows how to use a custom bundling image.
# Ensure rsync is installed | ||
RUN yum -q list installed rsync &>/dev/null || yum install -y rsync | ||
# Upgrade pip (required by cryptography v3.4 and above, which is a dependency of poetry) | ||
RUN pip install --upgrade pip | ||
RUN pip install pipenv poetry |
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.
We don't need rsync
and can just use cp
.
}, | ||
platform: architecture.dockerPlatform, | ||
}); | ||
this.image = image ?? defaultImage; |
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.
This is where a custom Docker image would get pulled in when bundling.
*/ | ||
readonly assetHashType?: AssetHashType; | ||
readonly runtime?: Runtime; |
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.
Making this optional.
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.
We decided to make this a required property in #18143
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.
Oh I missed that, thanks for the tip! Wouldn't it be nicer to default to Python 3.8 instead to better resolve #10248? That has a much longer EOL, has been around for a few years and is well supported.
*/ | ||
readonly assetHashType?: AssetHashType; |
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.
Moved to BundlingOptions
.
*/ | ||
readonly assetHash?: string; |
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.
Moved to BundlingOptions
.
if (props.runtime && props.runtime.family !== lambda.RuntimeFamily.PYTHON) { | ||
throw new Error('Only `PYTHON` runtimes are supported.'); | ||
} | ||
const { index = 'index.py', handler = 'handler', runtime = Runtime.PYTHON_3_7 } = props; |
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.
Defining defaults while unpacking props
.
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.
@setu4993 This looks awesome! Thanks for all the work on this, we're closing a lot of issues :)
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). |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thanks @corymhall! Glad we got this in place :). |
Hi @setu4993, sorry if this isn't an appropriate place to raise this, but I am having issues with my PythonFunction since this change.
This is my CDK code, which worked perfectly fine with CDK 1.137.0. With 1.138.0 (and 1.138.1), without making any changes to the above CDK code or the directory structure or the lambda implementation, my lambda suddenly stopped working with this error:
Perhaps I am not fully understanding the implications of the breaking changes, but my understanding is that my code should still set up the Docker container just as it did previously. Can you point me towards a solution that works with CDK 1.138.0 and above? |
@chrispykim : Thanks for flagging! I missed a |
Asset files are incorrectly being bundled under the `asset-input` directory instead of the root of the bundle. To also copy over hidden files (#18306 (comment)), I switched from using `-R` to `-a` based on what I found on [SO](https://stackoverflow.com/a/13020113) and the [man page](https://linux.die.net/man/1/cp). (`-a` is equivalent to `-dR`.) Fixes #18301 and @chrispykim's comment: #18082 (comment). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…mage (aws#18082) This refactors the bundling process to match the NodeJs and Go Lambda functions and allows providing a custom bundling docker image. Changes: - refactor bundling to use `cdk.BundlingOptions` - Use updated `Bundling` class - Update tests to use updated `Bundling` class Fixes aws#10298, aws#12949, aws#15391, aws#16234, aws#15306 BREAKING CHANGE: `assetHashType` and `assetHash` properties moved to new `bundling` property. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Asset files are incorrectly being bundled under the `asset-input` directory instead of the root of the bundle. To also copy over hidden files (aws#18306 (comment)), I switched from using `-R` to `-a` based on what I found on [SO](https://stackoverflow.com/a/13020113) and the [man page](https://linux.die.net/man/1/cp). (`-a` is equivalent to `-dR`.) Fixes aws#18301 and @chrispykim's comment: aws#18082 (comment). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…19143) #18082 added the ability to pass bundling parameters to `PythonFunction`'s. Currently when the `image` parameters is passed both the passed image and the default image are built even though the default image is not used ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This refactors the bundling process to match the NodeJs and Go Lambda functions and allows providing a custom bundling docker image.
Changes:
cdk.BundlingOptions
Bundling
classBundling
classFixes #10298, #12949, #15391, #16234, #15306
BREAKING CHANGE:
assetHashType
andassetHash
properties moved to newbundling
property.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license