feat: add option for Checkpoint_GC pod spec in task container defaults #9406

ShreyaLnuHpe · 2024-05-22T05:27:11Z

Ticket

Description

A user complained about the below issue:
Overview of the issue: We pull a lot of stuff from the experiment config into the checkpoint GC spec without really thinking it through. One thing we pull in are the cpu requests/limits from the training job.
This assumption that the experiment’s pod spec won’t work for every user. Issue shows a community user who has a GPU affinity in their pod spec which prevents their gc job from running.

In this PR we give the user the ability to tell us what their checkpoint gc pod spec looks like. We should add a checkpoint gc pod spec that will overwrite the experiments pod spec (if a user specifies a checkpoint gc pod spec) to task container defaults.

Test Plan

Added unit test coverage for overwriting the default Legacy config in the Experiment Pod Spec.
Added unit test coverage for the CheckpointGCSpec added to the master config.
Working devcluster with right value flow of the data from master config using command: det -u admin master config. Connected though Kubernetes Pods
Tested helm changes as helm chart applied the config to master using the port-forwarding.
Tested with setting the following values in the determined/tools/k8s/devcluster.yaml file

        task_container_defaults:
          checkpoint_gc_pod_spec:
            apiVersion: v1
            kind: Pod
            metadata:
              name: example-pod
            spec:
              affinity:
                nodeAffinity:
                  requiredDuringSchedulingIgnoredDuringExecution:
                    nodeSelectorTerms:
                    - matchExpressions:
                      - key: does-not-exist
                        operator: Exists

Running the below command
det e create e2e_tests/tests/fixtures/no_op/gc_checkpoints_decreasing.yaml e2e_tests/tests/fixtures/no_op/ -f
with devcluster -c tools/k8s/devcluster.yaml
Monitored logs through det master logs | grep "CHECKPOINT_GC" command

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-05-22T05:27:29Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`a5a0309`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6658b6cef46a7a0008482e9f

ShreyaLnuHpe · 2024-05-22T17:23:26Z

tools/devcluster.yaml

@@ -59,6 +59,10 @@ stages:
        enable_cors: true
        observability:
          enable_prometheus: true
+        task_container_defaults:
+          checkpoint_gc_pod_spec:
+            apiVersion: v1


Assuming we would only need the structure/outline of the pod spec to be defined

master/pkg/tasks/task_gc.go

NicholasBlaskey · 2024-05-23T14:17:02Z

master/pkg/tasks/task_gc.go

+	podSpec := g.LegacyConfig.Environment.PodSpec()
+	if g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil {
+		gcPodSpec := (*expconf.PodSpec)(g.Base.TaskContainerDefaults.CheckpointGCPodSpec)
+		podSpec = schemas.Merge(podSpec, gcPodSpec)


I don't think we should merge the pod spec from the experiment here in case it has node affinities that we won't want to add

Noted!
Replaced the code with overwriting the legacy PodSpec value

codecov · 2024-05-24T07:28:19Z

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 48.64%. Comparing base (b96ccba) to head (a5a0309).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9406      +/-   ##
==========================================
+ Coverage   48.60%   48.64%   +0.04%     
==========================================
  Files        1233     1233              
  Lines      159002   159010       +8     
  Branches     2778     2778              
==========================================
+ Hits        77283    77356      +73     
+ Misses      81545    81480      -65     
  Partials      174      174

Flag	Coverage Δ
backend	`42.29% <77.77%> (+0.13%)`	⬆️
harness	`64.02% <ø> (ø)`
web	`44.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/pkg/tasks/task_gc.go	`85.89% <100.00%> (+85.89%)`	⬆️
master/cmd/determined-master/root.go	`49.70% <50.00%> (+<0.01%)`	⬆️
master/pkg/model/task_container_defaults.go	`84.51% <66.66%> (-0.36%)`	⬇️

... and 1 file with indirect coverage changes

NicholasBlaskey

Code changes look great, nice work. We need a few additional extra changes to

We should update the Helm chart to allow users to set this when deploying Determined on K8s with Helm

cpu_pod_spec would be a good example of how to go about this

determined/helm/charts/determined/templates/master-config.yaml

Lines 225 to 227 in 3dfb9ec

    
                 {{- if .Values.taskContainerDefaults.gpuPodSpec }} 
        
                 gpu_pod_spec: {{ .Values.taskContainerDefaults.gpuPodSpec | toJson }} 
        
                 {{- end }}

doc updates. We have docs on master config / helm charts / and custom pod specs. We should update these docs to include this new feature.

determined/helm/charts/determined/templates/master-config.yaml

Line 223 in 3dfb9ec

cpu_pod_spec: {{ .Values.taskContainerDefaults.cpuPodSpec | toJson }}

determined/docs/reference/deploy/helm-config-reference.rst

Line 189 in 3dfb9ec

    
           -  ``cpuPodSpec``: Sets the default pod spec for all non-GPU tasks. See :ref:`custom-pod-specs`

determined/docs/setup-cluster/k8s/custom-pod-specs.rst

Line 104 in 3dfb9ec

:ref:`helm-config-reference` under ``taskContainerDefaults.cpuPodSpec`` and

For any user facing change we need a release note. We have some instructions on how to add them here

https://github.com/determined-ai/determined/tree/main/docs/release-notes

NicholasBlaskey · 2024-05-24T17:46:01Z

master/pkg/model/task_container_defaults_test.go

@@ -494,6 +521,13 @@ func TestPodSpecsDefaultMerging(t *testing.T) {
 				},
 			},
 		},
+		CheckpointGCPodSpec: &k8sV1.Pod{


does this test fail without this change?

This is testing merging into expconf so I don't know if we need this change

You are right. TestPodSpecsDefaultMerging doesn't need CheckpointGCPodSpec while merging.
Tests do not fail without this.

NicholasBlaskey · 2024-05-24T17:47:34Z

tools/devcluster.yaml

@@ -59,6 +59,10 @@ stages:
        enable_cors: true
        observability:
          enable_prometheus: true
+        task_container_defaults:


I would probably remove this change since this devcluster won't be on Kubernetes and most developers won't use this

if someone needs to test it they can add it themself

NicholasBlaskey · 2024-05-24T17:48:40Z

master/pkg/tasks/task_gc_test.go

+	}
+
+	for testCase, testVars := range tests {
+		t.Run(testCase, func(t *testing.T) {


this test looks great, nice job

NicholasBlaskey · 2024-05-28T13:04:27Z

docs/release-notes/feature-checkpointGC.rst

+**New Features**
+
+-  Kubernetes: Add a feature where Determined offers the users to provide custom Checkpoint GC pod spec for 
+    individual experiment. You can specify a custom ``checkpointGcPodSpec`` directly in the experiment 


You can specify a custom ``checkpointGcPodSpec`` directly in the experiment configuration file

This is for task container defaults only right and not in the experiment yaml?

NicholasBlaskey · 2024-05-28T13:05:00Z

docs/setup-cluster/k8s/custom-pod-specs.rst

+ Custom CheckpointGC Pod Specs
+*******************************
+
+Determined also provides a way to configure CheckpointGC pod specs for individual experiment. You can 


same comment about not how we don't let you configure per expeirment

NicholasBlaskey · 2024-05-28T13:08:00Z

helm/charts/determined/values.yaml

@@ -264,6 +264,10 @@ taskContainerDefaults:
  # cpuPodSpec:
  # gpuPodSpec:

+  # Configure a custom CheckpointGC pod spec for experiments. If a pod spec is defined for an 


same comment about how we don't support configuring this per experiment

NicholasBlaskey · 2024-05-28T13:12:57Z

harness/determined/deploy/gke/cli.py

@@ -280,6 +280,12 @@ def configure_helm(args: argparse.Namespace) -> None:

        helm_values["taskContainerDefaults"]["cpuPodSpec"] = cpu_pod_spec

+        checkpoint_gc_pod_spec = make_spec(


i'm a little worried this will overwrite the default value

determined/harness/determined/deploy/gke/cli.py

Line 20 in df7de93

pod_spec = {"apiVersion": "v1", "kind": "Pod"}

so when you start a cluster with a checkpoint gc spec, this will add one even if a user has not explicitly given one. This would mean we would no longer use the experiment's pod spec in the default case.

The broader point is gke/cli.py is experimental and hasn't been really been updated in like 2 years. I would avoid spending any time on this and just not make any changes to this file.

determined-ci · 2024-05-29T00:24:29Z

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

NicholasBlaskey · 2024-05-29T20:35:26Z

docs/release-notes/feature-checkpointGC.rst

+-  Kubernetes: Add a feature where Determined offers the users to provide custom Checkpoint GC pod spec.
+      This configuration is done using the ``task_container_defaults.checkpoint_gc_pod_spec`` field
+      within your ``value.yaml`` file. When a custom CheckpointGC pod spec is defined, it takes
+      precedence over the default CheckpointGC pod specifications. This provides flexibility to


I would be extra clear about what "default CheckpointGC pod specfications" means

I would mention that previous it would use the experiments pod spec but it can be overriden to use a configurable one with this field

NicholasBlaskey · 2024-05-29T20:35:53Z

docs/setup-cluster/k8s/custom-pod-specs.rst

+
+Determined also provides a way to configure CheckpointGC pod specs. This configuration is done using
+the ``task_container_defaults.checkpoint_gc_pod_spec`` field within your ``value.yaml`` file. When a
+custom CheckpointGC pod spec is defined, it takes precedence over the default CheckpointGC pod


same comment with the release note about clarifying what "default checkpointGC pod" refers to

NicholasBlaskey · 2024-05-29T20:36:44Z

docs/release-notes/feature-checkpointGC.rst

+**New Features**
+
+-  Kubernetes: Add a feature where Determined offers the users to provide custom Checkpoint GC pod spec.
+      This configuration is done using the ``task_container_defaults.checkpoint_gc_pod_spec`` field


the value.yaml expects taskContainerDefaults.checkpointGcSpec

Completely missed it. Good catch!

NicholasBlaskey · 2024-05-29T20:37:56Z

master/pkg/tasks/task_gc.go

@@ -37,10 +37,16 @@ func (g GCCkptSpec) ToTaskSpec() TaskSpec {
 	// Set Environment.
 	// Keep only the EnvironmentVariables provided by the experiment's config.
 	envVars := g.LegacyConfig.Environment.EnvironmentVariables()
+
+	podSpec := g.LegacyConfig.Environment.PodSpec()
+	if g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil {


just to be super safe here, I don't think this can be nil but inside MergeIntoExpConfig there is a nil check for it.

Suggested change

if g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil {

if g.Base.TaskContainerDefaults != nil && g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil {

As g.Base.TaskContainerDefaults is of type mismatch with nil, it will be better to create a new func to check if it is nil. Something like this:

if !g.Base.TaskContainerDefaults.IsNil() && g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil

func (c *TaskContainerDefaultsConfig) IsNil() bool { return c == nil }

Other option we have is to leverage the validate function and check for err != nil. Downside will be that err might not be empty because of other parameters being checked within that function.

err := g.Base.TaskContainerDefaults.Validate() if err != nil && g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil {

Let me know what do you think

Oh my bad since g.Base.TaskContainersDefaultConfig is not a pointer we don't need to do anything

The code was good as is. I made a mistake thinking it was a pointer.

The IsNil check won't add anything since it will always not be nil.

NicholasBlaskey · 2024-05-29T20:39:10Z

helm/charts/determined/values.yaml

@@ -264,6 +264,10 @@ taskContainerDefaults:
  # cpuPodSpec:
  # gpuPodSpec:

+  # Configure a custom CheckpointGC pod spec for experiments. When a custom CheckpointGC pod spec
+  # is defined, it takes precedence over the default CheckpointGC pod specifications. See


same comment about the "default spec"

NicholasBlaskey

last comment is about not needing the IsNil

i also requested a review from @tara-det-ai since there are doc changes

tara-det-ai

LGTM

NicholasBlaskey

LGTM

Great work on this PR!

ShreyaLnuHpe requested a review from NicholasBlaskey May 22, 2024 05:27

ShreyaLnuHpe requested a review from a team as a code owner May 22, 2024 05:27

cla-bot bot added the cla-signed label May 22, 2024

ShreyaLnuHpe commented May 22, 2024

View reviewed changes

ShreyaLnuHpe requested a review from a team as a code owner May 22, 2024 23:47

ShreyaLnuHpe changed the title ~~Add option for Checkpoint_GC pod spec in task container defaults~~ chore: add option for Checkpoint_GC pod spec in task container defaults May 22, 2024

ShreyaLnuHpe commented May 22, 2024

View reviewed changes

master/pkg/tasks/task_gc.go Show resolved Hide resolved

NicholasBlaskey reviewed May 23, 2024

View reviewed changes

ShreyaLnuHpe requested review from NicholasBlaskey and stoksc May 24, 2024 07:28

NicholasBlaskey reviewed May 24, 2024

View reviewed changes

ShreyaLnuHpe force-pushed the shreya/addGcPodSpecField branch from f7b456f to 067b353 Compare May 24, 2024 23:30

ShreyaLnuHpe changed the title ~~chore: add option for Checkpoint_GC pod spec in task container defaults~~ feat: add option for Checkpoint_GC pod spec in task container defaults May 24, 2024

determined-ci added the documentation Improvements or additions to documentation label May 25, 2024

ShreyaLnuHpe requested a review from NicholasBlaskey May 25, 2024 02:00

determined-ci requested a review from a team May 25, 2024 02:02

NicholasBlaskey reviewed May 28, 2024

View reviewed changes

ShreyaLnuHpe requested a review from a team as a code owner May 29, 2024 00:24

ShreyaLnuHpe requested a review from hkang1 May 29, 2024 00:24

ShreyaLnuHpe added 6 commits May 28, 2024 17:31

Add option for Checkpoint_GC pod spec

4271876

overwrite the default checkpoint gc to exp pod spec

e6b3ac8

overwrite the ExpPodSpec and cover with unit tests

83d8434

corrections based on comments

d35e954

add docs

b73ff68

docs correction

e14fe37

ShreyaLnuHpe force-pushed the shreya/addGcPodSpecField branch from 8bb2160 to e14fe37 Compare May 29, 2024 00:46

ShreyaLnuHpe requested a review from NicholasBlaskey May 29, 2024 18:23

NicholasBlaskey reviewed May 29, 2024

View reviewed changes

ShreyaLnuHpe added 2 commits May 29, 2024 17:58

correction

8b07f1c

remove test related changes

592e791

ShreyaLnuHpe requested a review from NicholasBlaskey May 30, 2024 01:22

NicholasBlaskey reviewed May 30, 2024

View reviewed changes

NicholasBlaskey requested a review from tara-det-ai May 30, 2024 14:43

remove isNil()

a5a0309

tara-det-ai approved these changes May 31, 2024

View reviewed changes

ShreyaLnuHpe requested a review from NicholasBlaskey May 31, 2024 07:34

NicholasBlaskey approved these changes May 31, 2024

View reviewed changes

ShreyaLnuHpe merged commit 8a9839a into main May 31, 2024
85 of 98 checks passed

ShreyaLnuHpe deleted the shreya/addGcPodSpecField branch May 31, 2024 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option for Checkpoint_GC pod spec in task container defaults #9406

feat: add option for Checkpoint_GC pod spec in task container defaults #9406

ShreyaLnuHpe commented May 22, 2024 •

edited

Loading

netlify bot commented May 22, 2024 •

edited

Loading

ShreyaLnuHpe May 22, 2024 •

edited

Loading

NicholasBlaskey May 23, 2024

ShreyaLnuHpe May 24, 2024

codecov bot commented May 24, 2024 •

edited

Loading

NicholasBlaskey left a comment

NicholasBlaskey May 24, 2024

ShreyaLnuHpe May 24, 2024

NicholasBlaskey May 24, 2024

NicholasBlaskey May 24, 2024

NicholasBlaskey May 28, 2024

NicholasBlaskey May 28, 2024

NicholasBlaskey May 28, 2024

NicholasBlaskey May 28, 2024

determined-ci commented May 29, 2024

NicholasBlaskey May 29, 2024

NicholasBlaskey May 29, 2024

NicholasBlaskey May 29, 2024

ShreyaLnuHpe May 30, 2024

NicholasBlaskey May 29, 2024

ShreyaLnuHpe May 30, 2024 •

edited

Loading

NicholasBlaskey May 30, 2024

NicholasBlaskey May 29, 2024

NicholasBlaskey left a comment

tara-det-ai left a comment

NicholasBlaskey left a comment

	{{- if .Values.taskContainerDefaults.gpuPodSpec }}
	gpu_pod_spec: {{ .Values.taskContainerDefaults.gpuPodSpec \| toJson }}
	{{- end }}

		@@ -280,6 +280,12 @@ def configure_helm(args: argparse.Namespace) -> None:

		helm_values["taskContainerDefaults"]["cpuPodSpec"] = cpu_pod_spec

		checkpoint_gc_pod_spec = make_spec(

	if g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil {
	if g.Base.TaskContainerDefaults != nil && g.Base.TaskContainerDefaults.CheckpointGCPodSpec != nil {

feat: add option for Checkpoint_GC pod spec in task container defaults #9406

feat: add option for Checkpoint_GC pod spec in task container defaults #9406

Conversation

ShreyaLnuHpe commented May 22, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented May 22, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

ShreyaLnuHpe May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

NicholasBlaskey 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

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

determined-ci commented May 29, 2024

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

ShreyaLnuHpe May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasBlaskey left a comment

Choose a reason for hiding this comment

tara-det-ai left a comment

Choose a reason for hiding this comment

NicholasBlaskey left a comment

Choose a reason for hiding this comment

ShreyaLnuHpe commented May 22, 2024 •

edited

Loading

netlify bot commented May 22, 2024 •

edited

Loading

ShreyaLnuHpe May 22, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

ShreyaLnuHpe May 30, 2024 •

edited

Loading