chore: add checkpoint and max slots config policy enforcements in PATCH experiment #10125

amandavialva01 · 2024-10-24T21:54:27Z

Ticket

CM-583

Description

add checkpoint and max slots config policy enforcements in PATCH experiment

Test Plan

CI passes.

Checklist

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

codecov · 2024-10-24T21:54:41Z

Codecov Report

Attention: Patch coverage is 68.88889% with 28 lines in your changes missing coverage. Please review.

Project coverage is 54.56%. Comparing base (962810a) to head (74f65d1).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
master/internal/experiment.go	40.00%	12 Missing ⚠️
master/internal/api_experiment.go	42.10%	11 Missing ⚠️
...ternal/configpolicy/postgres_task_config_policy.go	90.32%	3 Missing ⚠️
master/internal/configpolicy/utils.go	90.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10125      +/-   ##
==========================================
- Coverage   58.87%   54.56%   -4.32%     
==========================================
  Files         757     1267     +510     
  Lines      106045   159610   +53565     
  Branches     3637     3636       -1     
==========================================
+ Hits        62435    87084   +24649     
- Misses      43477    72393   +28916     
  Partials      133      133

Flag	Coverage Δ
backend	`45.94% <68.88%> (+2.13%)`	⬆️
harness	`72.56% <ø> (ø)`
web	`54.02% <ø> (ø)`

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

Files with missing lines	Coverage Δ
master/internal/configpolicy/utils.go	`74.05% <90.00%> (ø)`
...ternal/configpolicy/postgres_task_config_policy.go	`91.13% <90.32%> (ø)`
master/internal/api_experiment.go	`56.91% <42.10%> (ø)`
master/internal/experiment.go	`33.78% <40.00%> (ø)`

... and 506 files with indirect coverage changes

netlify · 2024-10-24T21:54:44Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`74f65d1`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/671bfc2fe8efe30008aff3dd

…CH experiment

kkunapuli

Some suggestions - PR looks great in general.

kkunapuli · 2024-10-25T13:17:36Z

master/internal/api_experiment.go

+	}
+	ctx := context.TODO()
+	return workspace.WorkspaceByName(ctx, wkspName)
+}


nice refactor

kkunapuli · 2024-10-25T13:18:12Z

master/internal/api_experiment_intg_test.go

+		require.NoError(t, err)
+		require.Equal(t, *wkspName, w.Name)
+	})
+}


❤️ love that you unit tested getWorkspaceByConfig!

kkunapuli · 2024-10-25T13:36:04Z

master/internal/configpolicy/postgres_task_config_policy.go

@@ -113,3 +114,55 @@ func DeleteConfigPolicies(ctx context.Context,
 	}
 	return nil
 }
+
+// GetEnforcedConfig gets the fields of the global invariant config or constraint if specified, and
+// the workspace invariant config or constraint otherwise. If neither is specified, returns nil.


Nice function description. I recommend describing what the function does, and then describing the priority order. So something like

// GetEnforcedConfig fetches the field from an invariant_config or constraints policyType, in order of precedence. Global scope has highest precedence, then workspace. Returns nil if none is found.

I find the field format to be unintuitive. e.g. "'resources' -> 'max_slots'"

Could you add an example to the function description? Or at least as a comment within the function?

I like the func description change! Changed this and added an example for the field arg

kkunapuli · 2024-10-25T13:38:54Z

master/internal/configpolicy/postgres_task_config_policy.go

@@ -113,3 +114,55 @@ func DeleteConfigPolicies(ctx context.Context,
 	}
 	return nil
 }
+


GetEnforcedConfig is fine as a name. I think something like GetConfigPolicyField is more descriptive. When I first read the function name, I thought it was only for invariant configs. It also wasn't clear that it was fetching a single field, rather than a whole or partial config.

Great point! Changed this

kkunapuli · 2024-10-25T13:47:27Z

master/internal/configpolicy/postgres_task_config_policy.go

+	if policyType != "invariant_config" && policyType != "constraints" {
+		return nil, fmt.Errorf("invalid policy type :%s", policyType)
+	}
+


Please also validate workloadType; I think all our other postgres functions do. There's no need to add a test case for it.

it seems like we actually don't validate workloadType in any of the postgres functions! At first this seemed odd, but then I remembered we made workload_type an enum!
I can still perform the validation if you'd like, but this function would be unique in that regard

In that case, don't add it. I was mistaken!

kkunapuli · 2024-10-25T13:54:18Z

master/internal/experiment.go

+		*msg.MaxSlots > *maxSlotsLimit {
+		log.Warnf("unable to set max slots")
+		return
+	}


I think this code should be in a separate function, preferably in configpolicies package. The main reason is that I prefer to keep "config policy logic" in configpolicies. In other words, an experiment API has no need to know how config policies work, that there's invariant_configs and constraints, precedence, etc. Ideally we would be able to change how config policies work by only changing code in configpolicies files.

The other reason is that the logic can be simplified if it's in its own function. The function could return at line 431 and then have no need to check enforcedMaxSlots == nil on line 442.

Love this idea, and makes sense! moved this work into its own func, great idea!

kkunapuli · 2024-10-25T18:57:57Z

master/internal/configpolicy/utils.go

+// enforced max slots for the workspace if that's set as an invariant config, and returns the
+// requested max slots otherwise. Returns an error when max slots is not set as an invariant config
+// and the requested max slots violates the constriant.
+func CanSetMaxSlots(slotsReq *int, wkspID int) (bool, *int, error) {


The function should return bool, int, error or *int, error. In the first return group, the bool lets the caller know if int was set or not. In the second group, a valid int is inferred from whether or not the pointer is nil.

I would simplify it further to just int, error. If there's an error, max_slots cannot be updated. If error is nil, then set max_slots to the returned int value.

Great point, changed return type to *int, error!

Ahh wait ok i see your point about int, error!
hmm, yes i see! ok ill change to this

Gah ok actually, I think it's easier to keep this as is since the func takes in an optional *int (so it can return that same optional *int).
So when the caller gets the func output, it can just replace its input w the func output.
Are you cool w leaving it *int, error instead of int, error?

…CH experiment (#10125)

amandavialva01 requested a review from a team as a code owner October 24, 2024 21:54

amandavialva01 requested a review from hamidzr October 24, 2024 21:54

cla-bot bot added the cla-signed label Oct 24, 2024

amandavialva01 force-pushed the amanda/CM583 branch from a33456e to bb87e37 Compare October 24, 2024 21:56

chore: add checkpoint and max slots config policy enforcements in PAT…

afe5e43

…CH experiment

amandavialva01 force-pushed the amanda/CM583 branch from 7ec5781 to afe5e43 Compare October 24, 2024 22:20

kkunapuli suggested changes Oct 25, 2024

View reviewed changes

add more tests, refactor to fix tests, and address comments

2ce783a

amandavialva01 force-pushed the amanda/CM583 branch from 70b0d5e to 2ce783a Compare October 25, 2024 17:24

kkunapuli reviewed Oct 25, 2024

View reviewed changes

amandavialva01 added 2 commits October 25, 2024 15:42

simplify CanSetMaxSlots

745bf7e

fix err message

74f65d1

kkunapuli approved these changes Oct 25, 2024

View reviewed changes

amandavialva01 enabled auto-merge (squash) October 25, 2024 20:30

amandavialva01 merged commit 233e095 into main Oct 25, 2024
80 of 94 checks passed

amandavialva01 deleted the amanda/CM583 branch October 25, 2024 20:48

thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024

chore: add checkpoint and max slots config policy enforcements in PAT…

d69fd3d

…CH experiment (#10125)

thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024

chore: add checkpoint and max slots config policy enforcements in PAT…

d1b893e

…CH experiment (#10125)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add checkpoint and max slots config policy enforcements in PATCH experiment #10125

chore: add checkpoint and max slots config policy enforcements in PATCH experiment #10125

amandavialva01 commented Oct 24, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

netlify bot commented Oct 24, 2024 •

edited

Loading

kkunapuli left a comment

kkunapuli Oct 25, 2024

kkunapuli Oct 25, 2024

kkunapuli Oct 25, 2024

kkunapuli Oct 25, 2024

amandavialva01 Oct 25, 2024

kkunapuli Oct 25, 2024

amandavialva01 Oct 25, 2024

kkunapuli Oct 25, 2024

amandavialva01 Oct 25, 2024

kkunapuli Oct 25, 2024

kkunapuli Oct 25, 2024

amandavialva01 Oct 25, 2024

kkunapuli Oct 25, 2024

amandavialva01 Oct 25, 2024

amandavialva01 Oct 25, 2024

amandavialva01 Oct 25, 2024 •

edited

Loading

chore: add checkpoint and max slots config policy enforcements in PATCH experiment #10125

chore: add checkpoint and max slots config policy enforcements in PATCH experiment #10125

Conversation

amandavialva01 commented Oct 24, 2024 • edited by jira bot Loading

Ticket

Description

Test Plan

Checklist

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

netlify bot commented Oct 24, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

kkunapuli 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

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

amandavialva01 Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

amandavialva01 commented Oct 24, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

netlify bot commented Oct 24, 2024 •

edited

Loading

amandavialva01 Oct 25, 2024 •

edited

Loading