chore: populate exp config priority with enforced priority before opting to rm default #10109
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10109 +/- ##
==========================================
- Coverage 54.49% 54.47% -0.02%
==========================================
Files 1267 1267
Lines 159437 159449 +12
Branches 3637 3636 -1
==========================================
- Hits 86885 86866 -19
- Misses 72419 72450 +31
Partials 133 133
Flags with carried forward coverage won't be shown. Click here to find out more.
|
✅ Deploy Preview for determined-ui canceled.
|
…ing to k8s default CM-586: #in-review
amandavialva01
force-pushed
the
amanda/CM586
branch
from
1265f30 to
2cf4d84
Compare
master/internal/core_experiment.go
Outdated
| prio := masterConfig.DefaultPriorityForPool(poolName.String()) | ||
| config.RawResources.RawPriority = &prio | ||
| // Returns an error if RM does not implement priority. | ||
| enforcedPriority, _, err := configpolicy.FindAllowedPriority( |
There was a problem hiding this comment.
i'm confused why an unset priority becomes the constraints.priority_limit.
There was a problem hiding this comment.
Really good catch, this should be enforcedPriority != nil, rather than enforcedPriority == nil
There was a problem hiding this comment.
Oh wait, actually I think it's good as is!
This is basically saying that if no priority is enforced with config policies, then we use the default priority specified by the rm.
Otherwise, we use the enforced priority specified in the invariant config or constraints
There was a problem hiding this comment.
If you're talking about in the FindAllowedPriority function, I am not entirely sure where you mean 🙈
There was a problem hiding this comment.
what I'm saying is we want to constrain by the constraint not enforce it, right?
There was a problem hiding this comment.
cool. an aside: FindAllowedPriority feels strange; it's sort of leaking details about how it chose the priority. at least in this callsite I think a better API be InvariantPriority(...) (prio int, ok bool) that returns the invariant priority for a workload or (0, false) if there is none.
There was a problem hiding this comment.
curious what you think @kkunapuli
There was a problem hiding this comment.
I think a better API be InvariantPriority(...) (prio int, ok bool) that returns the invariant priority for a workload or (0, false) if there is none.
+1 to this! I think we should separate out the logic for getting the invariant config and the constraints when finding different fields. FindAllowedPriority is really well commented so it's possible to read the comments and understand the logic behind the return values, but I kinda feel like it's a bit too much logic packed into one func, particularly with the second output boolean.
For what it's worth, I have a branch right now that uses generics to get a given config field and can potentially be used to shorten/refactor FindAllowedPriority to get the invariant configs in one foul swoop!
There was a problem hiding this comment.
@stoksc I agree that the function could use a refactor.
@amandavialva01 - the refactor should not impact functionality. With that in mind, please finish all other in-progress work before starting a refactor. Also, I would be more comfortable if we merge it after the release cutoff at this point.
There was a problem hiding this comment.
Cool! I implemented the generics function for PATCH experiment changes, not with the intention of refactoring FindAllowedPriority. I'll avoid making any refactor changes to that func!
Co-authored-by: Bradley Laney <bradley.laney@hpe.com>
|
Closing this as this is not a bug |
Ticket
CM-586
Description
Populate experiment config priority with the enforced priority specified in workspace or global invariant config or constraints policies.
If priority is not enforced by config policies within the experiment's scope, populate with the experiment config default priority specified by the rm.
Test Plan
Checklist
docs/release-notes/See Release Note for details.