-
Notifications
You must be signed in to change notification settings - Fork 363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: update default scheduler to priority for agentrm #9385
Conversation
✅ Deploy Preview for determined-ui canceled.
|
if rmExisted || rpsExisted { | ||
if !oldRMConfig { | ||
// Use configMap if RMs are not defined at all, or if they are defined using the new schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings on this change. I think it's the right thing to do, but I'm suspicious of unintended consequences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree on both fronts. i would just run a cluster before and after this change, call det master config
for both, and diff the results. then maybe post it here if there is anything concerning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! There's only one line different between the configs with/without this specific change. The "new" / proposed version has the addition of this line:
max_cpu_containers_per_agent: -1
Which looks like it's probably fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the -1 sentinel value seems right to me.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9385 +/- ##
==========================================
+ Coverage 46.02% 46.03% +0.01%
==========================================
Files 1228 1228
Lines 155889 155895 +6
Branches 2440 2440
==========================================
+ Hits 71749 71773 +24
+ Misses 83949 83931 -18
Partials 191 191
Flags with carried forward coverage won't be shown. Click here to find out more.
|
- Job Scheduling: Switching to ``priority`` for the default scheduler. Support for round-robin and | ||
fair share schedulers is discontinued. We recommend using the priority scheduler, as it meets | ||
most scheduling needs and simplifies configuration. Moving a job will require adjusting its | ||
priority; jobs cannot be shifted within the same priority group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Job Scheduling: The default scheduler is now
priority
. Support for the round-robin and fair share schedulers has been discontinued. We recommend using the priority scheduler, as it meets
most scheduling needs and simplifies configuration. To move a job, you will need to adjust its priority; jobs cannot be shifted within the same priority group.
531d387
to
f7e4aa9
Compare
f7e4aa9
to
f257aeb
Compare
Ticket
RM-254
Description
For agent resource managers, use the priority scheduler by default.
Test Plan
Start a new instance of Determined, e.g. by running devcluster, with no configuration. On the cluster overview page, the resource pool description should say Scheduler Type Priority.
Checklist
docs/release-notes/
.See Release Note for details.