determined-ai / determined

Determined is an open-source machine learning platform that simplifies distributed training, hyperparameter tuning, experiment tracking, and resource management. Works with PyTorch and TensorFlow.
https://determined.ai
Apache License 2.0
2.93k stars 347 forks source link

feat: add a new API endpoint to bind all unbound workspaces to autocreated namespaces #9602

Open salonig23 opened 2 weeks ago

salonig23 commented 2 weeks ago

Ticket

DET-10226

Description

Test Plan

Checklist

netlify[bot] commented 2 weeks ago

Deploy Preview for determined-ui ready!

Name Link
Latest commit 4b8e85bc7092bd139c584175f855c986be990b5d
Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6684680a41722f00084f0f89
Deploy Preview https://deploy-preview-9602--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.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 45.09804% with 28 lines in your changes missing coverage. Please review.

Project coverage is 51.79%. Comparing base (be2a357) to head (87d11bd).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## wksp-namespace-binding #9602 +/- ## ========================================================== + Coverage 51.75% 51.79% +0.03% ========================================================== Files 1255 1255 Lines 154083 154132 +49 Branches 3120 3120 ========================================================== + Hits 79752 79828 +76 + Misses 74176 74149 -27 Partials 155 155 ``` | [Flag](https://app.codecov.io/gh/determined-ai/determined/pull/9602/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | Coverage Δ | | |---|---|---| | [backend](https://app.codecov.io/gh/determined-ai/determined/pull/9602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | `43.93% <45.09%> (+0.10%)` | :arrow_up: | | [harness](https://app.codecov.io/gh/determined-ai/determined/pull/9602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | `72.61% <ø> (ø)` | | | [web](https://app.codecov.io/gh/determined-ai/determined/pull/9602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | `49.15% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/determined-ai/determined/pull/9602?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | Coverage Δ | | |---|---|---| | [master/internal/api\_workspace.go](https://app.codecov.io/gh/determined-ai/determined/pull/9602?src=pr&el=tree&filepath=master%2Finternal%2Fapi_workspace.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-bWFzdGVyL2ludGVybmFsL2FwaV93b3Jrc3BhY2UuZ28=) | `61.19% <45.09%> (+1.72%)` | :arrow_up: | ... and [6 files with indirect coverage changes](https://app.codecov.io/gh/determined-ai/determined/pull/9602/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai)
corban-beaird commented 12 hours ago

Just spit balling, but would it be worth having these structured as instead 2 endpoints, a GET & a POST, where the get will return the set of workspace_ids that aren't associated with a namespace & then a POST responsible for the auto-creation & binding of the namespace to the given workspaces based on the provided ids? The POST could potentially be framed as a bulk action endpoint.

My concern is this seems like a very niche endpoint that will likely only get used once per determined instance. It's absolutely okay to disagree, overall the code looks solid!

maxrussell commented 10 hours ago

I agree and had the same thought, Corban. I prefer explicit APIs over implicit ones. And in my experience it's usually worth the performance hit of roundtripping the list of ids back to a client. Something like

  // Binds all unbound workspaces to new auto-created namespaces.
  rpc GetUnboundWorkspaces(
      GetUnboundWorkspacesRequest)
      returns (GetUnboundWorkspacesResponse) {
    option (google.api.http) = {
      get: "/api/v1/workspaces/unbound",
    };
    option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = {
      tags: "Workspaces"
    };
  }

  // Binds the specified workspaces to new auto-created namespaces if they're unbound.
  rpc BulkAutoCreateWorkspaceNamespaceBindings(
      BulkAutoCreateWorkspaceNamespaceBindingsRequest)
      returns (BulkAutoCreateWorkspaceNamespaceBindingsResponse) {
    option (google.api.http) = {
      post: "/api/v1/workspaces/auto-namespace-bindings",
    };
    option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = {
      tags: "Workspaces"
    };
  }

// Request for finding all unbound workspaces.
message GetUnboundWorkspacesRequest {}

// Response to GetUnboundWorkspacesRequest.
message GetUnboundWorkspacesResponse {
  // The list of returned workspace ids.
  repeated int32 = 1;
  // Pagination information of the full dataset.
  Pagination pagination = 2;
}

// Request for binding the specified unbound workspaces to new auto-created namespaces.
message BulkAutoCreateWorkspaceNamespaceBindingsRequest {
  // The list of returned workspace ids.
  repeated int32 = 1;
}

// Response to BulkAutoCreateWorkspaceNamespaceBindingsRequest.
message BulkAutoCreateWorkspaceNamespaceBindingsResponse {}