New Decorator for Architecture Type for all Addons and Graviton Builder Classes && Gen AI Builder and Team #846

elamaran11 · 2023-09-21T20:45:00Z

Issue #, if available:

Description of changes:
Generative AI and Graviton Builder Classes

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

shapirov103

@elamaran11 great effort and there is some work we should perform before exposing it to the customers. Please see my comments.

lib/builders/graviton-builder.ts

shapirov103 · 2023-09-22T18:54:11Z

lib/builders/graviton-builder.ts

+                    version: options.kubernetesVersion,
+                    instanceTypes: [new ec2.InstanceType(`${options.instanceClass}.${options.instanceSize}`)],
+                    amiType: eks.NodegroupAmiType.AL2_ARM_64,
+                    desiredSize: 3,


If we are to move it beyond just a demo, these should be configurable. We need to add something like withNodeGroupOptions(options: Partial<ManagedNodeGroupProps>) to the builder to allow any kind of an override. The approach is similar to the one I recommended to Bhavye on his builder.

Why desiredSize, min, max are left hardcoded and not leveraging the potentially passed value from the options?

My recommendation is to use default MngClusterProviderProps and overwrite them with the supplied. That will be easier to implement and provides the best flexibility. In the current implementation it is more of a demo rather than a production ready builder.

shapirov103 · 2023-09-22T18:55:54Z

lib/builders/genai-builder.ts

+import { NestedStack, NestedStackProps } from 'aws-cdk-lib';
+import { Construct } from 'constructs';
+
+export class GenAIBuilder extends BlueprintBuilder {


I am unclear on what specifically makes this builder a GenAI builder other than usage tracking? Is there anything that we should add specific to genAI like enable a particular instance types or deploy an example workload - something?

docs/builders/genai-builder.md

shapirov103

Looks great. Some comments.
Please also run make lint - there are many warnings

lib/builders/graviton-builder.ts

lib/utils/architecture-utils.ts

elamaran11 · 2023-09-27T17:12:56Z

Looks great. Some comments. Please also run make lint - there are many warnings

Fixed all issues pointed in this iteration.

elamaran11 · 2023-09-27T17:32:43Z

@shapirov103 All issues fixed.

shapirov103

@elamaran11 looking great. A couple of comments left, please take a look.

@shapirov103 Agreed to the comment, fixed it. Please take a look.

shapirov103 · 2023-10-02T18:15:32Z

lib/builders/graviton-builder.ts

+                    version: options.kubernetesVersion,
+                    instanceTypes: [new ec2.InstanceType(`${options.instanceClass}.${options.instanceSize}`)],
+                    amiType: eks.NodegroupAmiType.AL2_ARM_64,
+                    desiredSize: 3,


Why desiredSize, min, max are left hardcoded and not leveraging the potentially passed value from the options?

shapirov103 · 2023-10-02T18:17:37Z

lib/builders/graviton-builder.ts

+                    version: options.kubernetesVersion,
+                    instanceTypes: [new ec2.InstanceType(`${options.instanceClass}.${options.instanceSize}`)],
+                    amiType: eks.NodegroupAmiType.AL2_ARM_64,
+                    desiredSize: 3,


My recommendation is to use default MngClusterProviderProps and overwrite them with the supplied. That will be easier to implement and provides the best flexibility. In the current implementation it is more of a demo rather than a production ready builder.

elamaran11 · 2023-10-03T16:52:31Z

@shapirov103 Added Gen AI Class and Team. Please check the approach

shapirov103

A couple of high level comments for now on the GenAI design.

shapirov103 · 2023-10-03T17:43:58Z

lib/teams/geneai-team/genai-team.ts

+ *Sets IRSA for access to bedrock with required IAM policy along with creating a namespace.
+*/
+
+export class GenAITeam extends ApplicationTeam {


since it is only setting up bedrock access, should we rename it to bedrock access or something more specific to the use case? We will have a bunch of genAI services right after RI, so we will have to eventually support them as well and it sounds like it will be a separate team per service?

shapirov103 · 2023-10-03T17:45:35Z

lib/teams/geneai-team/genai-team.ts

+    this.genAITeam = props;
+  }
+
+  setup(clusterInfo: ClusterInfo): void {


Does it work? I see this code will clash with the setupServiceAccount method from the application team. I don't think you need to set it up like this at all. All you need to do is pass the serviceaccountname and serviceAccountPolicies to with the props to the Application team and its default setup method should take care of the IRSA stuff.

elamaran11 · 2023-10-04T00:26:37Z

@shapirov103 PR is updated with teams supporting function as a property to teamProps and have also supplied an example which passes function as property for deploying a workload manifest to teamProps. Please review this.

shapirov103

Please see my comment

shapirov103 · 2023-10-05T19:13:43Z

lib/builders/bedrock-builder.ts

+     * This method helps you prepare a blueprint for setting up EKS cluster with 
+     * usage tracking addon
+     */
+    public static builder(): BedrockBuilder {


aren't we supposed to add a bedrock team here ootb? e.g. take those props with the function and add the team. Or another alternative is to add a method like addBedrockTeam() to make it explicit.
The way it stands now, there is no differentiator for this builder for the customers.

elamaran11 · 2023-10-05T19:56:07Z

@shapirov103 Makesense and i missed this, i fixed it now and all good. Please check and run e2e.

shapirov103

One small comment.

because of the quite significant surface area of chages, please test with patterns. Does it work for make test-all?

shapirov103 · 2023-10-05T20:48:50Z

lib/utils/architecture-utils.ts

+function addAddonArch(addonName: string, architecture: ArchType) {
+  if (addonArchitectureMap.has(addonName)) {
+    const value = addonArchitectureMap.get(addonName);
+    if (value !== undefined) {


Based on my observation:
this code when executed multiple times will keep adding supported archs to the global array. So if in my app I instantiate an addon 100 times, this will have at least 100 entries.

Let's add a check to add value only if it is not already there.

shapirov103 · 2023-10-05T21:14:33Z

/do-e2e-tests

aws-ia-ci

end to end tests passed

shapirov103

LGTM - great work!

Generative AI and Graviton Builder Classes

e2a2fcc

elamaran11 requested review from aws-ia-ci, parkand1, shapirov103, youngjeong46 and a team as code owners September 21, 2023 20:45

shapirov103 reviewed Sep 22, 2023

View reviewed changes

elamaran11 added 2 commits September 26, 2023 17:17

Adding support for Architectures

495ca4e

Adding support for Architectures

7322f48

elamaran11 changed the title ~~Generative AI and Graviton Builder Classes~~ New Decorator for Architecture Type for all Addons and Generative AI and Graviton Builder Classes Sep 26, 2023

elamaran11 added 2 commits September 26, 2023 17:34

Adding support for Architectures

0e09aaa

Adding support for Architectures

fefcab2

shapirov103 reviewed Sep 27, 2023

View reviewed changes

lib/builders/graviton-builder.ts Show resolved Hide resolved

lib/utils/architecture-utils.ts Outdated Show resolved Hide resolved

lib/utils/architecture-utils.ts Outdated Show resolved Hide resolved

lib/utils/architecture-utils.ts Outdated Show resolved Hide resolved

Adding support for Architectures - PR Issues

426ac8d

Adding support for Architectures - PR Issues

74e89c0

elamaran11 added 7 commits September 27, 2023 13:59

Adding support for Architectures - PR Issues

6ba7d09

Adding support for Architectures - PR Issues

d03cc3e

Changing to Class Decorator

113fd4b

Generative AI and Graviton Builder Classes

94d1fbc

Generative AI and Graviton Builder Classes

ea75e7b

Generative AI and Graviton Builder Classes

f8979cf

Generative AI and Graviton Builder Classes

cb48788

shapirov103 reviewed Oct 2, 2023

View reviewed changes

Generative AI and Graviton Builder Classes - PR

63c8012

elamaran11 changed the title ~~New Decorator for Architecture Type for all Addons and Generative AI and Graviton Builder Classes~~ New Decorator for Architecture Type for all Addons and Generative AI & Graviton Builder Classes Oct 3, 2023

elamaran11 added 3 commits October 3, 2023 10:16

Generative AI and Graviton Builder Classes - PR

fe166ea

Generative AI and Graviton Builder Classes - PR

57f2a8f

Removing Gen AI Builder class and doc

b096546

Removing Gen AI Builder class and doc

318efc0

elamaran11 changed the title ~~New Decorator for Architecture Type for all Addons and Generative AI & Graviton Builder Classes~~ New Decorator for Architecture Type for all Addons and Graviton Builder Classes Oct 3, 2023

Adding Gen AI Builder class and doc

d308154

elamaran11 changed the title ~~New Decorator for Architecture Type for all Addons and Graviton Builder Classes~~ New Decorator for Architecture Type for all Addons and Graviton Builder Classes && Gen AI Builder and Team Oct 3, 2023

Adding Gen AI Builder class and doc

cf0130f

shapirov103 reviewed Oct 3, 2023

View reviewed changes

elamaran11 added 6 commits October 3, 2023 16:42

Adding Gen AI Builder class and doc

8069405

Adding Gen AI Builder class and doc

f4f68ae

Adding Gen AI Builder class and doc

c0ffd50

Generative AI and Graviton Builder Classes

fb01aac

Generative AI and Graviton Builder Classes PR Comments

d84b9ab

Generative AI and Graviton Builder Classes PR Comments

571dbea

elamaran11 added 2 commits October 4, 2023 13:50

Generative AI and Graviton Builder Classes PR Comments

f48ffd8

Generative AI and Graviton Builder Classes PR Comments

1cdfec4

shapirov103 reviewed Oct 5, 2023

View reviewed changes

PR Fix for addBedRockTeam()

885f22c

shapirov103 reviewed Oct 5, 2023

View reviewed changes

Adding Gen AI Builder class and doc

78d12cb

aws-ia-ci approved these changes Oct 5, 2023

View reviewed changes

shapirov103 approved these changes Oct 6, 2023

View reviewed changes

shapirov103 merged commit 9db7c93 into main Oct 6, 2023

shapirov103 deleted the fix/genAIandGravitonBuilderClasses branch October 6, 2023 01:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Decorator for Architecture Type for all Addons and Graviton Builder Classes && Gen AI Builder and Team #846

New Decorator for Architecture Type for all Addons and Graviton Builder Classes && Gen AI Builder and Team #846

elamaran11 commented Sep 21, 2023

shapirov103 left a comment

shapirov103 Sep 22, 2023

shapirov103 Oct 2, 2023

shapirov103 Oct 2, 2023

shapirov103 Sep 22, 2023

shapirov103 left a comment

elamaran11 commented Sep 27, 2023

elamaran11 commented Sep 27, 2023

shapirov103 left a comment •

edited by elamaran11

Loading

shapirov103 Oct 2, 2023

shapirov103 Oct 2, 2023

elamaran11 commented Oct 3, 2023

shapirov103 left a comment

shapirov103 Oct 3, 2023

shapirov103 Oct 3, 2023

elamaran11 commented Oct 4, 2023

shapirov103 left a comment

shapirov103 Oct 5, 2023

elamaran11 commented Oct 5, 2023

shapirov103 left a comment

shapirov103 Oct 5, 2023

shapirov103 commented Oct 5, 2023

aws-ia-ci left a comment

shapirov103 left a comment

New Decorator for Architecture Type for all Addons and Graviton Builder Classes && Gen AI Builder and Team #846

New Decorator for Architecture Type for all Addons and Graviton Builder Classes && Gen AI Builder and Team #846

Conversation

elamaran11 commented Sep 21, 2023

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shapirov103 left a comment

Choose a reason for hiding this comment

elamaran11 commented Sep 27, 2023

elamaran11 commented Sep 27, 2023

shapirov103 left a comment • edited by elamaran11 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elamaran11 commented Oct 3, 2023

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elamaran11 commented Oct 4, 2023

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elamaran11 commented Oct 5, 2023

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shapirov103 commented Oct 5, 2023

aws-ia-ci left a comment

Choose a reason for hiding this comment

shapirov103 left a comment

Choose a reason for hiding this comment

shapirov103 left a comment •

edited by elamaran11

Loading