Skip to content
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

Merged

Conversation

MarlonJD
Copy link
Contributor

@MarlonJD MarlonJD commented Jun 3, 2024

Problem

#1543, #1486

Changes

I changed runtime 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 default NODEJS_18_X same as old functions, people only need to specify runtime: "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 to entry parameter when it's go or python runtimes.

Edit: I modified defineFunction to props and callback function can be parameters at the same time:

defineFunction({
  name: "my-regular-function",
})

// a go function
defineFunction((scope) => {
  return new GoFunction(scope, "GoFunction", {
    entry: "app/cmd/api",
  })
})

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

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the 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.

@MarlonJD MarlonJD requested review from a team as code owners June 3, 2024 16:17
Copy link

changeset-bot bot commented Jun 3, 2024

🦋 Changeset detected

Latest commit: aa6762e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@aws-amplify/backend-function Minor
@aws-amplify/backend Minor

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

@MarlonJD MarlonJD changed the title Feature/custom runtimes to define function Feature: custom runtimes to define function Jun 3, 2024
Copy link
Contributor

@edwardfoyle edwardfoyle left a 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:

  1. 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.
  2. 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
  3. E2E tests that exercise building and calling python and go functions need to be added
  4. 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.

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jun 3, 2024

Hi @edwardfoyle.
1-) I understand what you mean, okay, I'll try to change this
2-) What about adding new parameter without modifying runtime?
3-) e2e tests will be added
4-) python docker build is necessary on python build, if it's not possible ı can remove python in pr

@josefaidt
Copy link
Contributor

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 defineFunction with any runtime, with an opt-out of TypeScript-based features like typed environment variables and automatic secrets resolution. Would you be interested in pivoting this PR to match the proposal? Happy to discuss any findings/thoughts you have along the way in the RFC!

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jun 3, 2024

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:

const lambdaStack = Stack.of(backend.sayHelloHandler.resources.lambda.stack);

const goFunctionStack = new NestedStack(lambdaStack, "say-hello-go-lambda");

export const goFunction = defineFunctionWithAnyLanguage(
  new GoFunction(goFunctionStack, "GoFunction", {
    entry: "./amplify/functions/say-hello-go/",
    runtime: Runtime.PROVIDED_AL2023,
    environment: {
      GOARCH: "amd64",
    },
    bundling: {
      goBuildFlags: ["-tags lambda.norpc"],
    },
  });

But it's will depend to backend variable in backend.ts file. I think it's bad idea, defineFunction is using new Error().stack as callerStack in FunctionFactory. Should I use same new Error().stack as a scope of defineFunctionWithAnyLanguage ?

@josefaidt
Copy link
Contributor

Hey @MarlonJD you can define a union on the defineFunction argument to either take the existing props type or a callback like (scope: Construct) => lambda.Function, but this scope will be passed at buildtime from the defineFunction factory.

the props here will need adjustments
https://github.com/aws-amplify/amplify-backend/blob/main/packages/backend-function/src/factory.ts#L41
https://github.com/aws-amplify/amplify-backend/blob/main/packages/backend-function/src/factory.ts#L101

and the scope would be passed to the AmplifyFunction construct
https://github.com/aws-amplify/amplify-backend/blob/main/packages/backend-function/src/factory.ts#L235

We wouldn't want to necessarily change the scope but change how "props" (in this case, the callback) is called with the existing scope to initialize within the Amplify context

@MarlonJD MarlonJD force-pushed the feature/custom-runtimes-to-defineFunction branch from 9a96a36 to bb5e0df Compare June 3, 2024 22:25
@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jun 3, 2024

@josefaidt I changed PR to this:

 const lambda = defineFunction({
  entry: './test-assets/default-lambda/handler.ts',
  runtime: 16,
});

or

const lambda = defineFunction((scope) => {
  return new NodejsFunction(scope, 'customFunction', {
    entry:
      './packages/backend-function/src/test-assets/default-lambda/handler.ts',
  });
});

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, npm ci && export CI=1 && npx ampx pipeline-deploy --branch BRANCH_NAME --app-id AMPLIFY_APP_ID will do the trick for python.

What are you thinking about this changes? If it's okay, I'll try to write e2e tests, can you guide me again?

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jun 9, 2024

I hope you can review this changes @edwardfoyle

Copy link
Contributor

@edwardfoyle edwardfoyle left a 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.

.changeset/flat-chefs-refuse.md Outdated Show resolved Hide resolved
packages/backend-function/src/factory.ts Outdated Show resolved Hide resolved
packages/backend-function/src/factory.ts Outdated Show resolved Hide resolved
packages/backend-function/src/factory.ts Outdated Show resolved Hide resolved
@MarlonJD
Copy link
Contributor Author

@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?

@MarlonJD MarlonJD requested a review from edwardfoyle June 13, 2024 23:54
@MarlonJD
Copy link
Contributor Author

@edwardfoyle Hey there, I hope you can review new changes

@edwardfoyle
Copy link
Contributor

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.

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jul 4, 2024

@josefaidt @edwardfoyle Hey there! I have good news about python functions. There is already a local paramater in BundlingOptions. It gives us to bundle without docker. I just found a way to bypass docker build without a modify packages, here is the usage:

import { defineFunction } from "@aws-amplify/backend";
import { DockerImage, Duration } from "aws-cdk-lib";
import { Code, Function, Runtime } from "aws-cdk-lib/aws-lambda";
import { execSync } from "child_process";
import * as path from "path";

const functionDir = path.join(__dirname);

export const sayHelloPythonFunctionHandler = defineFunction(
  (scope) =>
    new Function(scope, "say-hello-python-xderawfe", {
      handler: "index.handler",
      runtime: Runtime.PYTHON_3_9,
      timeout: Duration.seconds(20),
      code: Code.fromAsset(functionDir, {
        bundling: {
          image: DockerImage.fromRegistry("dummy"),
          local: {
            tryBundle(outputDir: string) {
              // if you want to use another python version, make default this ie: pyvenv global 3.12
              // if you dont have python3.9 installed, you can use pyenv to install it from Amplify Hosting build settings
              // from amplify.yaml, you can add this command to install python3.9 in the build settings
              // ie: pyenv install 3.12, pyenv global 3.12

              // Add pyenv command here, if you have more than one python version installed in your functions

              execSync(
                `python3 -m pip install -r ${path.join(functionDir, "requirements.txt")} -t ${path.join(outputDir)} --platform manylinux2014_x86_64 --only-binary=:all:`
              );
              execSync(`rsync -rLv ${functionDir}/* ${path.join(outputDir)}`);
              return true;
            },
          },
        },
      }),
    })
);

I just tried any it's works. I used pure Function, for this reason we can use any language supported from Lambda, f you can bundle it correctly without any package use.
We can add samples to docs how people can use functions.
Now Go and Python functions ready to use, I hope we can move on to the PR and tests.

@MarlonJD
Copy link
Contributor Author

@edwardfoyle Any updates?

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Aug 9, 2024

PTAL

@Srakai
Copy link

Srakai commented Aug 11, 2024

Great work @MarlonJD! Hope this gets merged quickly as currently im trying to achieve same but in a really whacky way

@MarlonJD
Copy link
Contributor Author

Hey @josefaidt, I commented how we can use python functions without docker build, but there is no local parameter in PythonFunction in @aws-cdk/aws-lambda-python-alpha, so I just showed example on Pure Function in aws-cdk-lib/aws-lambda to create python function with local parameter, it's enough to use python function.

What are you thinking about this solution to using python functions like this, should we add local parameter to @aws-cdk/aws-lambda-python-alpha or is it okay to use like this?

@mrubelmann
Copy link

Any updates on this PR? @edwardfoyle, has @MarlonJD addressed your request? It would be super, super helpful to get this merged!

@guikcd
Copy link

guikcd commented Oct 4, 2024

@josefaidt @edwardfoyle Hey there! I have good news about python functions. There is already a local paramater in BundlingOptions. It gives us to bundle without docker. I just found a way to bypass docker build without a modify packages, here is the usage:

import { defineFunction } from "@aws-amplify/backend";
import { DockerImage, Duration } from "aws-cdk-lib";
import { Code, Function, Runtime } from "aws-cdk-lib/aws-lambda";
import { execSync } from "child_process";
import * as path from "path";

const functionDir = path.join(__dirname);

export const sayHelloPythonFunctionHandler = defineFunction(
  (scope) =>
    new Function(scope, "say-hello-python-xderawfe", {
      handler: "index.handler",
      runtime: Runtime.PYTHON_3_9,
      timeout: Duration.seconds(20),
      code: Code.fromAsset(functionDir, {
        bundling: {
          image: DockerImage.fromRegistry("dummy"),
          local: {
            tryBundle(outputDir: string) {
              // if you want to use another python version, make default this ie: pyvenv global 3.12
              // if you dont have python3.9 installed, you can use pyenv to install it from Amplify Hosting build settings
              // from amplify.yaml, you can add this command to install python3.9 in the build settings
              // ie: pyenv install 3.12, pyenv global 3.12

              // Add pyenv command here, if you have more than one python version installed in your functions

              execSync(
                `python3 -m pip install -r ${path.join(functionDir, "requirements.txt")} -t ${path.join(outputDir)} --platform manylinux2014_x86_64 --only-binary=:all:`
              );
              execSync(`rsync -rLv ${functionDir}/* ${path.join(outputDir)}`);
              return true;
            },
          },
        },
      }),
    })
);

I just tried any it's works. I used pure Function, for this reason we can use any language supported from Lambda, f you can bundle it correctly without any package use. We can add samples to docs how people can use functions. Now Go and Python functions ready to use, I hope we can move on to the PR and tests.

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 .amplify directory to force the re-generation of the bundle.

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Oct 5, 2024

@guikcd oh it's a good review, let me check how nodejs changes triggers, maybe we can found fix about it

@MarlonJD
Copy link
Contributor Author

@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?

Copy link
Member

@sobolk sobolk left a 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:

  1. 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.
  2. The error messages. For these please keep in mind that they need that docs PR to stabilize, so this might not be final iteration.

.changeset/long-berries-greet.md Outdated Show resolved Hide resolved
Comment on lines 95 to 103
throw new AmplifyUserError(
'FunctionProviderError',
{
message: e instanceof Error ? e.message : JSON.stringify(e),
// TODO better resolution
resolution: 'Ensure that function provider is working.',
},
e instanceof Error ? e : undefined
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new AmplifyUserError(
'FunctionProviderError',
{
message: e instanceof Error ? e.message : JSON.stringify(e),
// TODO better resolution
resolution: 'Ensure that function provider is working.',
},
e instanceof Error ? e : undefined
);
throw new AmplifyUserError(
'CustomFunctionProviderError',
{
message: e instanceof Error ? e.message : JSON.stringify(e),
resolution: "Ensure that callback passed to 'defineFunction' executes without error. See https://docs.amplify.aws/react/build-a-backend/functions/custom-functions for more details.",
},
e instanceof Error ? e : undefined
);

@josefaidt @Amplifiyer please take a look at proposed wording here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are a few examples of errors that would fall into this category? The wording seems fine but I'm concerned with whether it would be tough to debug

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that function passed to defineFunction can throw.

Including:

  1. Bundling errors,
  2. Invalid properties/validation errors
  3. Mistakes in function itself.

The underlying error will be printed in the console.

  1. We copy it's message
  2. We include actual root cause as cause, it is reported in std streams (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause ).

The idea here is that the troubleshooting section can be expanded on the docs page without changing this code. I.e. information about root cause and the docs page should be enough to investigate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. technically docker errors could be handled this way as well, but for now in PR they get their own category as we expect it to be common problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • When we are wrapping the error, we should provide a different error message than the one being wrapped, specially as it is being passed in the cause. This error message should describe what happened at the call site e.g. "Failed to instantiate the custom function provider", or something like that.
  • Ensure that callback passed to 'defineFunction' executes without error, are we conflating the concept of callback here?

Comment on lines 84 to 90
throw new AmplifyUserError(
'FunctionBundlingDockerError',
{
message: e.message,
// TODO better resolution
resolution:
'Ensure that docker is present and works correctly. See https://some.link.about.docker.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new AmplifyUserError(
'FunctionBundlingDockerError',
{
message: e.message,
// TODO better resolution
resolution:
'Ensure that docker is present and works correctly. See https://some.link.about.docker.',
throw new AmplifyUserError(
'CustomFunctionProviderDockerRequiredError',
{
message: e.message,
resolution:
'See https://docs.amplify.aws/react/build-a-backend/functions/custom-functions for more details about current limitations and troubleshooting steps.',

@josefaidt @Amplifiyer please take a look at proposed wording here. The goal here is to:

  • refrain from stating docker support in error message/resolution in case we find a way to support it (avoid hardcoding).
  • point to web page that we can edit out of bound with most recent information about the state of support. (providing support should not involve code change in this repo, therefore need for decoupling).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have that callout at the top of this page that calls out limitations with docker, but perhaps this warrants more clarity at the top of that page -- prereqs, etc.

There's a note I left on the docs PR to (separately) create a troubleshooting dedicated to docker errors which we can use to add more color for support

@MarlonJD
Copy link
Contributor Author

Hi @sobolk, I’ve implemented the changes based on your suggestions. Please let me know if there’s anything else that needs adjustment.

@sobolk
Copy link
Member

sobolk commented Dec 31, 2024

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

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jan 1, 2025

Hi @sobolk, I've added the missing imports to address the issue.

@sobolk
Copy link
Member

sobolk commented Jan 2, 2025

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 lint check? npm run lint:fix should do it.

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jan 3, 2025

Hi @sobolk,

I’ve resolved the linting errors and committed the changes. I ran npm run lint, and it now reports: All matched files use Prettier code style!

@sobolk
Copy link
Member

sobolk commented Jan 7, 2025

@MarlonJD could you please merge latest main to your branch? it should fix remaining checks.

@sobolk
Copy link
Member

sobolk commented Jan 7, 2025

And please npm run lint:fix one more time.

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jan 7, 2025

Hi @sobolk, I'm getting this error after npm run lint:fix:

> eslint --max-warnings 0 . && tsx scripts/check_package_json.ts && prettier --check . packages/backend-function


/Users/marlonjd/Developer/mobile/amplify/amplify-backend/packages/platform-core/src/cdk/enum_converters.ts
  97:66  error  Invalid type "never" of template literal expression  @typescript-eslint/restrict-template-expressions

✖ 1 problem (1 error, 0 warnings)

@sobolk
Copy link
Member

sobolk commented Jan 7, 2025

Hi @sobolk, I'm getting this error after npm run lint:fix:

> eslint --max-warnings 0 . && tsx scripts/check_package_json.ts && prettier --check . packages/backend-function


/Users/marlonjd/Developer/mobile/amplify/amplify-backend/packages/platform-core/src/cdk/enum_converters.ts
  97:66  error  Invalid type "never" of template literal expression  @typescript-eslint/restrict-template-expressions

✖ 1 problem (1 error, 0 warnings)

It's likely related to workspace being in inconsistent state after main merge.

You can try the following:

npm run clean && npm install && npm run build && npm run lint:fix

Or just run npx prettier --write . in the repository root.

@MarlonJD
Copy link
Contributor Author

MarlonJD commented Jan 7, 2025

Hi @sobolk,

Thank you for pointing this out. I followed your recommendations, which resolved the inconsistency, and I’ve committed the necessary changes.

I also want to express my gratitude for your guidance and support—it’s much appreciated!

@sobolk
Copy link
Member

sobolk commented Jan 7, 2025

Hi @sobolk,

Thank you for pointing this out. I followed your recommendations, which resolved the inconsistency, and I’ve committed the necessary changes.

I also want to express my gratitude for your guidance and support—it’s much appreciated!

Thank you for contributing this @MarlonJD .

@sobolk sobolk merged commit 3f521c3 into aws-amplify:main Jan 7, 2025
143 checks passed
@sofferor
Copy link

Happy to hear it's finally possible to define a python function!
I also saw you add documentation about it in:
https://docs.amplify.aws/nextjs/build-a-backend/functions/custom-functions/#python

Can you also please add documentation about how a Python function can interact with the Amplify Data?
For example how to create/update/read/delete a table in Amplify Data.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.