Add Templating for Configmaps, Default Values file #647

ewkoch · 2024-10-23T20:30:27Z

Pull Request Summary

Add templating for configmaps so that they don't have to be built separately. Aiming to comment a default values.yaml file so we can clearly show what needs to be configured for working deployment.

Test Plan and Usage Guide

Need to test/deploy from scratch prior to merge.

ewkoch · 2024-10-23T20:31:35Z

charts/model-engine/templates/_helpers.tpl

+      secretKeyRef:
+        name: {{ .Values.redis.authSecret }}
+        key: auth_token
+  {{- end }}


Need refactor/replace above, this should come from secret. Also need to verify this is actually used. Vaguely remember seeing that it was unnecessary (because we wind up putting the redis auth token in the fully specified URL in AWS secrets manager instead)

Yes let's apply this change

ewkoch · 2024-10-23T20:32:52Z

charts/model-engine/templates/aws_config_map.yaml

@@ -20,6 +20,9 @@ data:
    [profile {{ $profileName }}]
    role_arn = {{ index $annotations "eks.amazonaws.com/role-arn" }}
    web_identity_token_file = /var/run/secrets/eks.amazonaws.com/serviceaccount/token
+    [profile {{ $.Values.serviceAccount.sqsProfileName }}]
+    role_arn = {{ index $annotations "eks.amazonaws.com/role-arn" }}
+    web_identity_token_file = /var/run/secrets/eks.amazonaws.com/serviceaccount/token


Ideally consolidate this to one profile- need to verify that the SQS profile is assumable / check whether it has to match $profileName

ewkoch · 2024-10-23T20:33:11Z

charts/model-engine/templates/inference_framework_config.yaml

  labels:
    product: common
    team: infra
  annotations:
-    "helm.sh/hook": pre-install
+    "helm.sh/hook": pre-install,pre-upgrade


without this you can't update the vllm tags on upgrades

ah, internally, we have a separate upgrade process for vllm that doesn't require install helm charts. If you're interested, we can socialize that a bit better

ewkoch · 2024-10-23T20:33:21Z

charts/model-engine/templates/inference_framework_config.yaml

-  vllm_batch_v2: "latest"
+  vllm: "{{ .Values.vllmTag }}"
+  vllm_batch:  "{{ .Values.vllmTag }}"
+  vllm_batch_v2:  "{{ .Values.vllmTag }}"


nice to specify / could probably default to latest if not set

these are different images so we would want vllmTag, vllmBatchTag, vllmBatchV2Tag; and yes let's default to latest

ewkoch · 2024-10-23T20:34:00Z

charts/model-engine/templates/istio-virtualservice.yaml

-  {{- range .Values.virtualservice.gateways }}
-    - {{ . | quote }}
-  {{- end }}
+    - {{ $.Values.global.networking.internalGateway }}


im open to scrapping this- this was part of a DNS refactor we did to make specification of virtual services simpler

curious to see what the rest of the dns refactor changes look like

i made the rest of the refactor- it helps when model engine is a subchart to prevent duplication of changing the core domain a bunch of places

ewkoch · 2024-10-23T20:34:55Z

charts/model-engine/templates/service_config_map.yaml

-    env: {{ .Values.context | quote }}
-    {{- with .Values.config.values.infra }}
-    {{- range $key, $value := . }}
-    {{ $key }}: {{ $value | quote }}


This broke for some reason, I think related to how helm / go templating was handling certain types. I also don't like it because it doesn't clarify what actually needs to be in the config

imo we shouldn't hardcode the values that go into launch_service_config and infra_service_config; doing so introduces a coupling between the helm chart and the model engine code, e.g. if the values that need to go in either of the two configs changes. Probably good to see what ended up breaking though and fixing that

i would prefer a single source of truth for configuring the deployment of a new instance- manually changing the yaml files is also a coupling between config and code, and similarly error prone-

i think there's probably a bigger refactor necessary either way to reduce the complexity of interacting with those configs

can pull your changes to service/infra configs to a separate file for now for documentation purpose as we figure out a proper way to do this

ewkoch · 2024-10-23T20:39:47Z

charts/model-engine/templates/service_config_map.yaml

+          "infra.scale.com/team": "${team}",
+          "infra.scale.com/contact": "{{ .Values.contactEmail }}",
+          "infra.scale.com/customer": "AllCustomers",
+          "infra.scale.com/financialOwner": "{{ .Values.contactEmail}}",


need to template "infra.scale.com" to something or allow these as additional tags set in the values.yaml

ewkoch · 2024-10-23T20:40:44Z

charts/model-engine/templates/service_template_config_map.yaml

+          # {{- if empty $node_selector }}
+          # nodeSelector:
+          # {{- end }}
+          #   k8s.amazonaws.com/accelerator: ${GPU_TYPE}


will re-add this, just serves as a note that need to add documentation that this tag must be set on nodegroups

ewkoch · 2024-10-23T20:40:54Z

charts/model-engine/templates/service_template_config_map.yaml

          tolerations:
            - key: "nvidia.com/gpu"
              operator: "Exists"
              effect: "NoSchedule"
+            - key: "gpu_a100_multi"
+              operator: "Exists"
+              effect: "NoSchedule"


hack, need to delete

ewkoch · 2024-10-23T20:41:13Z

charts/model-engine/templates/service_template_config_map.yaml

@@ -522,6 +525,7 @@ data:
        loadBalancer:
          simple: LEAST_REQUEST
  {{- end }}
+  {{- if and (.Capabilities.APIVersions.Has "autoscaling.k8s.io/v1") (.Values.autoscaling.vertical.enabled) }}


this is important if you don't have autoscaling

ewkoch · 2024-10-23T20:41:56Z

charts/model-engine/values.yaml

-  node-lifecycle: normal
-nodeSelector:
-  node-lifecycle: normal
+# balloonNodeSelector:


note- need to build out a good default values.yaml with comments specifying the purpose of each configuration option.

seanshi-scale · 2024-10-23T22:50:34Z

charts/model-engine/templates/service_account_inference.yaml

@@ -1,25 +0,0 @@
-{{- if and (.Values.serviceTemplate) (.Values.serviceTemplate.createServiceAccount) (.Values.serviceTemplate.serviceAccountAnnotations) (.Values.serviceTemplate.serviceAccountName) (.Values.config.values.launch.endpoint_namespace)}}


is there a reason we're removing this? seems like we could just add some more conditions to the if to achieve the same effect

yea i'm fixing this- didn't realize this "createServiceAccount" was just for the inference account, which i was getting a naming conflict with so i removed for a quick fix

just updating with explicit naming, since we create another model-engine service account by default

dmchoiboi · 2024-11-01T19:08:20Z

charts/model-engine/templates/istio-virtualservice.yaml

@@ -1,24 +1,20 @@
-{{- if .Values.virtualservice.enabled -}}
+{{- if .values.virtualService.enabled -}}


was Values => values intentional?

dmchoiboi · 2024-11-01T19:15:14Z

charts/model-engine/values.yaml

+  port: 80
+
+# Creates istio virtual services for the Model Engine using the global domain nome and gateway specified below
+virtualService:


i think this is a backwards incompatible change; can we leave this as virtualservice

dmchoiboi · 2024-11-01T21:05:04Z

charts/model-engine/values.yaml

-  enabled: false
-redis:
-  auth:
+# If specified, will override the name of the deployed services


Thanks for the comprehensive documentation here. It's very much appreciated.

charts/model-engine/templates/service_config_map.yaml

Initial commit

21555e8

ewkoch marked this pull request as draft October 23, 2024 20:30

ewkoch commented Oct 23, 2024

View reviewed changes

seanshi-scale reviewed Oct 23, 2024

View reviewed changes

Updates to remove hacks and add default values file

c681522

ewkoch marked this pull request as ready for review November 1, 2024 18:58

ewkoch changed the title ~~[WIP] Add Templating for Configmaps~~ Add Templating for Configmaps, Default Values file Nov 1, 2024

dmchoiboi reviewed Nov 1, 2024

View reviewed changes

seanshi-scale reviewed Nov 1, 2024

View reviewed changes

charts/model-engine/templates/service_config_map.yaml Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Templating for Configmaps, Default Values file #647

Add Templating for Configmaps, Default Values file #647

ewkoch commented Oct 23, 2024

ewkoch Oct 23, 2024 •

edited

Loading

dmchoiboi Oct 30, 2024

ewkoch Oct 23, 2024

ewkoch Oct 23, 2024

dmchoiboi Nov 1, 2024

ewkoch Oct 23, 2024 •

edited

Loading

dmchoiboi Oct 30, 2024

ewkoch Nov 1, 2024

ewkoch Oct 23, 2024

dmchoiboi Oct 30, 2024

ewkoch Nov 1, 2024

ewkoch Oct 23, 2024

seanshi-scale Oct 23, 2024

ewkoch Nov 1, 2024

dmchoiboi Nov 1, 2024

ewkoch Oct 23, 2024

ewkoch Oct 23, 2024

ewkoch Oct 23, 2024

ewkoch Oct 23, 2024

ewkoch Oct 23, 2024

seanshi-scale Oct 23, 2024

ewkoch Nov 1, 2024 •

edited

Loading

dmchoiboi Nov 1, 2024

dmchoiboi Nov 1, 2024

dmchoiboi Nov 1, 2024

		@@ -1,25 +0,0 @@
		{{- if and (.Values.serviceTemplate) (.Values.serviceTemplate.createServiceAccount) (.Values.serviceTemplate.serviceAccountAnnotations) (.Values.serviceTemplate.serviceAccountName) (.Values.config.values.launch.endpoint_namespace)}}

		@@ -1,24 +1,20 @@
		{{- if .Values.virtualservice.enabled -}}
		{{- if .values.virtualService.enabled -}}

Add Templating for Configmaps, Default Values file #647

Are you sure you want to change the base?

Add Templating for Configmaps, Default Values file #647

Conversation

ewkoch commented Oct 23, 2024

Pull Request Summary

Test Plan and Usage Guide

ewkoch Oct 23, 2024 • edited Loading

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

ewkoch Oct 23, 2024 • edited Loading

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

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

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

Choose a reason for hiding this comment

ewkoch Nov 1, 2024 • edited Loading

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

ewkoch Oct 23, 2024 •

edited

Loading

ewkoch Oct 23, 2024 •

edited

Loading

ewkoch Nov 1, 2024 •

edited

Loading