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

Add Support for Graviton 2 / ARM Architectures #1057

Closed

Conversation

turingbeing
Copy link

@turingbeing turingbeing commented Oct 14, 2021

Description

This PR adds support for Graviton 2 / ARM64 Lambda's. It adds a new architectures configuration option, which defaults to x86_64. This parameter is also used for correct architecture wheel selection.

This is my first submission to Zappa, so please do CR and feedback anything I might have missed, or need to do.

AWS API Docs

GitHub Issues

Resolves: #1051

@turingbeing
Copy link
Author

@hellno @javulticat

How do we get PR's reviewed? Is there a group / person we should tag?

From experience allowing PR's to go stale discourages contributions

Copy link
Collaborator

@hellno hellno left a comment

Choose a reason for hiding this comment

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

Looks great overall, a small naming convention (I don't want to spam it on every line 😅):
Why is the parameter name architectures?
In the settings I can only select one and it is being stored as a list, but when used, you take only ever take the first object (architectures[0]).

I might be missing something here, sorry for my ignorance if that's the case.
Happy to discuss and get this merged, sorry for the CR delay

@turingbeing
Copy link
Author

turingbeing commented Oct 26, 2021

Looks great overall, a small naming convention (I don't want to spam it on every line 😅):

Why is the parameter name architectures?

In the settings I can only select one and it is being stored as a list, but when used, you take only ever take the first object (architectures[0]).

I might be missing something here, sorry for my ignorance if that's the case.

Happy to discuss and get this merged, sorry for the CR delay

Thanks for the feedback, and no worries on the delay, I just wasn't sure if I needed to tag someone / CR Group.

The only reason I used 'architectures' is to be consistent with the AWS API Terminology. I do see your point and will happily change it, I guess the same is true for having it as a List, again that was mirroring the API, but as long as I pass it in as a list, we're good.

If you agree I can go ahead and make those updates, in all honesty I wasn't too clear on what conventions I should be using, but that's all part of contributing 👍

To Summarise:

  • Remove pluralisation of architecture parameter
  • Change type from List to String

@hellno
Copy link
Collaborator

hellno commented Oct 28, 2021

Yes, that sounds great! thanks for the effort 💪🏼

@turingbeing
Copy link
Author

I'll work on this at the weekend, been busy!

@hootisfix
Copy link

@turingbeing , do you need help on this PR ?

@Nebu1eto
Copy link

I want to use this feature. How can I help this to get merged?

@turingbeing
Copy link
Author

I want to use this feature. How can I help this to get merged?

You should be able to use it from the branch without merging it, just needs some minor refactoring, but been mega busy with life and work to do it

@turingbeing
Copy link
Author

Actually I think it's here: #1123

@monkut
Copy link
Collaborator

monkut commented Aug 12, 2022

@turingbeing Looks like this would be nice to get merged in.

Do you have time to address the comments in the coming months and rebase off of master?

@monkut
Copy link
Collaborator

monkut commented Aug 12, 2022

This PR is further along, closing this one.

#1123

@monkut monkut closed this Aug 12, 2022
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.

Support for Lambda on Arm CPUs
5 participants