fix: persist workspace id/name & experiment id for historic allocations [DET-10378] #9550

corban-beaird · 2024-06-20T22:11:27Z

Ticket

Description

Creates allocation_workspace_information table, responsible for persisting workspace id/name & experiment id for historic allocation reports. Adds new entries on successful creation of allocation for trials & NTSC tasks.

Backfills table using existing process used to retrieve workspace & experiment information for the historic allocation CSV endpoints.

Modifies logic for historic allocation report to leverage allocation_workspace_information instead of resolving based on current state.

Modifies existing e2e test to confirm workspace name is consistent when experiments are moved & deleted.

Test Plan

e2e test passes

Checklist

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

netlify · 2024-06-20T22:11:46Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`1fe4274`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/667b5ba347e9ff00083ada72

eecsliu

It looks mostly good, though I have some slight concerns about how it behaves in the event insertion into allocation_workspace_info fails but allocation starts.

master/internal/command/command.go

eecsliu · 2024-06-20T23:08:31Z

master/internal/core.go

@@ -459,38 +459,13 @@ func (m *Master) getResourceAllocations(c echo.Context) error {
 		Where("ts.event_type = 'IMAGEPULL'").
 		Group("a.allocation_id")

-	taskWorkspaceIDs := db.Bun().NewSelect().


This is just

Love to see this removed

eecsliu · 2024-06-20T23:17:58Z

master/static/migrations/20240617155550_add-allocation-workspace-table.tx.up.sql

+-- Populate the new table with existing data based on the previous method of resolving the workspace info & experiment_id
+insert into allocation_workspace_info (allocation_id, workspace_id, workspace_name, experiment_id)
+select
+	allocation_workspace.allocation_id,


nit: de-indent?

should I also de-indent all the above select columns as well?

I kinda like the indentation for clarity, but I will make sure that I'm consistent with my case (insert into & select should be caps)

master/internal/trial.go

codecov · 2024-06-24T21:21:19Z

Codecov Report

Attention: Patch coverage is 67.21311% with 20 lines in your changes missing coverage. Please review.

Project coverage is 49.82%. Comparing base (2331e5b) to head (1fe4274).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9550      +/-   ##
==========================================
- Coverage   52.20%   49.82%   -2.39%     
==========================================
  Files         753     1247     +494     
  Lines      112452   162278   +49826     
  Branches     2888     2888              
==========================================
+ Hits        58711    80847   +22136     
- Misses      53569    81259   +27690     
  Partials      172      172

Flag	Coverage Δ
backend	`43.94% <67.21%> (+9.73%)`	⬆️
harness	`63.74% <ø> (ø)`
web	`46.16% <ø> (ø)`

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

Files	Coverage Δ
master/pkg/model/task.go	`15.26% <ø> (ø)`
master/pkg/model/workspace.go	`0.00% <ø> (ø)`
master/internal/core.go	`6.51% <0.00%> (ø)`
master/internal/command/command.go	`66.80% <63.63%> (ø)`
master/internal/trial.go	`41.86% <54.54%> (ø)`
master/internal/task/postgres_allocations.go	`53.84% <77.77%> (ø)`

... and 489 files with indirect coverage changes

carolinaecalderon

looks good, modulo a few nits & making sure CircleCI is green.
If more tests can't be added in this PR I do want to see this PR linked in a backlog ticket to write them

master/internal/command/command.go

master/internal/task/postgres_allocations.go

…letion and move

… for lost entries due to experiment deletion

…in memory workspace information

…spec

azhou-determined · 2024-06-25T19:52:32Z

master/pkg/model/task.go

+	AllocationID  AllocationID `db:"allocation_id" bun:"allocation_id,notnull"`
+	ExperimentID  int          `db:"experiment_id" bun:"experiment_id"`
+	WorkspaceID   int          `db:"workspace_id" bun:"workspace_id,notnull"`
+	WorkspaceName string       `db:"workspace_name" bun:"workspace_name,notnull"`


is there a reason why we need to persist workspace name separately? is it to avoid doing another join on the workspaces table?

if at the time of the allocation/persist, the workspace (let's say ID=1) had name "myworkspace", but when fetching historical records the workspace 1 had changed names to "neworkspacename", is expected behavior to show the old name or the latest?

The intention here is to show the old name, this was primarily done to handle cases where the workspace had been deleted

azhou-determined · 2024-06-25T19:53:40Z

do we have a sense of how long the migration will take? might be worth calling out in release notes.

azhou-determined

few clarifying questions but otherwise lgtm

corban-beaird · 2024-06-25T20:08:08Z

do we have a sense of how long the migration will take? might be worth calling out in release notes.

@azhou-determined
I had concern about the migration being pretty time intensive, but @eecsliu had ran the select query that populates the values for the insert that the migration does to backfill and it returned in around 500ms on Grenoble, so I'm under the impression it won't be to performance heavy.

eecsliu

looks good! Thanks for handling it :)

gt2345

LGTM

gt2345 · 2024-06-25T21:53:57Z

master/static/migrations/20240617155550_add-allocation-workspace-table.tx.up.sql

+alloc_experiments AS (
+    SELECT
+        allocations.allocation_id,
+        rtrim(substring(task_id, '\d+?\.'), '.')::int AS experiment_id


Technically this can be any string, right? Should we have a fallback in case there is some "bad" data?

Yep! If there's not a match or if the string is null, this will return null

…ns [DET-10378] (#9550) (cherry picked from commit fa1c1e1)

corban-beaird requested a review from eecsliu June 20, 2024 22:11

corban-beaird requested review from a team as code owners June 20, 2024 22:11

corban-beaird requested review from hamidzr and MikhailKardash June 20, 2024 22:11

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

eecsliu reviewed Jun 20, 2024

View reviewed changes

corban-beaird added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Jun 24, 2024

corban-beaird requested review from carolinaecalderon June 24, 2024 18:16

corban-beaird force-pushed the corban/historic-alloc-workspace-info branch from c205145 to 2cd5f4b Compare June 24, 2024 21:18

carolinaecalderon approved these changes Jun 25, 2024

View reviewed changes

master/internal/command/command.go Outdated Show resolved Hide resolved

master/internal/task/postgres_allocations.go Show resolved Hide resolved

corban-beaird added 6 commits June 25, 2024 10:43

fix: persist workspace and experiment allocation information after de…

4f69ae6

…letion and move

chore: add e2e test cases for deletion & experiment move

3e407e6

chore: correct allocation_workspace_information definition to allow…

c0ba624

… for lost entries due to experiment deletion

refactor: move workspace persist call and drop foreign key constraint

cf14855

refactor: replace list comprehension with list()

a94f859

refactor: add integration tests, push up workspace persist call, use …

9a06d53

…in memory workspace information

corban-beaird force-pushed the corban/historic-alloc-workspace-info branch from f4e1272 to 9a06d53 Compare June 25, 2024 16:43

chore: clean up linter, and ensure mock trials have workspace in task…

fab7b5a

…spec

corban-beaird removed the request for review from MikhailKardash June 25, 2024 19:03

corban-beaird assigned azhou-determined and unassigned azhou-determined Jun 25, 2024

corban-beaird requested a review from azhou-determined June 25, 2024 19:03

azhou-determined reviewed Jun 25, 2024

View reviewed changes

azhou-determined approved these changes Jun 25, 2024

View reviewed changes

corban-beaird added 2 commits June 25, 2024 14:24

chore: ensure integration test cleans up its workspaces

a514e96

chore: cleanup e2e test workspaces

96eb30e

corban-beaird force-pushed the corban/historic-alloc-workspace-info branch from 0dd2204 to 96eb30e Compare June 25, 2024 20:59

eecsliu approved these changes Jun 25, 2024

View reviewed changes

gt2345 approved these changes Jun 25, 2024

View reviewed changes

corban-beaird added 2 commits June 25, 2024 17:44

chore: source workspace information for trials through experiment

374dae4

chore: use default workspace for testing

1fe4274

corban-beaird merged commit fa1c1e1 into main Jun 26, 2024
82 of 98 checks passed

corban-beaird deleted the corban/historic-alloc-workspace-info branch June 26, 2024 01:06

github-actions bot pushed a commit that referenced this pull request Jun 26, 2024

fix: persist workspace id/name & experiment id for historic allocatio…

a6c7918

…ns [DET-10378] (#9550) (cherry picked from commit fa1c1e1)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: persist workspace id/name & experiment id for historic allocations [DET-10378] #9550

fix: persist workspace id/name & experiment id for historic allocations [DET-10378] #9550

corban-beaird commented Jun 20, 2024 •

edited by jira bot

Loading

netlify bot commented Jun 20, 2024 •

edited

Loading

eecsliu left a comment

eecsliu Jun 20, 2024

eecsliu Jun 20, 2024

corban-beaird Jun 24, 2024

corban-beaird Jun 24, 2024

codecov bot commented Jun 24, 2024 •

edited

Loading

carolinaecalderon left a comment •

edited

Loading

azhou-determined Jun 25, 2024

corban-beaird Jun 25, 2024

azhou-determined commented Jun 25, 2024

azhou-determined left a comment

corban-beaird commented Jun 25, 2024 •

edited

Loading

eecsliu left a comment

gt2345 left a comment

gt2345 Jun 25, 2024

corban-beaird Jun 25, 2024

fix: persist workspace id/name & experiment id for historic allocations [DET-10378] #9550

fix: persist workspace id/name & experiment id for historic allocations [DET-10378] #9550

Conversation

corban-beaird commented Jun 20, 2024 • edited by jira bot Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Jun 20, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

eecsliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

carolinaecalderon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azhou-determined commented Jun 25, 2024

azhou-determined left a comment

Choose a reason for hiding this comment

corban-beaird commented Jun 25, 2024 • edited Loading

eecsliu left a comment

Choose a reason for hiding this comment

gt2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corban-beaird commented Jun 20, 2024 •

edited by jira bot

Loading

netlify bot commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

carolinaecalderon left a comment •

edited

Loading

corban-beaird commented Jun 25, 2024 •

edited

Loading