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.

Sign up for GitHub

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

Jump to bottom

feat: support config file for tensorboard [DET-3900] #1191

Merged
merged 3 commits into from
Sep 2, 2020
Merged

feat: support config file for tensorboard [DET-3900] #1191

merged 3 commits into from
Sep 2, 2020

Conversation

eecsliu
Copy link
Contributor

@eecsliu eecsliu commented Aug 27, 2020 •
edited
Loading

Description

Allow a yaml file to be passed as an argument into the tensorboard start command, providing the option of running tensorboard in different container types. Additionally adds the config arg to print the config of a specified tensorboard.

Test Plan

Tested manually. Tensorboard uses a cpu-only container by default, so specify a config file that uses a different container type (e.g. a gpu container). Use det tensorboard config tensorboard_id to verify that the specified container is being used.

Commentary (optional)

Checklist

  • 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.

@cla-bot cla-bot bot added the cla-signed label Aug 27, 2020
@eecsliu eecsliu changed the title support config file for tensorboard [DET-3900] feat: support config file for tensorboard [DET-3900] Aug 27, 2020
@eecsliu eecsliu force-pushed the tensorboard-config branch 2 times, most recently from 8f23ba9 to c9d2d6e Compare August 27, 2020 18:48
@eecsliu eecsliu requested a review from aaron276h August 27, 2020 18:55
neilconway
neilconway reviewed Aug 27, 2020
View reviewed changes
Copy link
Contributor

@neilconway neilconway left a comment

Choose a reason for hiding this comment

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

Let's also make sure we update the Tensorboard docs.

cli/determined_cli/tensorboard.py Show resolved Hide resolved
aaron276h
aaron276h reviewed Aug 27, 2020
View reviewed changes
Copy link
Contributor

@aaron276h aaron276h left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to add config_file to the list of accepted args. It would also be nice to add a det tensorboard config option that shows the config being used, similar to what we have for notebooks.

@aaron276h aaron276h assigned eecsliu and unassigned aaron276h Aug 27, 2020
@eecsliu
Copy link
Contributor Author

eecsliu commented Aug 27, 2020

This is my bad, I incorrectly moved work between branches.

@eecsliu eecsliu force-pushed the tensorboard-config branch 4 times, most recently from 1d9a54a to 3981889 Compare August 31, 2020 19:18
@eecsliu eecsliu assigned neilconway and aaron276h and unassigned eecsliu Aug 31, 2020
@eecsliu eecsliu requested a review from aaron276h August 31, 2020 19:19
aaron276h
aaron276h approved these changes Aug 31, 2020
View reviewed changes
Copy link
Contributor

@aaron276h aaron276h left a comment

Choose a reason for hiding this comment

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

Nice Work!

@aaron276h aaron276h assigned eecsliu and unassigned aaron276h Aug 31, 2020
neilconway
neilconway approved these changes Aug 31, 2020
View reviewed changes
Copy link
Contributor

@neilconway neilconway left a comment

Choose a reason for hiding this comment

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

lgtm!

docs/how-to/tensorboard.txt Outdated
@@ -51,6 +51,20 @@ TensorBoard for multiple experiments use
metrics from persistent storage. It may take up to 5 minutes for TensorBoard
to receive data and render visualizations.

Specifying a Command Config
Copy link
Contributor

Choose a reason for hiding this comment

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

"Customizing TensorBoards" or something similar?

docs/how-to/tensorboard.txt
can be found in the :ref:`command-notebook-configuration`.

To launch Tensorboard with a config file, use
``det tensorboard start <experiment-id> --config-file=my_config.yaml``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an example of a configuration customization we expect to be somewhat common?

docs/how-to/tensorboard.txt Outdated
Determined supports initializing TensorBoard with a config file. This
feature can be useful for running TensorBoard with a specific container
image or with a different resource allocation. Configuration settings
can be found in the :ref:`command-notebook-configuration`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the command-notebook-configuration doc to specify how that configuration can be applied to TensorBoards as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a clarification in the command-notebook-configuration, but everything there should be compatible with tensorboard.

@neilconway neilconway removed their assignment Aug 31, 2020
@eecsliu eecsliu force-pushed the tensorboard-config branch 2 times, most recently from d8429ef to 25dd59d Compare September 1, 2020 20:56
@eecsliu eecsliu assigned neilconway and unassigned eecsliu Sep 1, 2020
neilconway
neilconway reviewed Sep 1, 2020
View reviewed changes
Copy link
Contributor

@neilconway neilconway left a comment

Choose a reason for hiding this comment

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

Can we fully update command-notebook-config.txt to give parity to TensorBoards? e.g., the title of the page still only talks about commands and notebooks, not TensorBoards. Commands are much less important than TensorBoards and notebooks -- for brevity if we want to talk about only notebooks and TBs, that would be fine with me. (And then maybe in passing that the same thing works for det cmd).

@neilconway neilconway assigned eecsliu and unassigned neilconway and aaron276h Sep 1, 2020
@eecsliu eecsliu assigned neilconway and unassigned neilconway and eecsliu Sep 1, 2020
@eecsliu eecsliu merged commit 4041e5b into determined-ai:master Sep 2, 2020
@eecsliu eecsliu deleted the tensorboard-config branch September 2, 2020 20:33
@dannysauer dannysauer added this to the 0.13.2 milestone Feb 6, 2024
rb-determined-ai pushed a commit that referenced this pull request Feb 29, 2024
@maxrussell @djanicekpach
fix: stop double substituting unauthorized error (#1191)
571b5e6
amandavialva01 pushed a commit that referenced this pull request Mar 18, 2024
@maxrussell @determined-ci
fix: stop double substituting unauthorized error (#1191)
f7d06b3
eecsliu pushed a commit that referenced this pull request Apr 18, 2024
@maxrussell @determined-ci
fix: stop double substituting unauthorized error (#1191)
6deb8a1
eecsliu pushed a commit to determined-ai/determined-release-testing that referenced this pull request Apr 22, 2024
@maxrussell @eecsliu
fix: stop double substituting unauthorized error (determined-ai#1191)
6ab3447
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilconway neilconway neilconway approved these changes

@aaron276h aaron276h aaron276h approved these changes

Assignees

@neilconway neilconway

Labels
cla-signed
Projects
None yet
Milestone
0.13.2
Development

Successfully merging this pull request may close these issues.
4 participants
@eecsliu @neilconway @aaron276h @dannysauer