-
Notifications
You must be signed in to change notification settings - Fork 73
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
Feature: custom runtimes to define function #1602
base: main
Are you sure you want to change the base?
Feature: custom runtimes to define function #1602
Conversation
🦋 Changeset detectedLatest commit: cbaa8bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 @MarlonJD thanks for taking the time to open this. We will likely not merge this in its current form. There are a few reasons for this:
- We cannot depend on CDK alpha packages in our stable release line. The approach outlined in the RFC you commented on moves the alpha CDK dependency to the customer's project so they are explicitly aware they are using alpha functionality.
- Changing the
runtime
parameter from a number union to a string union is a breaking API change which we cannot do without a major version bump of our libraries - E2E tests that exercise building and calling python and go functions need to be added
- The dependency on docker to build the functions needs more consideration of the implications (eg for backend deployments on Amplify hosting and getting started locally)
If you are interested, you can work with @josefaidt to define an interface that is appropriate here and then we can have further discussion on the CDK and docker dependencies.
Hi @edwardfoyle. |
Hey @MarlonJD 👋 thanks for taking the time to file this! To add to @edwardfoyle 's note we'd like to avoid taking a dependency on Docker and the alpha distributions of L2 constructs for Go and Python. While those two runtimes are top of mind, the escape hatch is desirable to offer the capability of using |
Hello @josefaidt. I'm trying to change my PR, I'm trying to create defineFunctionWithAnyLanguage in @aws-amplify/backend. Now I'm struggling about creating callback function for Construct/scope and use this function, any help in here? I can create directly with Stack parameter like this:
But it's will depend to |
Hey @MarlonJD you can define a union on the the props here will need adjustments and the scope would be passed to the We wouldn't want to necessarily change the scope but change how "props" (in this case, the callback) is called with the existing |
9a96a36
to
bb5e0df
Compare
@josefaidt I changed PR to this:
or
Golang function will be usable out of box, python function will be require docker, so python function cannot be used on CI/CD, it can be deploy with custom pipeline on local. ie: disabling auto deploy, What are you thinking about this changes? If it's okay, I'll try to write e2e tests, can you guide me again? |
I hope you can review this changes @edwardfoyle |
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 heading in the right direction! Another thing we need to do is turn off generating the typed process.env file when using custom runtimes. We can probably do that by creating a new noop FunctionEnvironmentTypeGenerator
if a callback is specified to defineFunction
We should also have an integration and/or e2e test that asserts permissions and environment variables are correctly attached to functions with custom runtimes.
@edwardfoyle I did changes, if it's okay about code, can you help me how and where should write for integration and/or e2e tests? |
@edwardfoyle Hey there, I hope you can review new changes |
Hi @MarlonJD, sorry for the delay here. While this PR is on the right track in terms of implementation, we are concerned about the implications when deploying using the Amplify hosting service. Specifically, CDK requires Docker to build some runtimes and currently Docker is not installed in the hosting build container. Docker also cannot be installed using a custom build script because it requires permissions that the execution environment doesn't have. This means that customers using custom runtimes would not be able to use Hosting deployments. We want to consider this feature holistically rather than only offer partial support across our product offering. As a result, we need to do some internal prerequisite work to ensure custom runtime deployments can work in Amplify hosting deployments. |
@josefaidt @edwardfoyle Hey there! I have good news about python functions. There is already a
I just tried any it's works. I used pure |
@edwardfoyle Any updates? |
PTAL |
Great work @MarlonJD! Hope this gets merged quickly as currently im trying to achieve same but in a really whacky way |
Hey @josefaidt, I commented how we can use python functions without docker build, but there is no local parameter in What are you thinking about this solution to using python functions like this, should we add local parameter to |
Any updates on this PR? @edwardfoyle, has @MarlonJD addressed your request? It would be super, super helpful to get this merged! |
I was able to build my python custom function with my requirements. The only problem I have now is that this seems to be never updated even if I alter this line. Seems that a missing force/cache somewhere. Only workaround I've found now: remove the |
@guikcd oh it's a good review, let me check how nodejs changes triggers, maybe we can found fix about it |
@josefaidt Hey there, I was looking into tracking changes on lambda functions, but I couldn't found this yet, I found FilesChangesTracker class but it's for sandbox file changes, can you help me how can I found how lambda functions triggering bundling again when function have changes? |
@MarlonJD We're planning to evaluate this change further, i.e. test proposed changes at runtime. We'll get back to you with further guidance. |
Hello! |
I need to fix conflicts, but many things changed, now there is schedule, layers, bundling, resourceGroupName, logging more, I need to create this pr from the beginning again. I'll create new commits with force push to my branch, maybe it's good to wait new release if you already doing some works in these files? What are you suggesting? @sobolk |
@MarlonJD Please allow a bit more time for us to evaluate and provide guidance. Meanwhile, rebasing this PR with latest |
@MarlonJD
|
Hey @sobolk. Thanks for detailed review, I'm trying to understand changes and I'll try to prepare your requested changes. I'll let you know when I'm ready to share my changes. |
ad33727
to
e7f415c
Compare
Hey @sobolk 👋. I just made changes, add unit test and integration test, PTAL. |
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.
The changes look good. Please take a look at pull request checks and make them passing.
I ran e2e tests here https://github.com/aws-amplify/amplify-backend/actions/runs/12435184989 . They are passing.
packages/integration-tests/src/test-projects/advanced-auth-and-functions/amplify/function.ts
Outdated
Show resolved
Hide resolved
// TODO better resolution | ||
resolution: | ||
'Ensure that docker is present and works correctly. See https://some.link.about.docker.', |
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're still evaluating the docker builds. Therefore we'll keep the PR open until it's done on our side.
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 can use go functions without and error, an also we can use python functions without docker. I tried and gave example before how we can use python without docker Any change to pass at least for golang?
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 understand and we did see these samples. The problem is that chances are high that customers trying this new functionality will run into "docker not found or not working" errors, non-docker bundling requires extra steps and is not mainstream CDK scenario. I.e. we need to prepare that link mentioned in resolution message.
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.
What about we make bundling without docker on Linux by default in python alpha package? On ci/cd it will use Amazon Linux, so we don't need docker in Linux for Python, it can be fast solution
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.
What about we make bundling without docker on Linux by default in python alpha package? On ci/cd it will use Amazon Linux, so we don't need docker in Linux for Python, it can be fast solution
These kind of improvements can be added later outside of this PR. What we need to merge this PR is docs that clearly state the scope of support. Please allow us a bit more time to come up with that (we're in holidays season).
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 can also create docs for creating new go and python runtimes with this new support, is this works or should I wait?
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 can also create docs for creating new go and python runtimes with this new support, is this works or should I wait?
You could give it a stab. We should aim to have a sub page under https://docs.amplify.aws/react/build-a-backend/functions/ . Our docs are defined in https://github.com/aws-amplify/docs repo.
Couple of things to keep in mind:
- The page should be called "Custom functions"
- The page must call out lack of docker support.
- We can use docker-less python build as an example.
- I recommend to avoid GO as an example (it doesn't seem to have first class support, see https://aws.amazon.com/blogs/compute/migrating-aws-lambda-functions-from-the-go1-x-runtime-to-the-custom-runtime-on-amazon-linux-2/ )
- We might need to wait for reviewers quorum until after Jan 6th due to holidays.
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.
Okay I'll try it, Golang is not anymore directly runtime but it still can be using directly just by giving runtime as provided.al2023, I personally using this without an error, I would be happy if I can add Golang there with Python.
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 created PR for documents here. Happy holidays.
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.
@josefaidt please take a look at docs PR ^ when you're back.
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.
@MarlonJD
I looked at latest diff and it looks good overall.
Couple of areas I left comments:
- test project - I ran e2e tests here https://github.com/aws-amplify/amplify-backend/actions/runs/12552245657 - they failed, I left comments with changes to make them passing.
- The error messages. For these please keep in mind that they need that docs PR to stabilize, so this might not be final iteration.
packages/integration-tests/src/test-projects/advanced-auth-and-functions/amplify/function.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Kamil Sobol <sobol.k.r@gmail.com>
Co-authored-by: Kamil Sobol <sobol.k.r@gmail.com>
Co-authored-by: Kamil Sobol <sobol.k.r@gmail.com>
Co-authored-by: Kamil Sobol <sobol.k.r@gmail.com>
Hi @sobolk, I’ve implemented the changes based on your suggestions. Please let me know if there’s anything else that needs adjustment. |
Thank you. The change in test project needs imports, see https://github.com/aws-amplify/amplify-backend/actions/runs/12556641214/job/35018558350?pr=1602 |
Hi @sobolk, I've added the missing imports to address the issue. |
Thanks. The e2e look good here https://github.com/aws-amplify/amplify-backend/actions/runs/12584741475 . Could you please address |
Hi @sobolk, I’ve resolved the linting errors and committed the changes. I ran |
Problem
#1543, #1486
Changes
I changedruntime
parameter to string from int. It was only nodejs version. Now it can be:'GO_1_X_PROVIDED_AL2023' | 'NODEJS_16_X' | 'NODEJS_18_X' | 'NODEJS_20_X' | 'PYTHON_3_8' | 'PYTHON_3_9' | 'PYTHON_3_10' | 'PYTHON_3_11' | 'PYTHON_3_12'
It's defaultNODEJS_18_X
same as old functions, people only need to specifyruntime: "NODEJS_16_X"
if they want another version, it's giving lint error, so it's easy to inform people if it's changed. If it's empty, it's still creating nodejs version, I also add required toentry
parameter when it's go or python runtimes.Edit: I modified
defineFunction
to props and callback function can be parameters at the same time:can be usable like proposal in RFC, go function will be usable out of box, python function will be require docker, so python function cannot be used on CI/CD, it can be deploy with custom pipeline on local.
ie: disabling auto deploy,
npm ci && export CI=1 && npx ampx pipeline-deploy --branch BRANCH_NAME --app-id AMPLIFY_APP_ID
will do the trick for python.Checklist
run-e2e
label set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.