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

Add Study.study_name #157

Merged
merged 25 commits into from Sep 14, 2018
Merged

Add Study.study_name #157

merged 25 commits into from Sep 14, 2018

Conversation

toshihikoyanase
Copy link
Member

@toshihikoyanase toshihikoyanase commented Aug 15, 2018 •
edited

  • Add study_name to Study class.
  • study_name can be specified by create-study subcommand of CLI or pfnopt.create_study function.
  • This change requires schema version update of RDB.

@toshihikoyanase toshihikoyanase changed the title [WIP] Add Study.study_name Add Study.study_name Aug 15, 2018
g-votte
g-votte requested changes Aug 22, 2018
View reviewed changes
pfnopt/cli.py Outdated
@@ -44,14 +44,16 @@ def get_parser(self, prog_name):
# type: (str) -> ArgumentParser

parser = super(CreateStudy, self).get_parser(prog_name)
parser.add_argument('--study_name', default=None,
Copy link
Member

Choose a reason for hiding this comment

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

To keep consistency, please use - instead of _ for argument 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.

Thank you for your comment. --study_name was replaced with --study-name to keep the consistency with other arguments.

pfnopt/cli.py Outdated
@@ -145,6 +148,8 @@ def get_parser(self, prog_name):
'number is set to CPU counts.')
parser.add_argument('--study', help='Study UUID.')
parser.add_argument('--create-study', action='store_true', help='Create a new study.')
parser.add_argument('--study_name', default=None,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

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 also fixed this one. Thanks.

pfnopt/storages/in_memory.py Outdated
def get_study_id_from_name(self, study_name):
# type: (str) -> int

if study_name is None:
Copy link
Member

Choose a reason for hiding this comment

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

Given that study_name is typed as str as well as this method is not exposed to users, I don't think None handling is necessary here.

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. This none-value checking was removed. Thank you for your comment.

tests/storages_tests/test_storages.py Outdated

# Test not existing study.
with pytest.raises(ValueError):
storage.get_study_id_from_name('dummy-uuid')
Copy link
Member

Choose a reason for hiding this comment

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

Although it does not affect test logic itself, dummy-uuid has better be replaced with dummy-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.

dummy-uuid was replaced with dummy-name. Thank you!

tests/storages_tests/test_storages.py Outdated
@@ -96,6 +111,29 @@ def test_get_study_id_from_uuid_and_get_study_uuid_from_id(storage_init_func):
assert storage.get_study_id_from_uuid(summary.study_uuid) == summary.study_id


@parametrize_storage
Copy link
Member

Choose a reason for hiding this comment

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

StorageSupplier is our current strategic way to parametrize storage. (I think we can address it in another PR.)

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 rewrote this test with StorageSupplier. Now, I'm sure that it provide better way to control backend storages than @parametrize_storage. Thank you for your suggestion.

tests/storages_tests/test_storages.py Outdated
study_name = 'sample_study_name'
# Test existing study.
study_id = storage.create_new_study_id(study_name)
summary = storage.get_all_study_summaries()[0]
Copy link
Member

Choose a reason for hiding this comment

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

Once StorageSupplier is introduced, the number of summaries returned by storage.get_all_study_summaries() is not necessarily 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for you advice. According to tests in test_study.py, I used Study created by pfnopt.create_study instances instead of StudySummary.

@toshihikoyanase toshihikoyanase mentioned this pull request Aug 23, 2018
Merged
@iwiwi iwiwi self-requested a review September 6, 2018 05:17
iwiwi
iwiwi reviewed Sep 10, 2018
View reviewed changes
tests/storages_tests/test_storages.py Outdated

# TODO(Yanase): Use Replace StorageSupplier instead of @parametrize_storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand English here. Do you want to replace StorageSupplier by parametrize_storage, or do you want to replace parametrize_storage by StorageSupplier?

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'm sorry for bothering you due to my poor English sentence. I want to say parametrize_storage should be replaced by StorageSupplier. I fixed the comment.

@iwiwi
Copy link
Contributor

iwiwi commented Sep 10, 2018

A concern that I want to discuss: --study-name argument of minimize subcommand is confusing to me. While --study argument is only used to look up the study to be continued, --study-name is only used to specify the name of a new study to be created (under --create-study) and never used for looking up existing studies. I think it is very difficult to understand this behavior without reading code.

@toshihikoyanase
Copy link
Member Author

As you mentioned, behavior of --study-name argument is not intuitive, and users may feel uncomfortable to use it. I think we can delete this argument from minimize subcommand in this PR. It seems consistent with the future design of minimize because we have a plan to limit the storage of minimize to InMemoryDB.

iwiwi
iwiwi approved these changes Sep 13, 2018
View reviewed changes
Copy link
Contributor

@iwiwi iwiwi left a comment

Choose a reason for hiding this comment

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

LGTM

@toshihikoyanase
Copy link
Member Author

SCHEMA_VERSION was changed from 7 to 8 to avoid conflict with the change in #162.

g-votte
g-votte approved these changes Sep 14, 2018
View reviewed changes
Copy link
Member

@g-votte g-votte left a comment

Choose a reason for hiding this comment

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

LGTM!

@g-votte g-votte merged commit bec562d into master Sep 14, 2018
@g-votte g-votte deleted the add-study-name branch September 14, 2018 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iwiwi iwiwi iwiwi approved these changes

@g-votte g-votte g-votte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@toshihikoyanase @iwiwi @g-votte