chore: check task config policy priority limit for [CM-490] #9958
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9958 +/- ##
==========================================
- Coverage 54.53% 54.53% -0.01%
==========================================
Files 1257 1258 +1
Lines 156933 157009 +76
Branches 3614 3612 -2
==========================================
+ Hits 85589 85619 +30
- Misses 71211 71257 +46
Partials 133 133
Flags with carried forward coverage won't be shown. Click here to find out more.
|
kkunapuli
requested review from
jesse-amano-hpe and
amandavialva01
and removed request for
jesse-amano-hpe
| } | ||
| }() | ||
|
|
||
| // No limit set. |
There was a problem hiding this comment.
// No global priority NTSC limit set
| return priorityWithinLimit(priority, limit, smallerHigher), nil | ||
| } | ||
|
|
||
| // No priority limit has been set. |
There was a problem hiding this comment.
Nice comments throughout this function!
|
|
||
| // SmallerValueIsHigherPriority returns true if smaller priority values indicate a higher priority level. | ||
| func (m *DispatcherResourceManager) SmallerValueIsHigherPriority() (bool, error) { | ||
| return false, fmt.Errorf("priority not implemented") |
There was a problem hiding this comment.
just to confirm, does this mean that Determined doesn't support setting job priority on slurm clusters?
|
|
||
| // SmallerValueIsHigherPriority returns true if smaller priority values indicate a higher priority level. | ||
| func (m *MultiRMRouter) SmallerValueIsHigherPriority() (bool, error) { | ||
| return false, nil |
There was a problem hiding this comment.
Just to confirm, are we setting this to false because multiRM implies the cluster must be all k8s RMs, and k8s RMs use higher value -> higher priority?
There was a problem hiding this comment.
Yes; does that approach sound reasonable to you?
There was a problem hiding this comment.
Yeaa I like this a lot! Super clean way to handle this Kristine!
Can we add a comment mentioning this?
There was a problem hiding this comment.
I was writing a comment, and felt bad about stating that kind of assumption so I rewrote it to call the underlying RM. 😅
There was a problem hiding this comment.
Ahh lol comments keep us honest 😆. FWIW, I do think you initial method of returning false was clean as well, but it's definitely more intuitive / easy to understand from an outside perspective to call the underlying RM, nice work!
| wkspIDDoesNotExist := 404 | ||
| _, found, err = GetPriorityLimit(ctx, &wkspIDDoesNotExist, model.NTSCType) | ||
| require.NoError(t, err) | ||
| require.False(t, found) |
There was a problem hiding this comment.
Nice! I like the graceful exist for non-existent workspaces
Ticket
CM-490
Description
Includes a check for a priority limit set via task config policies in the "manage job" workflow. Includes support for NTSC, and Experiments. It works for policies set for Workspaces, and Global scope.
Test Plan
Covered by unit and integration tests.
Checklist
docs/release-notes/See Release Note for details.