-
Notifications
You must be signed in to change notification settings - Fork 10
convert Lambda to a container function #99
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
base: main
Are you sure you want to change the base?
Conversation
@chuckwondo can you take a look at infrastructure/aws/Dockerfile? If you have any ideas on how to optimize it further for smaller final image size and minimized startup time I am all ears! |
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 @hrodmn! Thanks for pinging me to review.
Regarding the size of the resulting docker image goes, I don't see anything obvious for shrinking it further. Looks good.
I'm approving because the comments I've made are simply some hygiene suggestions, so feel free to accept or reject any or all at your pleasure.
# Lambda container optimizations | ||
os.environ.setdefault("PYTHONPATH", "/var/runtime") | ||
os.environ.setdefault("AWS_DEFAULT_REGION", os.environ.get("AWS_REGION", "us-west-2")) |
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.
Why are you setting these in python code? Shouldn't you set these via the cdk code?
.git | ||
infrastructure/aws/cdk.out | ||
.venv | ||
.mypy_cache | ||
.pytest_cache | ||
.ruff_cache |
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 recommend using this file as a "permit" list rather than a "deny" list, which offers safety along with minimal docker context transfers.
.git | |
infrastructure/aws/cdk.out | |
.venv | |
.mypy_cache | |
.pytest_cache | |
.ruff_cache | |
* | |
!src/ | |
!infrastructure/aws/lambda/*.py | |
!.python-version | |
!pyproject.toml | |
!README.md | |
!uv.lock |
By making this change, we can see very explicitly what exactly is going into the image, rather than what isn't. Further, it avoids accidental inclusion of unnecessary context transfers, which can happen when you forget to ignore something, such as a large directory structure (e.g., .venv
). If you forget to "deny" something, docker doesn't complain, but if you forget to "permit" something, it will, which helps keep the "deny" list both correct and minimal.
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.
Something about this change was not compatible with cdk deploy
. cdk synth
could build the image, but the deployment failed with this error:
titiler-multidim-development: start: Building 3c5ae7ad7830d4fa4db2c808913a529ec0658455228ce644c565cd1cb957ab9d
ERROR: failed to build: resolve : lstat infrastructure: no such file or directory
titiler-multidim-development: fail: docker build --tag cdkasset-3c5ae7ad7830d4fa4db2c808913a529ec0658455228ce644c565cd1cb957ab9d --file infrastructure/aws/lambda/Dockerfile --platform linux/amd64 . exited with error code 1: ERROR: failed to build: resolve : lstat infrastructure: no such file or directory
full log: https://github.com/developmentseed/titiler-multidim/actions/runs/18014979319/job/51257489935
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.
Hmmm. That's weird.
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
infrastructure/aws/lambda/Dockerfile
Outdated
|
||
# Copy package size manifest for debugging | ||
COPY --from=builder /tmp/package_size.txt /tmp/ |
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.
Oops! Did these get accidentally removed by my suggestion?
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, I removed those - decided to just skip it!
We have historically deployed titiler applications as zip packages. Since we are dangerously close to the size limit for this type of Lambda deployment, we are interested in changing it to a container image function.