From 655f63598a387eef12b9f669ea67fb00dfe86147 Mon Sep 17 00:00:00 2001 From: Amanda Vialva Date: Tue, 15 Oct 2024 15:53:19 -0400 Subject: [PATCH 1/3] fix: experiment config slots to comply with constraint max slots --- master/internal/configpolicy/utils.go | 6 +-- master/internal/configpolicy/utils_test.go | 43 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/master/internal/configpolicy/utils.go b/master/internal/configpolicy/utils.go index d7b645d07f9..e1b28a211a1 100644 --- a/master/internal/configpolicy/utils.go +++ b/master/internal/configpolicy/utils.go @@ -159,9 +159,9 @@ func checkConstraintConflicts(constraints *model.Constraints, maxSlots, slots, p if maxSlots != nil && *constraints.ResourceConstraints.MaxSlots != *maxSlots { return fmt.Errorf("invariant config & constraints are trying to set the max slots") } - if slots != nil && *constraints.ResourceConstraints.MaxSlots > *slots { - return fmt.Errorf("invariant config & constraints are attempting to set an invalid max slot123: %v vs %v", - *constraints.ResourceConstraints.MaxSlots, *slots) + if slots != nil && *constraints.ResourceConstraints.MaxSlots < *slots { + return fmt.Errorf("invariant config has %v slots per trial. violates constraints max slots of %v", + *slots, *constraints.ResourceConstraints.MaxSlots) } return nil diff --git a/master/internal/configpolicy/utils_test.go b/master/internal/configpolicy/utils_test.go index 3649fa4c68d..e6d68ea53b1 100644 --- a/master/internal/configpolicy/utils_test.go +++ b/master/internal/configpolicy/utils_test.go @@ -7,6 +7,7 @@ import ( "gotest.tools/assert" "github.com/determined-ai/determined/master/pkg/model" + "github.com/determined-ai/determined/master/pkg/ptrs" "github.com/determined-ai/determined/master/pkg/schemas/expconf" ) @@ -309,3 +310,45 @@ func TestUnmarshalJSONNTSC(t *testing.T) { }) } } + +func TestCheckConstraintsConflicts(t *testing.T) { + constraints := &model.Constraints{ + ResourceConstraints: &model.ResourceConstraints{ + MaxSlots: ptrs.Ptr(10), + }, + PriorityLimit: ptrs.Ptr(50), + } + t.Run("max_slots differs to high", func(t *testing.T) { + err := checkConstraintConflicts(constraints, ptrs.Ptr(11), ptrs.Ptr(5), nil) + require.Error(t, err) + }) + t.Run("max_slots differs to low", func(t *testing.T) { + err := checkConstraintConflicts(constraints, ptrs.Ptr(9), ptrs.Ptr(5), nil) + require.Error(t, err) + }) + + t.Run("slots_per_trial too high", func(t *testing.T) { + err := checkConstraintConflicts(constraints, ptrs.Ptr(5), ptrs.Ptr(11), nil) + require.Error(t, err) + }) + + t.Run("slots_per_trial within range", func(t *testing.T) { + err := checkConstraintConflicts(constraints, ptrs.Ptr(10), ptrs.Ptr(8), nil) + require.NoError(t, err) + }) + + t.Run("priority differs too high", func(t *testing.T) { + err := checkConstraintConflicts(constraints, nil, nil, ptrs.Ptr(100)) + require.Error(t, err) + }) + + t.Run("priority differs too low", func(t *testing.T) { + err := checkConstraintConflicts(constraints, nil, nil, ptrs.Ptr(10)) + require.Error(t, err) + }) + + t.Run("all comply", func(t *testing.T) { + err := checkConstraintConflicts(constraints, ptrs.Ptr(10), ptrs.Ptr(10), ptrs.Ptr(50)) + require.NoError(t, err) + }) +} From e2bc7444468555a4cbad4975940ad9d415d68857 Mon Sep 17 00:00:00 2001 From: Amanda Vialva Date: Tue, 15 Oct 2024 16:57:51 -0400 Subject: [PATCH 2/3] check slots per trial adheres to slots constriant --- master/internal/configpolicy/task_config_policy.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/master/internal/configpolicy/task_config_policy.go b/master/internal/configpolicy/task_config_policy.go index 6a146611eb1..e40e67553b5 100644 --- a/master/internal/configpolicy/task_config_policy.go +++ b/master/internal/configpolicy/task_config_policy.go @@ -88,10 +88,15 @@ func CheckExperimentConstraints( if constraints.ResourceConstraints != nil && constraints.ResourceConstraints.MaxSlots != nil { // users cannot specify number of slots for an experiment - slotsRequest := *constraints.ResourceConstraints.MaxSlots - if err = checkSlotsConstraint(*constraints.ResourceConstraints.MaxSlots, slotsRequest, - workloadConfig.Resources().MaxSlots()); err != nil { - return err + if workloadConfig.RawResources != nil { + slotsRequest := workloadConfig.RawResources.RawSlotsPerTrial + if slotsRequest != nil { + if err = checkSlotsConstraint(*constraints.ResourceConstraints.MaxSlots, + *slotsRequest, + workloadConfig.Resources().MaxSlots()); err != nil { + return err + } + } } } From 3dce288769dc83e11fc96bf8f7b749920c3f1984 Mon Sep 17 00:00:00 2001 From: Amanda Vialva Date: Tue, 15 Oct 2024 18:16:59 -0400 Subject: [PATCH 3/3] fix tests --- .../internal/api_config_policies_intg_test.go | 6 ++-- .../configpolicy/task_config_policy.go | 28 +++++++++++-------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/master/internal/api_config_policies_intg_test.go b/master/internal/api_config_policies_intg_test.go index 61489a2f7a7..b51fc30fef7 100644 --- a/master/internal/api_config_policies_intg_test.go +++ b/master/internal/api_config_policies_intg_test.go @@ -861,7 +861,7 @@ invariant_config: // Additional NTSC combinatory tests (YAML). { "YAML NTSC valid config valid constraints", model.NTSCType, - validNTSCConfigPolicyYAML + validConstraintsPolicyYAML, fmt.Errorf("invalid ntsc config policy"), + validNTSCConfigPolicyYAML + validConstraintsPolicyYAML, nil, }, { "YAML NTSC valid constraints invalid constraints", model.NTSCType, @@ -1192,8 +1192,8 @@ func TestValidatePoliciesAndWorkloadTypeJSON(t *testing.T) { // Additional NTSC combinatory tests (JSON). { - "JSON NTSC valid config invalid constraints", model.NTSCType, - "{" + validNTSCConfigPolicyJSON + "," + validConstraintsPolicyJSON + "}", fmt.Errorf("invalid ntsc config policy"), + "JSON NTSC valid config valid constraints", model.NTSCType, + "{" + validNTSCConfigPolicyJSON + "," + validConstraintsPolicyJSON + "}", nil, }, { "JSON NTSC valid constraints invalid constraints", model.NTSCType, diff --git a/master/internal/configpolicy/task_config_policy.go b/master/internal/configpolicy/task_config_policy.go index e40e67553b5..a7d119d817e 100644 --- a/master/internal/configpolicy/task_config_policy.go +++ b/master/internal/configpolicy/task_config_policy.go @@ -54,7 +54,7 @@ func CheckNTSCConstraints( } if constraints.ResourceConstraints != nil && constraints.ResourceConstraints.MaxSlots != nil { - if err = checkSlotsConstraint(*constraints.ResourceConstraints.MaxSlots, workloadConfig.Resources.Slots, + if err = checkSlotsConstraint(*constraints.ResourceConstraints.MaxSlots, &workloadConfig.Resources.Slots, workloadConfig.Resources.MaxSlots); err != nil { return err } @@ -90,12 +90,16 @@ func CheckExperimentConstraints( // users cannot specify number of slots for an experiment if workloadConfig.RawResources != nil { slotsRequest := workloadConfig.RawResources.RawSlotsPerTrial - if slotsRequest != nil { - if err = checkSlotsConstraint(*constraints.ResourceConstraints.MaxSlots, - *slotsRequest, - workloadConfig.Resources().MaxSlots()); err != nil { - return err - } + if err = checkSlotsConstraint(*constraints.ResourceConstraints.MaxSlots, + slotsRequest, + workloadConfig.Resources().MaxSlots()); err != nil { + return err + } + slotsRequest = workloadConfig.RawResources.RawMaxSlots + if err = checkSlotsConstraint(*constraints.ResourceConstraints.MaxSlots, + slotsRequest, + workloadConfig.Resources().MaxSlots()); err != nil { + return err } } } @@ -126,10 +130,12 @@ func checkPriorityConstraint(smallerHigher bool, priorityLimit *int, priorityReq return nil } -func checkSlotsConstraint(slotsLimit int, slotsRequest int, maxSlotsRequest *int) error { - if slotsLimit < slotsRequest { - return fmt.Errorf("requested resources.slots [%d] exceeds limit set by admin [%d]: %w", - slotsRequest, slotsLimit, errResourceConstraintFailure) +func checkSlotsConstraint(slotsLimit int, slotsRequest *int, maxSlotsRequest *int) error { + if slotsRequest != nil { + if slotsLimit < *slotsRequest { + return fmt.Errorf("requested resources.slots [%d] exceeds limit set by admin [%d]: %w", + slotsRequest, slotsLimit, errResourceConstraintFailure) + } } if maxSlotsRequest != nil {