feat: Add tensorboard delete command to CLI #8227

AmanuelAaron · 2023-10-23T21:32:07Z

Description

Creating a CLI command for deleting tensorboard files from a given experiment ID. We want some of the existing logic for checkpointGC to perform the file deleteion.

Usage of command would be det tensorboards delete <experiment-id>

Test Plan

Create an experiment
Example: det e create examples/tutorials/mnist_pytorch/const.yaml examples/tutorials/mnist_pytorch/
Verify that Tensorboard files exist:

$ cd /tmp/determined-cp/
$ find . -name 'tensorboard*' -type d
$ >> <folder-name>/tensorboard
$ cd <folder-name>/tensorboard/experiment/<exp-id>/trial/<exp-id>
$ ls

Delete tensorboard files:
det e delete-tb-files <exp-id>
Check that files are deleted:

$ cd <folder-name>/tensorboard/experiment/
$ ls (folder for <exp-id> should be gone)

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

DET-9865

netlify · 2023-10-23T21:32:13Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`5578b82`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6545567816b4c30008461cc0

master/pkg/tasks/task_gc.go

master/internal/tensorboard_gc.go

stoksc

could you add an end to end test for this that verifies the TB files are actually deleted. i think just an e2e cpu is fine. if we want per-storage manager tests we can integration test it, i just would like at least one e2e feature test for this.

determined-ci · 2023-10-30T21:46:18Z

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

e2e_tests/tests/command/test_tensorboard.py

stoksc · 2023-11-01T18:02:43Z

harness/determined/cli/tensorboard.py

+                "delete",
+                delete_tensorboard,
+                "delete TensorBoard files associate with the proived experiment ID",


@azhou-determined or someone from ml sys should review this change.

personally i think it could be a bit confusing for a user. maybe det e delete-tensorboard-files :ID is more clear about what it does but a pain to type.

"tensorboard" in this context so far means "a running tensorboard application" not "tensorboard files" so "det tensorboard delete ..." makes me think it would do something similar to "det tensorboard kill ...".

agreed that det tensorboard delete reads like you want to delete the tensorboard instance.

maybe something like delete-files, or delete-experiment?

i like det tensorboard delete-files --experiment-id $ID or det experiment delete-tensorboard-files $ID. preferring the latter.

From offline discussion with @wes-turner & @stoksc. We went with det e delete-tb-files

wes-turner

Python side 👍

stoksc

looks great

master/internal/api_experiment.go

stoksc · 2023-11-04T00:46:06Z

master/internal/api_experiment.go

+		return nil, err
+	}
+
+	exp, err := db.ExperimentByID(context.TODO(), int(req.ExperimentId))


I missed this sorry, but we should be passing ctx here and on 2991.

AmanuelAaron requested review from stoksc, carolinaecalderon and NicholasBlaskey October 23, 2023 21:32

cla-bot bot added the cla-signed label Oct 23, 2023

stoksc reviewed Oct 26, 2023

View reviewed changes

master/pkg/tasks/task_gc.go Outdated Show resolved Hide resolved

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

stoksc reviewed Oct 30, 2023

View reviewed changes

cla-bot bot removed the cla-signed label Oct 30, 2023

determined-ci requested a review from a team October 30, 2023 21:46

determined-ci added the documentation Improvements or additions to documentation label Oct 30, 2023

add tensorboard_gc.go

206ceb4

AmanuelAaron force-pushed the tbs_delete branch from 17fce1f to 206ceb4 Compare October 30, 2023 22:23

cla-bot bot added the cla-signed label Oct 30, 2023

determined-ci removed the documentation Improvements or additions to documentation label Oct 30, 2023

AmanuelAaron added 4 commits October 30, 2023 18:27

remove tensorboard_gc.go

f6ff4ad

add generated proto files

bb9eedf

lint

ea305b7

add e2e test

5492142

AmanuelAaron marked this pull request as ready for review October 31, 2023 17:30

AmanuelAaron requested review from a team as code owners October 31, 2023 17:30

AmanuelAaron requested a review from azhou-determined October 31, 2023 17:30

lint

97e5123

cla-bot bot removed the cla-signed label Oct 31, 2023

AmanuelAaron added the User-facing API Change label Oct 31, 2023

determined-ci added the documentation Improvements or additions to documentation label Nov 1, 2023

stoksc reviewed Nov 1, 2023

View reviewed changes

determined-ai deleted a comment from cla-bot bot Nov 1, 2023

AmanuelAaron added 5 commits November 2, 2023 16:46

change cli command to delete-files

a8d528d

update doc and tests

83341df

api delete -> deleteFiles

5393265

fix bindings

d3d9a0e

lint docs

05f919d

determined-ai deleted a comment from cla-bot bot Nov 3, 2023

AmanuelAaron added 6 commits November 3, 2023 11:21

doc fix

4121572

add det e delete-tb-files

2531455

remove command for tensorboard cli

d43f003

update docs

20e5742

move api from tensorboard to experiment

2c102f9

lint

72afd4f

AmanuelAaron requested review from wes-turner and removed request for NicholasBlaskey November 3, 2023 20:03

wes-turner approved these changes Nov 3, 2023

View reviewed changes

stoksc approved these changes Nov 3, 2023

View reviewed changes

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

cla-bot bot removed the cla-signed label Nov 3, 2023

AmanuelAaron force-pushed the tbs_delete branch 2 times, most recently from a0aeea3 to f818046 Compare November 3, 2023 20:17

spacing

5578b82

AmanuelAaron force-pushed the tbs_delete branch from f818046 to 5578b82 Compare November 3, 2023 20:22

cla-bot bot added the cla-signed label Nov 3, 2023

determined-ai deleted a comment from cla-bot bot Nov 3, 2023

AmanuelAaron merged commit 1966373 into main Nov 3, 2023
74 of 84 checks passed

AmanuelAaron deleted the tbs_delete branch November 3, 2023 20:59

wes-turner mentioned this pull request Nov 3, 2023

feat: delete TB files from the SDK #8329

Merged

4 tasks

stoksc reviewed Nov 4, 2023

View reviewed changes

dannysauer added this to the 0.26.4 milestone Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add tensorboard delete command to CLI #8227

feat: Add tensorboard delete command to CLI #8227

AmanuelAaron commented Oct 23, 2023 •

edited

Loading

netlify bot commented Oct 23, 2023 •

edited

Loading

stoksc left a comment

determined-ci commented Oct 30, 2023

stoksc Nov 1, 2023

azhou-determined Nov 2, 2023

stoksc Nov 2, 2023

AmanuelAaron Nov 3, 2023

wes-turner left a comment

stoksc left a comment

stoksc Nov 4, 2023

AmanuelAaron Nov 6, 2023 •

edited

Loading

feat: Add tensorboard delete command to CLI #8227

feat: Add tensorboard delete command to CLI #8227

Conversation

AmanuelAaron commented Oct 23, 2023 • edited Loading

Description

Test Plan

Checklist

Ticket

netlify bot commented Oct 23, 2023 • edited Loading

✅ Deploy Preview for determined-ui canceled.

stoksc left a comment

Choose a reason for hiding this comment

determined-ci commented Oct 30, 2023

stoksc Nov 1, 2023

Choose a reason for hiding this comment

azhou-determined Nov 2, 2023

Choose a reason for hiding this comment

stoksc Nov 2, 2023

Choose a reason for hiding this comment

AmanuelAaron Nov 3, 2023

Choose a reason for hiding this comment

wes-turner left a comment

Choose a reason for hiding this comment

stoksc left a comment

Choose a reason for hiding this comment

stoksc Nov 4, 2023

Choose a reason for hiding this comment

AmanuelAaron Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

AmanuelAaron commented Oct 23, 2023 •

edited

Loading

netlify bot commented Oct 23, 2023 •

edited

Loading

AmanuelAaron Nov 6, 2023 •

edited

Loading