chore: refactor ResourceManager interface for multirm #8847

carolinaecalderon · 2024-02-14T19:41:03Z

Description

Refactor the requests/responses in ResourceManager interface, as a pre-requisite for the multiRM project, remove sproto message types where necessary.

Add a ResourceManager field to sproto.AllocateRequest{} and expconf.ResourceConfigV0.

Test Plan

Pass existing tests.

Commentary (optional)

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.

Ticket

RM-44

codecov · 2024-02-14T20:46:32Z

Codecov Report

Attention: Patch coverage is 34.54545% with 144 lines in your changes are missing coverage. Please review.

Project coverage is 47.54%. Comparing base (4618389) to head (8134116).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8847   +/-   ##
=======================================
  Coverage   47.53%   47.54%           
=======================================
  Files        1066     1066           
  Lines      170434   170450   +16     
  Branches     2235     2235           
=======================================
+ Hits        81015    81036   +21     
+ Misses      89261    89256    -5     
  Partials      158      158

Flag	Coverage Δ
backend	`43.39% <34.54%> (+0.03%)`	⬆️
harness	`63.69% <ø> (ø)`
web	`42.53% <ø> (ø)`

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

Files	Coverage Δ
master/internal/api_command.go	`47.34% <100.00%> (+0.25%)`	⬆️
master/internal/api_tasks.go	`47.83% <100.00%> (ø)`
master/internal/checkpoint_gc.go	`68.04% <100.00%> (+2.10%)`	⬆️
master/internal/command/command.go	`68.10% <100.00%> (+0.13%)`	⬆️
master/internal/core_experiment.go	`61.33% <100.00%> (+0.17%)`	⬆️
master/internal/rm/agentrm/agents.go	`32.43% <ø> (ø)`
master/internal/rm/kubernetesrm/pods.go	`15.70% <ø> (ø)`
master/internal/task/allocation.go	`76.46% <100.00%> (+0.36%)`	⬆️
master/internal/telemetry/reports.go	`69.76% <100.00%> (ø)`
master/internal/api_job.go	`0.00% <0.00%> (ø)`
... and 16 more

... and 3 files with indirect coverage changes

master/pkg/model/experiment_config.go

netlify · 2024-02-19T21:56:03Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`8134116`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/65df2c0a88616e00083b8184

NicholasBlaskey

Looks pretty good to me

master/internal/api_generic_tasks.go

master/internal/api_resourcepool.go

master/internal/experiment_job_service.go

master/internal/job/jobservice/jobservice.go

master/internal/rm/tasklist/job.go

NicholasBlaskey · 2024-02-21T20:13:53Z

proto/src/determined/api/v1/agent.proto

@@ -47,6 +47,8 @@ message GetAgentsResponse {
 message GetAgentRequest {
  // The id of the agent.
  string agent_id = 1;
+  // The resource manager for the request.
+  string resource_manager = 2;


Is this optional? Or required?

Is it optional if you just have a single resource manager?

Can we mark optional fields with optional string resource_manager = 2;

I mean, it can always be blank "" -- there will have to be some logic in the router that checks that checks these cases.

proto/src/determined/api/v1/job.proto

master/internal/rm/resource_manager_iface.go

master/pkg/schemas/expconf/legacy.go

master/internal/api_experiment.go

master/internal/api_trials.go

master/internal/rm/kubernetesrm/kubernetes_resource_manager.go

This reverts commit 6857ecf.

This reverts commit c3012ff. Revert "chore: refactor ResourceManager interface for multirm (#8847)" This reverts commit 6857ecf. Revert "chore: refactor proto, schema, and jobservice for multiRM (#8875)" This reverts commit 7c6bec9.

* Revert "chore: add multirm module to ResourceManager (#8857)" This reverts commit c3012ff. Revert "chore: refactor ResourceManager interface for multirm (#8847)" This reverts commit 6857ecf. Revert "chore: refactor proto, schema, and jobservice for multiRM (#8875)" This reverts commit 7c6bec9.

…-ai#1196)

cla-bot bot added the cla-signed label Feb 14, 2024

carolinaecalderon changed the title ~~Chore replace rp struct~~ chore: replace ResourceManager interface vars with ResourcePoolName Feb 14, 2024

carolinaecalderon force-pushed the chore-replace-rp-struct branch from 98f34b3 to 3eede86 Compare February 14, 2024 21:28

carolinaecalderon marked this pull request as ready for review February 14, 2024 21:32

carolinaecalderon requested a review from a team as a code owner February 14, 2024 21:32

carolinaecalderon requested review from maxrussell, stoksc and NicholasBlaskey and removed request for a team February 14, 2024 21:32

NicholasBlaskey reviewed Feb 15, 2024

View reviewed changes

master/pkg/model/experiment_config.go Outdated Show resolved Hide resolved

master/pkg/model/experiment_config.go Outdated Show resolved Hide resolved

master/pkg/model/experiment_config.go Outdated Show resolved Hide resolved

Base automatically changed from remove-redundant-rm-interface to main February 16, 2024 16:45

carolinaecalderon force-pushed the chore-replace-rp-struct branch from 3eede86 to 82b6894 Compare February 19, 2024 21:55

carolinaecalderon requested review from a team as code owners February 19, 2024 21:55

carolinaecalderon requested review from thiagodallacqua-hpe and dzhu February 19, 2024 21:55

carolinaecalderon force-pushed the chore-replace-rp-struct branch from 82b6894 to e8fb827 Compare February 19, 2024 21:56

carolinaecalderon changed the base branch from main to nickb_multirm_config February 19, 2024 21:57

carolinaecalderon force-pushed the chore-replace-rp-struct branch 2 times, most recently from aa8c82b to 90d55fe Compare February 20, 2024 08:44

carolinaecalderon marked this pull request as draft February 20, 2024 09:44

carolinaecalderon force-pushed the chore-replace-rp-struct branch from 6de32e4 to 32fbd56 Compare February 20, 2024 13:16

carolinaecalderon changed the title ~~chore: replace ResourceManager interface vars with ResourcePoolName~~ chore: add ResourceManager fields to Allocate & ResourceConfig Feb 20, 2024

carolinaecalderon removed request for a team, thiagodallacqua-hpe and dzhu February 20, 2024 13:19

NicholasBlaskey force-pushed the nickb_multirm_config branch from 5343ec6 to 8624b04 Compare February 20, 2024 15:55

carolinaecalderon requested a review from tayritenour February 21, 2024 17:27

carolinaecalderon changed the title ~~chore: add ResourceManager fields to Allocate & ResourceConfig~~ chore: add ResourceManager fields to AllocateRequest & ResourceConfig Feb 21, 2024

carolinaecalderon requested a review from NicholasBlaskey February 21, 2024 18:41

NicholasBlaskey reviewed Feb 21, 2024

View reviewed changes

carolinaecalderon assigned stoksc Feb 22, 2024

carolinaecalderon changed the base branch from main to carolinac/multirm-proto-schema-changes February 23, 2024 04:34

carolinaecalderon force-pushed the chore-replace-rp-struct branch from d2a1fd7 to 70a1acf Compare February 23, 2024 14:58

carolinaecalderon force-pushed the carolinac/multirm-proto-schema-changes branch from b834b79 to dea6aad Compare February 23, 2024 19:10

carolinaecalderon force-pushed the chore-replace-rp-struct branch 2 times, most recently from e976eb5 to 626f7c0 Compare February 26, 2024 02:00

stoksc approved these changes Feb 27, 2024

View reviewed changes

Base automatically changed from carolinac/multirm-proto-schema-changes to main February 27, 2024 20:14

carolinaecalderon added 2 commits February 27, 2024 18:18

chore: refactor resourceManager interface for multirm

f636af8

updates

99a53ba

carolinaecalderon force-pushed the chore-replace-rp-struct branch from 976a47a to 99a53ba Compare February 27, 2024 23:37

remove legacy expconf changes

8134116

carolinaecalderon changed the title ~~chore: add ResourceManager fields to AllocateRequest & ResourceConfig~~ chore: refactor ResourceManager interface for multirm Feb 28, 2024

carolinaecalderon merged commit 6857ecf into main Feb 28, 2024
82 of 101 checks passed

carolinaecalderon deleted the chore-replace-rp-struct branch February 28, 2024 16:58

rb-determined-ai pushed a commit that referenced this pull request Feb 29, 2024

chore: update dispatcherrm for multirm #8847 (#1196)

9ef4b80

carolinaecalderon added a commit that referenced this pull request Mar 5, 2024

Revert "chore: refactor ResourceManager interface for multirm (#8847)"

48ed476

This reverts commit 6857ecf.

amandavialva01 pushed a commit that referenced this pull request Mar 18, 2024

chore: update dispatcherrm for multirm #8847 (#1196)

4e398bd

maxrussell pushed a commit that referenced this pull request Mar 21, 2024

chore: refactor ResourceManager interface for multirm (#8847)

af3ee35

eecsliu pushed a commit that referenced this pull request Apr 18, 2024

chore: update dispatcherrm for multirm #8847 (#1196)

baa1c82

eecsliu pushed a commit to determined-ai/determined-release-testing that referenced this pull request Apr 22, 2024

chore: update dispatcherrm for multirm determined-ai#8847 (determined…

d4bd615

…-ai#1196)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor ResourceManager interface for multirm #8847

chore: refactor ResourceManager interface for multirm #8847

carolinaecalderon commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

netlify bot commented Feb 19, 2024 •

edited

Loading

NicholasBlaskey left a comment

NicholasBlaskey Feb 21, 2024

carolinaecalderon Feb 21, 2024

chore: refactor ResourceManager interface for multirm #8847

chore: refactor ResourceManager interface for multirm #8847

Conversation

carolinaecalderon commented Feb 14, 2024 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

netlify bot commented Feb 19, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

NicholasBlaskey left a comment

Choose a reason for hiding this comment

NicholasBlaskey Feb 21, 2024

Choose a reason for hiding this comment

carolinaecalderon Feb 21, 2024

Choose a reason for hiding this comment

carolinaecalderon commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

netlify bot commented Feb 19, 2024 •

edited

Loading