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

Add TensorBoard integration. #1244

Merged
merged 34 commits into from Jul 3, 2020

Conversation

VladSkripniuk
Copy link
Contributor

fix #1060 .

Description of the changes

This PR introduces optuna.integration.TensorBoardCallback. This callback saves values of objective function and parameters to HParams dashboard.

@VladSkripniuk
Copy link
Contributor Author

To let Tensorboard know the range of values for parameters I use param_distributions parameter in TensorBoardCallback.__init__. I think we need to discuss if we can somehow pass the distribution from trial.suggest to the callback.

@codecov-io
Copy link

Codecov Report

Merging #1244 into master will decrease coverage by 0.09%.
The diff coverage is 73.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
- Coverage   85.97%   85.88%   -0.10%     
==========================================
  Files          92       93       +1     
  Lines        6751     6800      +49     
==========================================
+ Hits         5804     5840      +36     
- Misses        947      960      +13     
Impacted Files Coverage Δ
optuna/integration/tensorboard.py 73.46% <73.46%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b6e303...36d38a5. Read the comment docs.

Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thank you for the significant PR! I have some comments on optuna/integration/tensorboard.py.

In my opinion, it seems that specifying thee distribution classes in optuna/distributions confuses users, who are not familiar with Optuna deeply. How about letting the user simply specify the type of distribution and the information needed (low, high, choices, etc.) in the form of a tuple? It is sufficient to create the class of optuna/distributions internally in optuna/integration/tensorboard.py.

Comment on lines 39 to 43
Args:
metric_name:
Name of the metric. Since the metric itself is just a number,
`metric_name` can be used to give it a name. So you know later
if it was roc-auc or accuracy.
Copy link
Member

Choose a reason for hiding this comment

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

Please specify whole information of all arguments. (dirname and param_distributions)

optuna/integration/tensorboard.py Outdated Show resolved Hide resolved
VladSkripniuk and others added 3 commits May 20, 2020 14:21
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I have a minor comment.

Comment on lines 17 to 18
def train_test_model(num_units, dropout_rate, optimizer):
# type: (int, float, str) -> float
Copy link
Member

Choose a reason for hiding this comment

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

We can use the new style for the type hinting. For example, please refer to here.

@codecov-commenter
Copy link

Codecov Report

Merging #1244 into master will increase coverage by 0.90%.
The diff coverage is 70.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
+ Coverage   85.97%   86.87%   +0.90%     
==========================================
  Files          92       95       +3     
  Lines        6751     7298     +547     
==========================================
+ Hits         5804     6340     +536     
- Misses        947      958      +11     
Impacted Files Coverage Δ
optuna/integration/tensorboard.py 70.76% <70.76%> (ø)
optuna/trial/_frozen.py 81.25% <0.00%> (-3.75%) ⬇️
optuna/integration/chainermn.py 75.70% <0.00%> (-1.13%) ⬇️
optuna/testing/storage.py 82.35% <0.00%> (-0.51%) ⬇️
optuna/storages/rdb/models.py 95.19% <0.00%> (-0.44%) ⬇️
optuna/storages/rdb/storage.py 95.83% <0.00%> (-0.39%) ⬇️
optuna/pruners/hyperband.py 97.84% <0.00%> (ø)
optuna/pruners/successive_halving.py 93.58% <0.00%> (ø)
optuna/multi_objective/samplers/__init__.py 100.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b6e303...3e3c4a3. Read the comment docs.

Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the update! I have some minor comments.

Comment on lines 6 to 8
if type_checking.TYPE_CHECKING:
from typing import Dict # NOQA
from typing import Any # NOQA
Copy link
Member

Choose a reason for hiding this comment

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

In the new type hinting style, we can remove the type_checking, just import typing/Dict or Any outside if type_checking.TYPE_CHECKING: and remove NOQAs.

Comment on lines 5 to 9
if type_checking.TYPE_CHECKING:
from typing import Any # NOQA
from typing import Dict # NOQA
from typing import Optional # NOQA
from typing import Tuple # NOQA
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@HideakiImamura
Copy link
Member

Hi @VladSkripniuk. I would like to discuss the user interface of TensorBoardCallback. I think that we can remove the param_distributions argument by automatically looking up the first finished trial. And it might be possible to add parameters by looking up finished trials automatically. What do you think?

@HideakiImamura HideakiImamura self-assigned this May 28, 2020
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Hi @VladSkripniuk! I'm sorry I left a vague comment. I'm going to comment specifically on what I was thinking. Could you take a look?

optuna/integration/tensorboard.py Outdated Show resolved Hide resolved
optuna/integration/tensorboard.py Outdated Show resolved Hide resolved
optuna/integration/tensorboard.py Outdated Show resolved Hide resolved
optuna/integration/tensorboard.py Outdated Show resolved Hide resolved
examples/tensorboard_simple.py Outdated Show resolved Hide resolved
examples/tensorboard_simple.py Outdated Show resolved Hide resolved
examples/tensorboard_simple.py Outdated Show resolved Hide resolved
tests/integration_tests/test_tensorboard.py Outdated Show resolved Hide resolved
tests/integration_tests/test_tensorboard.py Outdated Show resolved Hide resolved
tests/integration_tests/test_tensorboard.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Jun 22, 2020
VladSkripniuk and others added 10 commits June 25, 2020 19:16
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
@VladSkripniuk
Copy link
Contributor Author

Hi @HideakiImamura thank for your great help! I've applied proposed changes.

@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label Jun 28, 2020
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Basically, LGTM. I left some minor comments.

examples/tensorboard_simple.py Outdated Show resolved Hide resolved
examples/tensorboard_simple.py Outdated Show resolved Hide resolved
examples/tensorboard_simple.py Outdated Show resolved Hide resolved
optuna/integration/tensorboard.py Outdated Show resolved Hide resolved
tests/integration_tests/test_tensorboard.py Outdated Show resolved Hide resolved
tests/integration_tests/test_tensorboard.py Outdated Show resolved Hide resolved
VladSkripniuk and others added 6 commits June 30, 2020 08:52
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for your quick updates! I have other minor comments. Could you take a look?

examples/tensorboard_simple.py Outdated Show resolved Hide resolved
tests/integration_tests/test_tensorboard.py Outdated Show resolved Hide resolved
tests/integration_tests/test_tensorboard.py Outdated Show resolved Hide resolved
VladSkripniuk and others added 3 commits July 1, 2020 10:40
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for your swift action! LGTM!

@toshihikoyanase toshihikoyanase added feature Change that does not break compatibility, but affects the public interfaces. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. labels Jul 2, 2020
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your great work!

image

I think we'll make a followup PR:

  • Add experimental decorator
  • Filter out non-COMPLETE trials in __call__
  • Skip execution of the example in Python 3.8.

@toshihikoyanase toshihikoyanase merged commit c927373 into optuna:master Jul 3, 2020
@toshihikoyanase toshihikoyanase added this to the v2.0.0 milestone Jul 3, 2020
@toshihikoyanase toshihikoyanase modified the milestones: v2.0.0-rc0, v2.0.0 Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Change that does not break compatibility, but affects the public interfaces. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Add integration with Tensorboard
5 participants