feat: remove round robin scheduler for agentrm #9493

kkunapuli · 2024-06-10T18:15:22Z

Ticket

RM-322

Description

Remove round robin scheduler for agent resource managers. Deprecation announced in 0.33.0.

Test Plan

None needed.

Checklist

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

netlify · 2024-06-10T18:15:38Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`1f3c2bf`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/666b3ea19aa75300080491f6

kkunapuli · 2024-06-10T18:18:30Z

docs/get-started/architecture/introduction.rst

@@ -428,7 +428,7 @@ Here is an example master configuration illustrating the potential problem.
   resource_manager:
     type: agent
     scheduler:
-       type: round_robin


updating example to only use schedulers that still exist

codecov · 2024-06-10T18:21:33Z

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 48.99%. Comparing base (de03909) to head (1f3c2bf).
Report is 16 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9493   +/-   ##
=======================================
  Coverage   48.99%   48.99%           
=======================================
  Files        1235     1234    -1     
  Lines      160191   160153   -38     
  Branches     2780     2781    +1     
=======================================
- Hits        78482    78474    -8     
+ Misses      81534    81504   -30     
  Partials      175      175

Flag	Coverage Δ
backend	`43.83% <95.00%> (+0.01%)`	⬆️
harness	`63.96% <ø> (ø)`
web	`44.12% <ø> (ø)`

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

Files	Coverage Δ
master/internal/config/config.go	`71.37% <100.00%> (+0.72%)`	⬆️
master/internal/config/scheduler_config.go	`84.09% <100.00%> (+15.90%)`	⬆️
master/internal/rm/agentrm/agents.go	`63.30% <ø> (ø)`
master/internal/rm/agentrm/scheduler.go	`83.33% <100.00%> (+27.77%)`	⬆️
...ster/internal/rm/agentrm/agent_resource_manager.go	`49.22% <75.00%> (+0.11%)`	⬆️

... and 2 files with indirect coverage changes

ShreyaLnuHpe

LGTM

tara-hpe

LGTM

NicholasBlaskey

one comment but other that, looks good to me

NicholasBlaskey · 2024-06-13T19:51:48Z

master/internal/config/config.go

-		if r.ResourceManager.AgentRM != nil && r.ResourceManager.AgentRM.Scheduler == nil {
-			r.ResourceManager.AgentRM.Scheduler = DefaultSchedulerConfig()
+		if r.ResourceManager.AgentRM != nil {
+			if r.ResourceManager.AgentRM.Scheduler == nil {
+				r.ResourceManager.AgentRM.Scheduler = DefaultSchedulerConfig()
+			}
+			if r.ResourceManager.AgentRM.Scheduler.GetType() == FairShareScheduling {
+				log.Warn("Fair-Share Scheduler has been deprecated, please update master config to use Priority Scheduler.")
+			}
+			if r.ResourceManager.AgentRM.Scheduler.GetType() == RoundRobinScheduling {
+				log.Error("Round Robin Scheduler has been removed, please update master config to use Priority Scheduler.")
+				log.Info("Priority Scheduler with all priorities equal will have the same behavior as a Round Robin Scheduler.")
+				return fmt.Errorf("scheduler not available")
+			}


I think the changes in MakeScheduler should take care of this right?

I think this would log two warnings if you specified fairshare in the resource manager level

The changes in MakeScheduler do take care of it, for the most part.

If the resource manger is configured to use round_robin but all resource pools are configured to use other schedulers (like below), then MakeScheduler will not complain. Do we care about that (admittedly unlikely) case?

resource_manager: type: agent scheduler: type: round_robin fitting_policy: best resource_pools: - pool_name: pool1 scheduler: type: fair_share fitting_policy: best

I think it is probably fine to not handle that case.

Though if we do want to handle that case it would be nice to do it without logging more than expected warnings.

NicholasBlaskey · 2024-06-13T19:55:58Z

master/internal/rm/agentrm/agents.go

@@ -205,6 +205,7 @@ func (a *agents) createAgent(

 	var poolConfig *config.ResourcePoolConfig
 	for _, pc := range a.poolConfigs {
+		// The address of a loop variable is always the same. Use a temporary variable to capture the address.


this is no longer the case in Go 1.22

https://go.dev/blog/loopvar-preview

make -C master check complained when I removed pc := pc. I can remove the comment, though.

NicholasBlaskey

I think changes are fine as is

NicholasBlaskey · 2024-06-17T13:44:03Z

master/internal/config/config.go

-		if r.ResourceManager.AgentRM != nil && r.ResourceManager.AgentRM.Scheduler == nil {
-			r.ResourceManager.AgentRM.Scheduler = DefaultSchedulerConfig()
+		if r.ResourceManager.AgentRM != nil {
+			if r.ResourceManager.AgentRM.Scheduler == nil {
+				r.ResourceManager.AgentRM.Scheduler = DefaultSchedulerConfig()
+			}
+			if r.ResourceManager.AgentRM.Scheduler.GetType() == FairShareScheduling {
+				log.Warn("Fair-Share Scheduler has been deprecated, please update master config to use Priority Scheduler.")
+			}
+			if r.ResourceManager.AgentRM.Scheduler.GetType() == RoundRobinScheduling {
+				log.Error("Round Robin Scheduler has been removed, please update master config to use Priority Scheduler.")
+				log.Info("Priority Scheduler with all priorities equal will have the same behavior as a Round Robin Scheduler.")
+				return fmt.Errorf("scheduler not available")
+			}


I think it is probably fine to not handle that case.

Though if we do want to handle that case it would be nice to do it without logging more than expected warnings.

cla-bot bot added the cla-signed label Jun 10, 2024

determined-ci added the documentation Improvements or additions to documentation label Jun 10, 2024

determined-ci requested a review from a team June 10, 2024 18:15

kkunapuli commented Jun 10, 2024

View reviewed changes

kkunapuli added 6 commits June 11, 2024 14:50

checkpoint

baba804

remove round robin from documentation

c69845f

refine messaging

5022d40

nit

06e899e

default to priority for rps

b3cd708

remove debug line

e5d29d0

kkunapuli force-pushed the kunapuli/remove-round-robin branch from f76b2de to e5d29d0 Compare June 11, 2024 18:50

kkunapuli added 4 commits June 11, 2024 14:57

describe why temp var exists

7a3ad3f

docs fmt

f58ac57

remove debug

00affac

change if to ifelse

0178146

kkunapuli changed the title ~~Kunapuli/remove round robin~~ feat: remove round robin scheduler for agentrm Jun 11, 2024

kkunapuli added 2 commits June 12, 2024 11:51

update tests; add covereage

6607a9c

lint fixes

fa10ac0

kkunapuli marked this pull request as ready for review June 12, 2024 16:19

kkunapuli requested review from a team as code owners June 12, 2024 16:19

kkunapuli requested a review from ShreyaLnuHpe June 12, 2024 16:19

kkunapuli added 2 commits June 12, 2024 12:23

add release note

d8b9ad2

fix formatting

f729275

ShreyaLnuHpe approved these changes Jun 12, 2024

View reviewed changes

tara-hpe approved these changes Jun 13, 2024

View reviewed changes

kkunapuli added 2 commits June 13, 2024 14:18

protect against case with resource pool using round-robin

fa02300

format

c5a871c

determined-ci requested a review from a team June 13, 2024 18:19

add test enforcing deprecation of round robin resource pool

1f3c2bf

NicholasBlaskey approved these changes Jun 13, 2024

View reviewed changes

kkunapuli requested a review from NicholasBlaskey June 13, 2024 21:32

NicholasBlaskey approved these changes Jun 17, 2024

View reviewed changes

kkunapuli merged commit 10667f1 into main Jun 17, 2024
89 of 102 checks passed

kkunapuli deleted the kunapuli/remove-round-robin branch June 17, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove round robin scheduler for agentrm #9493

feat: remove round robin scheduler for agentrm #9493

kkunapuli commented Jun 10, 2024 •

edited by jira bot

Loading

netlify bot commented Jun 10, 2024 •

edited

Loading

kkunapuli Jun 10, 2024

codecov bot commented Jun 10, 2024 •

edited

Loading

ShreyaLnuHpe left a comment

tara-hpe left a comment

NicholasBlaskey left a comment

NicholasBlaskey Jun 13, 2024

kkunapuli Jun 13, 2024

NicholasBlaskey Jun 17, 2024

NicholasBlaskey Jun 13, 2024

kkunapuli Jun 13, 2024

NicholasBlaskey left a comment

NicholasBlaskey Jun 17, 2024

feat: remove round robin scheduler for agentrm #9493

feat: remove round robin scheduler for agentrm #9493

Conversation

kkunapuli commented Jun 10, 2024 • edited by jira bot Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Jun 10, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

kkunapuli Jun 10, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2024 • edited Loading

Codecov Report

ShreyaLnuHpe left a comment

Choose a reason for hiding this comment

tara-hpe left a comment

Choose a reason for hiding this comment

NicholasBlaskey left a comment

Choose a reason for hiding this comment

NicholasBlaskey Jun 13, 2024

Choose a reason for hiding this comment

kkunapuli Jun 13, 2024

Choose a reason for hiding this comment

NicholasBlaskey Jun 17, 2024

Choose a reason for hiding this comment

NicholasBlaskey Jun 13, 2024

Choose a reason for hiding this comment

kkunapuli Jun 13, 2024

Choose a reason for hiding this comment

NicholasBlaskey left a comment

Choose a reason for hiding this comment

NicholasBlaskey Jun 17, 2024

Choose a reason for hiding this comment

kkunapuli commented Jun 10, 2024 •

edited by jira bot

Loading

netlify bot commented Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading