Zappa icon indicating copy to clipboard operation
Zappa copied to clipboard

Add Support for Graviton 2 / ARM `Architectures`

Open turingbeing opened this issue 3 years ago • 8 comments

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: https://github.com/zappa/Zappa/issues/1051

turingbeing avatar Oct 14 '21 22:10 turingbeing

@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

turingbeing avatar Oct 26 '21 14:10 turingbeing

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

turingbeing avatar Oct 26 '21 19:10 turingbeing

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

hellno avatar Oct 28 '21 11:10 hellno

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

turingbeing avatar Nov 02 '21 20:11 turingbeing

@turingbeing , do you need help on this PR ?

hootisfix avatar Jan 31 '22 12:01 hootisfix

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

Nebu1eto avatar Jul 24 '22 02:07 Nebu1eto

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 avatar Jul 24 '22 06:07 turingbeing

Actually I think it's here: https://github.com/zappa/Zappa/pull/1123

turingbeing avatar Jul 24 '22 06:07 turingbeing

@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 avatar Aug 12 '22 08:08 monkut

This PR is further along, closing this one.

https://github.com/zappa/Zappa/pull/1123

monkut avatar Aug 12 '22 08:08 monkut