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

Use callback to report results to MLflow. #799

Merged
merged 4 commits into from
Dec 18, 2019

Conversation

toshihikoyanase
Copy link
Member

This PR uses a callback function to report optimization results to MLflow.
In this PR, I used study_name as run_name of MLflow, but I'm not fully confident if it is a relevant usage of run_name. Please let me know if you have any ideas about it.

image

@harupy
Copy link
Contributor

harupy commented Dec 17, 2019

@toshihikoyanase The script looks much simpler now. Thanks!

@toshihikoyanase
Copy link
Member Author

@harupy Thank you for your comment!
I'll merge #801 to fix the CI failures for further review.



if __name__ == '__main__':
study = optuna.create_study(direction='minimize')
study.optimize(objective, n_trials=100, timeout=600)
study = optuna.create_study(study_name='keras-wine', direction='minimize')
Copy link
Member

Choose a reason for hiding this comment

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

I think 'study_name' argument can be removed since this example doesn't use an RDB storage.

Copy link
Member Author

@toshihikoyanase toshihikoyanase Dec 17, 2019

Choose a reason for hiding this comment

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

Thank you for your comment.
I set it to show somewhat meaningful names rather than no-name-xxx in MLflow UI. But we can distinguish studies if we remove the name. So I'll remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed: ef61f41

I also removed direction option from create_study because the default direction is minimize in the commit 5c4942c.

The new screenshot of mlflow ui is as follows:

image

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

This is just a thought. Current default study names are not human-friendly, so it might worth to improve the name generation logic in the future (e.g., adopt the naming rule being used by Docker).

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you. I posted an issue about the name generation as #803. Thank you.

By the way, the following part of my comment #799 (comment) is not true because all studies created with InMemoryStorage have the same default name (i.e., no-name-00000000-0000-0000-0000-000000000000). I'm sorry to give you a wrong information.

But we can distinguish studies if we remove the name.

Copy link
Member

Choose a reason for hiding this comment

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

I posted an issue about the name generation as #803.

Thanks!

all studies created with InMemoryStorage have the same default name (i.e., no-name-00000000-0000-0000-0000-000000000000).

Oops. That is, there is no actual merit to use the default name...?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. Maybe storage classes should share the name generator instead of adding name generation code for InMemoryStorage to this example.

Copy link
Member

@sile sile left a comment

Choose a reason for hiding this comment

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

LGTM

@sile sile added the example label Dec 18, 2019
@sile sile added this to the v0.20.0 milestone Dec 18, 2019
@codecov-io
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

Merging #799 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #799   +/-   ##
=======================================
  Coverage   90.15%   90.15%           
=======================================
  Files         106      106           
  Lines        8769     8769           
=======================================
  Hits         7906     7906           
  Misses        863      863

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 d9d8188...5c4942c. Read the comment docs.

Copy link
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

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

LGTM!

@hvy hvy merged commit 0389962 into optuna:master Dec 18, 2019
@toshihikoyanase toshihikoyanase deleted the use-callback-in-mlflow-example branch January 29, 2020 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants