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

A contrib package for building AWS Lambdas from python code. #6881

Merged
merged 5 commits into from Dec 8, 2018

Conversation

Projects
None yet
5 participants
@benjyw
Copy link
Contributor

benjyw commented Dec 7, 2018

Builds a pex, then runs lambdex to turn that pex into a bundle suitable for uploading to AWS Lambda.

@benjyw benjyw requested review from jsirois and baroquebobcat Dec 7, 2018

@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

Very cool!

  1. Consider renaming awslambda to aws_lambda. I find the latter more readable and idiomatic, although at some level this is personal preference.
  2. Are there any meaningful unit tests we could add? Maybe for compute_dependency_specs? It's okay if there aren't, but unit tests could be helpful to catch regressions. The integration test does look solid.
  3. Overall really good code 🔥
targets = self.get_targets(self._is_python_lambda)
with self.invalidated(targets=targets) as invalidation_check:
python_lambda_product = self.context.products.get_data('python_aws_lambda', dict)
for vt in invalidation_check.all_vts:

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Dec 7, 2018

Contributor

I'm not sure what vt refers to. Disclaimer I've never used AWS Lambdas.

This comment has been minimized.

@wisechengyi

wisechengyi Dec 7, 2018

Contributor

VersionedTarget

class VersionedTarget(VersionedTargetSet):

It's a common pattern for a lot of the tasks.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

VersionedTarget -- it's a common abbreviation used in implementations of v1 tasks.

@cosmicexplorer
Copy link
Contributor

cosmicexplorer left a comment

I'm a little concerned about the integration testing for this new contrib package, and mentioned that one solution might be to incorporate an amazon linux shard into our CI. I also mentioned the convergence of pex tool subsystems might be a good idea -- would love to discuss any of that. Very exciting work.

from pants.backend.python.subsystems.python_tool_base import PythonToolBase


class Lambdex(PythonToolBase):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

Would love to see convergence of PythonToolBase with pants.binaries.executable_pex_tool.ExecutablePexTool, especially since the latter happens to already handle creating the pex itself. I can make a separate PR merging ExecutablePexTool into PythonToolBase, not sure if there's a utility to a separate PythonToolBase that I'm missing.

This comment has been minimized.

@stuhood

stuhood Dec 7, 2018

Member

Blargh. Forgot about that. Yea.

This comment has been minimized.

@benjyw

benjyw Dec 7, 2018

Contributor

Yeah, I missed that. That's going to have to be a separate change. I'll tackle it.

This comment has been minimized.

@benjyw

benjyw Dec 7, 2018

Contributor

Issue here: #6888

Note that your pex must be built to run on Amazon Linux, e.g., if it contains native code.
When deploying the lambda, its handler should be set to `lambdex_handler.handler`, which
is a wrapper around the target-specified handler.
"""

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

On the face of it, I would love it if an amazon linux image could be added to our set of travis images before merging this. I don't have a problem with merging this otherwise except that I would feel uncomfortable if the only testing was outside the open-source repo. I think the ability to test aws lambda functions would be a fantastic feature to incorporate on its own, and would love to discuss in an issue or something how best to incorporate that kind of testing. I'm mostly concerned about bitrot -- this is a great feature and I'm trying to not block it. Not sure if there is context I'm missing here wrt testing, and not sure how that testing would be accomplished.

This comment has been minimized.

@benjyw

benjyw Dec 7, 2018

Contributor

moto, the mock AWS library, does support Lambda APIs, but requires Docker to actually run them. That seems like the right path, but that may be a non-trivial change to our CI, I think. Still, running Amazon Linux in a Docker container is likely easier and less costly than having a travis image just for this one thing.

@wisechengyi
Copy link
Contributor

wisechengyi left a comment

Thanks, @benjyw! Looks good, with some nits


def execute(self):
targets = self.get_targets(self._is_python_lambda)
with self.invalidated(targets=targets) as invalidation_check:

This comment has been minimized.

@wisechengyi

wisechengyi Dec 7, 2018

Contributor

iirc invalidated is a little expensive even with empty targets, so would recommend adding

if not targets:
  return

This comment has been minimized.

@benjyw

benjyw Dec 7, 2018

Contributor

I'm not aware of such a performance issue, but if so, this check should be inside invalidated, probably, rather than every task having to think of it.

targets = self.get_targets(self._is_python_lambda)
with self.invalidated(targets=targets) as invalidation_check:
python_lambda_product = self.context.products.get_data('python_aws_lambda', dict)
for vt in invalidation_check.all_vts:

This comment has been minimized.

@wisechengyi

wisechengyi Dec 7, 2018

Contributor

VersionedTarget

class VersionedTarget(VersionedTargetSet):

It's a common pattern for a lot of the tasks.

pants_run = self.run_pants(command=command, config=config)
self.assert_success(pants_run)
awslambda = os.path.join(distdir, 'hello-lambda.pex')
output = subprocess.check_output(env={'PEX_INTERPRETER': '1'}, args=[

This comment has been minimized.

@wisechengyi

wisechengyi Dec 7, 2018

Contributor

Is the goal here to check the pex is lambdex'ed rather than a normal pex? If so, would you kindly add a comment?

This comment has been minimized.

@benjyw

benjyw Dec 7, 2018

Contributor

Will add an appropriate comment.

@stuhood

stuhood approved these changes Dec 7, 2018

Copy link
Member

stuhood left a comment

Thanks!


from pants.contrib.awslambda.python.examples.hello_lib import say_hello


This comment has been minimized.

@stuhood

stuhood Dec 7, 2018

Member

Conventionally these would go in examples or testprojects folders at contrib/awslambda/python/{examples,testprojects}

from pants.backend.python.subsystems.python_tool_base import PythonToolBase


class Lambdex(PythonToolBase):

This comment has been minimized.

@stuhood

stuhood Dec 7, 2018

Member

Blargh. Forgot about that. Yea.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 7, 2018

@Eric-Arellano Re your other questions/comments: the way we use compute_dependency_specs there is idiomatic, and used thus elswhere. So it's not really the right place to test that. And re the name, I've seenawslambda used quite frequently in other people's python code (lambda is a reserved word, this is what happens when branding collides with reality...) so I prefer it on balance.

benjyw added some commits Dec 7, 2018

output = subprocess.check_output(env={'PEX_INTERPRETER': '1'}, args=[
'{} -c "from lambdex_handler import handler; handler(None, None)"'.format(awslambda)
], shell=True)
self.assertEquals('Hello from United States!'.encode('utf8'), output.strip())

This comment has been minimized.

@wisechengyi

wisechengyi Dec 7, 2018

Contributor
Suggested change Beta
self.assertEquals('Hello from United States!'.encode('utf8'), output.strip())
self.assertEquals('Hello from the United States!'.encode('utf8'), output.strip())
@Eric-Arellano

This comment has been minimized.

You can alternatively prefix the string with b. Both work

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 7, 2018

Ah I wasn't sure what the status of the b prefix is in Python 3. Does it require the literal to be ascii? Or does it just assume the encoding of the source file?

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Dec 7, 2018

@benjyw yes, b prefix requires string literal to be only ASCII, and UTF-8 is a superset of ASCII so that all works out. (TIL, hadn't looked into it till now)

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 7, 2018

Interesting. They could also have gone with "the string of bytes that represents this literal in the source file, whatever that may be".

@benjyw benjyw merged commit 5befacd into pantsbuild:master Dec 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:contrib_lambda branch Dec 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment