Skip to content
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

chore: refactor proto, schema, and jobservice for multiRM #8875

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

carolinaecalderon
Copy link
Contributor

@carolinaecalderon carolinaecalderon commented Feb 23, 2024

Description

Make changes to api calls to include ResourceManager field for requests that call implemented agentRM/kubernetesRM ResourceManager methods.
Bubble up these changes to the jobservice & ResourceManager interface.

Test Plan

Pass existing tests.

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-43

@cla-bot cla-bot bot added the cla-signed label Feb 23, 2024
Copy link

netlify bot commented Feb 23, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 13be116
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65de150c052f5700082f7258
😎 Deploy Preview https://deploy-preview-8875--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

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

Project coverage is 47.55%. Comparing base (c029327) to head (13be116).
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #8875    +/-   ##
========================================
  Coverage   47.54%   47.55%            
========================================
  Files        1065     1065            
  Lines      170218   170421   +203     
  Branches     2241     2241            
========================================
+ Hits        80937    81041   +104     
- Misses      89123    89222    +99     
  Partials      158      158            
Flag Coverage Δ
backend 43.34% <15.58%> (-0.01%) ⬇️
harness 63.74% <25.60%> (-0.07%) ⬇️
web 42.56% <57.96%> (+0.03%) ⬆️

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

Files Coverage Δ
master/internal/command/command.go 67.96% <100.00%> (+0.13%) ⬆️
master/internal/rm/agentrm/resource_pool.go 25.16% <ø> (ø)
master/internal/rm/kubernetesrm/resource_pool.go 0.00% <ø> (ø)
master/pkg/model/experiment_config.go 39.24% <ø> (ø)
master/pkg/schemas/expconf/experiment_config.go 64.21% <ø> (ø)
master/pkg/schemas/zgen_schemas.go 1.24% <ø> (ø)
master/internal/api_experiment.go 54.84% <0.00%> (ø)
master/internal/api_job.go 0.00% <0.00%> (ø)
master/internal/experiment_job_service.go 0.00% <0.00%> (ø)
master/pkg/tasks/task_generic.go 55.31% <0.00%> (ø)
... and 9 more

... and 4 files with indirect coverage changes

@carolinaecalderon carolinaecalderon force-pushed the carolinac/multirm-proto-schema-changes branch from b834b79 to dea6aad Compare February 23, 2024 19:10
@carolinaecalderon carolinaecalderon marked this pull request as ready for review February 24, 2024 00:19
@carolinaecalderon carolinaecalderon requested review from a team as code owners February 24, 2024 00:19
@carolinaecalderon carolinaecalderon requested review from rb-determined-ai and removed request for ioga February 27, 2024 17:00
Comment on lines +55 to +61
"resource_manager": {
"type": [
"string",
"null"
],
"default": ""
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this is a dumb question... but why do users need to specify the resource manager and the resource pool?

Why don't we just use resource pools and they specify those? This is a layer of hierarchy that has never made sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is for use in the multi-RM project -- multiple resource managers can have resource pools with the same name. In that case, we need to use the resource manager name to qualify & identify it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just disallow multiple resource pools with the same name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear the answer to this is "because slurm".

Gross, but ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW @rb-determined-ai , the way we're approaching this is users will only need to specify if there is a conflict, still. Otherwise no one knows this param exists.

@carolinaecalderon carolinaecalderon merged commit 7c6bec9 into main Feb 27, 2024
72 of 87 checks passed
@carolinaecalderon carolinaecalderon deleted the carolinac/multirm-proto-schema-changes branch February 27, 2024 20:14
carolinaecalderon added a commit that referenced this pull request Mar 5, 2024
carolinaecalderon added a commit that referenced this pull request Mar 6, 2024
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.
carolinaecalderon added a commit that referenced this pull request Mar 6, 2024
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.
carolinaecalderon added a commit that referenced this pull request Mar 6, 2024
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.
carolinaecalderon added a commit that referenced this pull request Mar 7, 2024
* 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.
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
* 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.
eecsliu pushed a commit to determined-ai/determined-release-testing that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants