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

Default to ARM64 when running on Arm hosts #4730

Closed
wants to merge 1 commit into from

Conversation

AGSaidi
Copy link
Member

@AGSaidi AGSaidi commented Feb 17, 2023

If the function architecture isn't specified and we're running on an Arm platform assume we should be trying to run the function for ARM64

Fixes #4708

Which issue(s) does this change fix?

#4708

Why is this change necessary?

Today if the architecture isn't set on an Arm based platform sam-cli tries to use the x86 runner which won't work

How does it address the issue?

If the architecture isn't set and the host in Arm (linux-aarch64 or mac os-arm64) it assumes arm64 is the intent

What side effects does this change have?

When running local invoke on a Arm platform if the function architecture isn't specified it defaults to the architecture of host

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • [ NA] Add input/output type hints to new functions/methods
  • [NA ] Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • [NA] Write/update integration tests
  • [NA] Write/update functional tests if needed
  • make pr passes
  • [NA] make update-reproducible-reqs if dependencies were changed
  • [NA] Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@AGSaidi AGSaidi requested a review from a team as a code owner February 17, 2023 15:13
@AGSaidi AGSaidi requested review from hawflau and torresxb1 February 17, 2023 15:13
@github-actions github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Feb 17, 2023
If the function architecture isn't specified and we're running on an Arm
platform assume we should be trying to run the function for ARM64

Fixes aws#4708
@jfuss
Copy link
Contributor

jfuss commented Feb 17, 2023

Left a comment on the Discussion post, but I don't think this is the right direction for SAM CLI to be taking.

@lucashuy lucashuy removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Feb 17, 2023
@lucashuy
Copy link
Contributor

Thanks for raising this PR, the default behaviour for Cloudformation/Lambda is to set the architecture to x86 if none was provided. Changing the default behaviour to be set depending on the host machine's architecture might introduce some breaking changes for customers. We'll have to discuss this with the team to see where to go with this.

@mndeveci
Copy link
Contributor

mndeveci commented Mar 8, 2023

Thanks for your PR!

As it is been discussed above an in the discussion thread, setting Lambda's architecture has no relation between developer machine's architecture. Moreover, this might lead to an invalid state where your local invocation will succeed but your actual deployment would fail due to different architectures that is been changed on the fly for sam local commands.

We would recommend using sam sync until the actual root cause have been identified and fixed.

Appreciate your understanding!

@mndeveci mndeveci closed this Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants