-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ETL-683] Increase memory of raw lambda #140
Conversation
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.
Great benchmarking! I'll leave this to @rxu17 for a review.
👀 the nesting of context managers (unfortunate, but makes sense)
701e98e
to
7da9ed5
Compare
7da9ed5
to
58b2078
Compare
Quality Gate passedIssues Measures |
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.
LGTM! Just a few minor comments
|
||
- name: Test lambda scripts with pytest | ||
run: | | ||
pipenv run python -m pytest tests/test_s3_event_config_lambda.py -v | ||
pipenv run python -m pytest tests/test_s3_to_glue_lambda.py -v | ||
pipenv run python -m pytest -v tests/test_lambda_raw.py |
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.
Nit: Depending on if this gets merged first or mine (may be a slight merge conflict), but i think you can link the tests like:
- name: Test scripts with pytest (lambda, etc.)
run: |
pipenv run python -m pytest \
tests/test_s3_event_config_lambda.py \
tests/test_s3_to_glue_lambda.py \
tests/test_lambda_dispatch.py \
tests/test_consume_logs.py \
tests/test_lambda_raw.py -v
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.
Eh, it's better for the commands to be discrete and explicit, imo. I'm not sure if the behavior is the same running the tests one at a time (separate pipenv run...
) vs running all the tests at one time (a single pipenv run...
). But if there are advantages (other than brevity) I'm open to running all the tests at one time.
@@ -52,7 +52,7 @@ Resources: | |||
Handler: app.lambda_handler | |||
Runtime: !Sub "python${LambdaPythonVersion}" | |||
Role: !Ref RoleArn | |||
MemorySize: 1024 | |||
MemorySize: 1769 |
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.
Nit: Something i was thinking of but do we want to make this a variable instead? So the develop version of the lambda can declare a smaller memory size than the prod version since it won't use as much? Or were you running into this for both?
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 there's much benefit. This is not an expensive function to run, so no harm in keeping the configurations the same.
Changes:
compressed_data
in a context manager so that we don't have to manually closemain
in case we ever want to use a batch size larger than 1.Details about benchmarking exploration are here.