From 8c65be23faacba8bb33cd6d3de87da966c3f6ad4 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Mon, 13 Aug 2018 15:39:25 +0900 Subject: [PATCH 01/21] Add study_name attribute to Study. --- pfnopt/storages/base.py | 18 ++++++++- pfnopt/storages/in_memory.py | 24 +++++++++++- pfnopt/storages/rdb/models.py | 21 +++++++++- pfnopt/storages/rdb/storage.py | 29 ++++++++++++-- pfnopt/structs.py | 1 + pfnopt/study.py | 6 ++- .../storages_tests/rdb_tests/test_storage.py | 3 ++ tests/storages_tests/test_storages.py | 39 +++++++++++++++++++ 8 files changed, 132 insertions(+), 9 deletions(-) diff --git a/pfnopt/storages/base.py b/pfnopt/storages/base.py index da4ebb7163..576cf08827 100644 --- a/pfnopt/storages/base.py +++ b/pfnopt/storages/base.py @@ -5,6 +5,7 @@ from typing import Any # NOQA from typing import Dict # NOQA from typing import List # NOQA +from typing import Optional # NOQA from typing import Tuple # NOQA from pfnopt import distributions # NOQA @@ -19,8 +20,8 @@ class BaseStorage(object): # Basic study manipulation @abc.abstractmethod - def create_new_study_id(self): - # type: () -> int + def create_new_study_id(self, study_name=None): + # type: (Optional[str]) -> int raise NotImplementedError @@ -59,6 +60,19 @@ def get_study_uuid_from_id(self, study_id): raise NotImplementedError + @abc.abstractmethod + def get_study_id_from_name(self, study_name): + # type: (Optional[str]) -> int + + # study_name can be None when it is specified by StudySummary.study_name. + raise NotImplementedError + + @abc.abstractmethod + def get_study_name_from_id(self, study_id): + # type: (int) -> Optional[str] + + raise NotImplementedError + @ abc.abstractmethod def get_study_task(self, study_id): # type: (int) -> structs.StudyTask diff --git a/pfnopt/storages/in_memory.py b/pfnopt/storages/in_memory.py index bc9687c9a2..e80900668f 100644 --- a/pfnopt/storages/in_memory.py +++ b/pfnopt/storages/in_memory.py @@ -4,6 +4,7 @@ from typing import Any # NOQA from typing import Dict # NOQA from typing import List # NOQA +from typing import Optional # NOQA from pfnopt import distributions # NOQA from pfnopt.storages import base @@ -22,6 +23,7 @@ def __init__(self): self.param_distribution = {} # type: Dict[str, distributions.BaseDistribution] self.task = structs.StudyTask.NOT_SET self.study_user_attrs = {} # type: Dict[str, Any] + self.study_name = None # type: Optional[str] self._lock = threading.Lock() @@ -36,9 +38,10 @@ def __setstate__(self, state): self.__dict__.update(state) self._lock = threading.Lock() - def create_new_study_id(self): - # type: () -> int + def create_new_study_id(self, study_name=None): + # type: (Optional[str]) -> int + self.study_name = study_name self.study_user_attrs[base.SYSTEM_ATTRS_KEY] = {} return IN_MEMORY_STORAGE_STUDY_ID # TODO(akiba) @@ -70,6 +73,22 @@ def get_study_uuid_from_id(self, study_id): self._check_study_id(study_id) return IN_MEMORY_STORAGE_STUDY_UUID + def get_study_id_from_name(self, study_name): + # type: (Optional[str]) -> int + + if study_name is None: + raise ValueError("study_name is supposed to be str, not None.") + if study_name != self.study_name: + raise ValueError("No such study {}.".format(study_name)) + + return IN_MEMORY_STORAGE_STUDY_ID + + def get_study_name_from_id(self, study_id): + # type: (int) -> Optional[str] + + self._check_study_id(study_id) + return self.study_name + def get_study_task(self, study_id): # type: (int) -> structs.StudyTask @@ -96,6 +115,7 @@ def get_all_study_summaries(self): return [structs.StudySummary( study_id=IN_MEMORY_STORAGE_STUDY_ID, study_uuid=IN_MEMORY_STORAGE_STUDY_UUID, + study_name=self.study_name, task=self.task, best_trial=best_trial, user_attrs=copy.deepcopy(self.study_user_attrs), diff --git a/pfnopt/storages/rdb/models.py b/pfnopt/storages/rdb/models.py index d412409e95..118b5aaa00 100644 --- a/pfnopt/storages/rdb/models.py +++ b/pfnopt/storages/rdb/models.py @@ -18,7 +18,7 @@ from pfnopt.structs import StudyTask from pfnopt.structs import TrialState -SCHEMA_VERSION = 5 +SCHEMA_VERSION = 6 NOT_FOUND_MSG = 'Record does not exist.' @@ -29,6 +29,7 @@ class StudyModel(BaseModel): __tablename__ = 'studies' study_id = Column(Integer, primary_key=True) study_uuid = Column(String(255), unique=True) + study_name = Column(String(255), unique=True) task = Column(Enum(StudyTask), nullable=False) @classmethod @@ -67,6 +68,24 @@ def find_or_raise_by_uuid(cls, study_uuid, session): return study + @classmethod + def find_by_name(cls, study_name, session): + # type: (str, orm.Session) -> Optional[StudyModel] + + study = session.query(cls).filter(cls.study_name == study_name).one_or_none() + + return study + + @classmethod + def find_or_raise_by_name(cls, study_name, session): + # type: (str, orm.Session) -> StudyModel + + study = cls.find_by_name(study_name, session) + if study is None: + raise ValueError(NOT_FOUND_MSG) + + return study + @classmethod def all(cls, session): # type: (orm.Session) -> List[StudyModel] diff --git a/pfnopt/storages/rdb/storage.py b/pfnopt/storages/rdb/storage.py index acaaec9435..2dabc24f47 100644 --- a/pfnopt/storages/rdb/storage.py +++ b/pfnopt/storages/rdb/storage.py @@ -40,8 +40,8 @@ def __init__(self, url, connect_args=None): self._check_table_schema_compatibility() self.logger = logging.get_logger(__name__) - def create_new_study_id(self): - # type: () -> int + def create_new_study_id(self, study_name=None): + # type: (Optional[str]) -> int session = self.scoped_session() @@ -51,7 +51,8 @@ def create_new_study_id(self): if study is None: break - study = models.StudyModel(study_uuid=study_uuid, task=structs.StudyTask.NOT_SET) + study = models.StudyModel(study_uuid=study_uuid, study_name=study_name, + task=structs.StudyTask.NOT_SET) session.add(study) session.commit() @@ -112,6 +113,27 @@ def get_study_uuid_from_id(self, study_id): return study.study_uuid + def get_study_id_from_name(self, study_name): + # type: (Optional[str]) -> int + + if study_name is None: + raise ValueError("study_name is supposed to be str, not None.") + + session = self.scoped_session() + + study = models.StudyModel.find_or_raise_by_name(study_name, session) + + return study.study_id + + def get_study_name_from_id(self, study_id): + # type: (int) -> Optional[str] + + session = self.scoped_session() + + study = models.StudyModel.find_or_raise_by_id(study_id, session) + + return study.study_name + def get_study_task(self, study_id): # type: (int) -> structs.StudyTask @@ -172,6 +194,7 @@ def get_all_study_summaries(self): study_sumarries.append(structs.StudySummary( study_id=study_model.study_id, study_uuid=study_model.study_uuid, + study_name=study_model.study_name, task=self.get_study_task(study_model.study_id), best_trial=best_trial, user_attrs=self.get_study_user_attrs(study_model.study_id), diff --git a/pfnopt/structs.py b/pfnopt/structs.py index c64386a25a..f4bbf626ed 100644 --- a/pfnopt/structs.py +++ b/pfnopt/structs.py @@ -47,6 +47,7 @@ class FrozenTrial( 'StudySummary', [('study_id', int), ('study_uuid', str), + ('study_name', Optional[str]), ('task', StudyTask), ('best_trial', Optional[FrozenTrial]), ('user_attrs', Dict[str, Any]), diff --git a/pfnopt/study.py b/pfnopt/study.py index e2292f9459..448dc2d50d 100644 --- a/pfnopt/study.py +++ b/pfnopt/study.py @@ -52,6 +52,7 @@ def __init__( storage, # type: Union[None, str, storages.BaseStorage] sampler=None, # type: samplers.BaseSampler pruner=None, # type: pruners.BasePruner + study_name=None # type: str ): # type: (...) -> None @@ -310,6 +311,7 @@ def create_study( storage=None, # type: Union[None, str, storages.BaseStorage] sampler=None, # type: samplers.BaseSampler pruner=None, # type: pruners.BasePruner + study_name=None, # type: Optional[str] ): # type: (...) -> Study @@ -323,6 +325,8 @@ def create_study( Sampler object that implements background algorithm for value suggestion. pruner: Pruner object that decides early stopping of unpromising trials. + study_name: + Name of study. Returns: A study object. @@ -330,7 +334,7 @@ def create_study( """ storage = storages.get_storage(storage) - study_uuid = storage.get_study_uuid_from_id(storage.create_new_study_id()) + study_uuid = storage.get_study_uuid_from_id(storage.create_new_study_id(study_name)) return Study(study_uuid=study_uuid, storage=storage, sampler=sampler, pruner=pruner) diff --git a/tests/storages_tests/rdb_tests/test_storage.py b/tests/storages_tests/rdb_tests/test_storage.py index a76ab5aa7b..4a82407902 100644 --- a/tests/storages_tests/rdb_tests/test_storage.py +++ b/tests/storages_tests/rdb_tests/test_storage.py @@ -150,6 +150,7 @@ def test_get_all_study_summaries_with_multiple_studies(): expected_summary_1 = StudySummary( study_id=study_id_1, study_uuid=storage.get_study_uuid_from_id(study_id_1), + study_name=storage.get_study_name_from_id(study_id_1), task=StudyTask.MINIMIZE, user_attrs={SYSTEM_ATTRS_KEY: {}}, best_trial=summaries[0].best_trial, # This always passes. @@ -159,6 +160,7 @@ def test_get_all_study_summaries_with_multiple_studies(): expected_summary_2 = StudySummary( study_id=study_id_2, study_uuid=storage.get_study_uuid_from_id(study_id_2), + study_name=storage.get_study_name_from_id(study_id_2), task=StudyTask.MAXIMIZE, user_attrs={SYSTEM_ATTRS_KEY: {}}, best_trial=summaries[1].best_trial, # This always passes. @@ -168,6 +170,7 @@ def test_get_all_study_summaries_with_multiple_studies(): expected_summary_3 = StudySummary( study_id=study_id_3, study_uuid=storage.get_study_uuid_from_id(study_id_3), + study_name=storage.get_study_name_from_id(study_id_3), task=StudyTask.NOT_SET, user_attrs={SYSTEM_ATTRS_KEY: {}}, best_trial=None, diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index e30f7f774f..666c2cd710 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -74,6 +74,19 @@ def test_create_new_study_id(storage_init_func): assert summaries[0].study_id == study_id +@parametrize_storage +def test_create_new_study_id_with_name(storage_init_func): + # type: (Callable[[], BaseStorage]) -> None + + study_name = 'sample_study_name' + storage = storage_init_func() + study_id = storage.create_new_study_id(study_name) + + summaries = storage.get_all_study_summaries() + assert len(summaries) == 1 + assert summaries[0].study_name == study_name + + @parametrize_storage def test_get_study_id_from_uuid_and_get_study_uuid_from_id(storage_init_func): # type: (Callable[[], BaseStorage]) -> None @@ -96,6 +109,32 @@ 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 +def test_get_study_id_from_name_and_get_study_name_from_id(storage_init_func): + # type: (Callable[[], BaseStorage]) -> None + + storage = storage_init_func() + + # Test not existing study. + with pytest.raises(ValueError): + storage.get_study_id_from_name('dummy-uuid') + + with pytest.raises(ValueError): + storage.get_study_name_from_id(-1) + + study_name = 'sample_study_name' + # Test existing study. + study_id = storage.create_new_study_id(study_name) + summary = storage.get_all_study_summaries()[0] + + assert study_id == summary.study_id + assert storage.get_study_name_from_id(summary.study_id) == summary.study_name + assert storage.get_study_id_from_name(summary.study_name) == summary.study_id + + with pytest.raises(ValueError): + storage.get_study_id_from_name(study_name=None) + + @parametrize_storage def test_set_and_get_study_task(storage_init_func): # type: (Callable[[], BaseStorage]) -> None From cbd26db1e5eda2f5d5d9751d38f0c24ba3942515 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Mon, 13 Aug 2018 15:40:31 +0900 Subject: [PATCH 02/21] Add --name option to create-study subcommand of CLI. --- pfnopt/cli.py | 7 ++++--- tests/test_cli.py | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pfnopt/cli.py b/pfnopt/cli.py index ac13272d4d..a011c4922e 100644 --- a/pfnopt/cli.py +++ b/pfnopt/cli.py @@ -44,6 +44,7 @@ def get_parser(self, prog_name): # type: (str) -> ArgumentParser parser = super(CreateStudy, self).get_parser(prog_name) + parser.add_argument('--name', default=None, help='Name of study.') return parser def take_action(self, parsed_args): @@ -51,7 +52,7 @@ def take_action(self, parsed_args): storage_url = get_storage_url(self.app_args.storage, self.app_args.config) storage = pfnopt.storages.RDBStorage(storage_url) - study_uuid = pfnopt.create_study(storage).study_uuid + study_uuid = pfnopt.create_study(storage, study_name=parsed_args.name).study_uuid print(study_uuid) @@ -79,7 +80,7 @@ def take_action(self, parsed_args): class Studies(Lister): _datetime_format = '%Y-%m-%d %H:%M:%S' - _study_list_header = ('UUID', 'TASK', 'N_TRIALS', 'DATETIME_START') + _study_list_header = ('UUID', 'NAME', 'TASK', 'N_TRIALS', 'DATETIME_START') def get_parser(self, prog_name): # type: (str) -> ArgumentParser @@ -97,7 +98,7 @@ def take_action(self, parsed_args): for s in summaries: start = s.datetime_start.strftime(self._datetime_format) \ if s.datetime_start is not None else None - row = (s.study_uuid, s.task.name, s.n_trials, start) + row = (s.study_uuid, s.study_name, s.task.name, s.n_trials, start) rows.append(row) return self._study_list_header, tuple(rows) diff --git a/tests/test_cli.py b/tests/test_cli.py index f1f57df886..dc3bafa39e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -138,12 +138,12 @@ def get_row_elements(row_index): # Check study_uuid and n_trials for the first study. elms = get_row_elements(3) assert elms[0] == study_uuid_1 - assert elms[2] == '0' + assert elms[3] == '0' # Check study_uuid and n_trials for the second study. elms = get_row_elements(4) assert elms[0] == study_uuid_2 - assert elms[2] == '10' + assert elms[3] == '10' @pytest.mark.parametrize('options', [['storage'], ['config'], ['storage', 'config']]) From 21de4e0fc637760a98aa1e8cae9660df9ed64c62 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Mon, 13 Aug 2018 15:44:16 +0900 Subject: [PATCH 03/21] Remove an unused variable. --- tests/storages_tests/test_storages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index 666c2cd710..9ff23a742a 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -80,7 +80,7 @@ def test_create_new_study_id_with_name(storage_init_func): study_name = 'sample_study_name' storage = storage_init_func() - study_id = storage.create_new_study_id(study_name) + storage.create_new_study_id(study_name) summaries = storage.get_all_study_summaries() assert len(summaries) == 1 From 89030d512c12a933674ff455afec9d351f7800bd Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 15 Aug 2018 14:41:14 +0900 Subject: [PATCH 04/21] Replace 'None' with a empty string when study_name is None. --- pfnopt/cli.py | 3 ++- tests/test_cli.py | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/pfnopt/cli.py b/pfnopt/cli.py index a011c4922e..763d50dba7 100644 --- a/pfnopt/cli.py +++ b/pfnopt/cli.py @@ -96,9 +96,10 @@ def take_action(self, parsed_args): rows = [] for s in summaries: + study_name = '' if s.study_name is None else s.study_name start = s.datetime_start.strftime(self._datetime_format) \ if s.datetime_start is not None else None - row = (s.study_uuid, s.study_name, s.task.name, s.n_trials, start) + row = (s.study_uuid, study_name, s.task.name, s.n_trials, start) rows.append(row) return self._study_list_header, tuple(rows) diff --git a/tests/test_cli.py b/tests/test_cli.py index dc3bafa39e..e0bfe2c595 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -116,7 +116,8 @@ def test_studies_command(options): study_uuid_1 = storage.get_study_uuid_from_id(storage.create_new_study_id()) # Second study. - study_uuid_2 = storage.get_study_uuid_from_id(storage.create_new_study_id()) + study_uuid_2 = storage.get_study_uuid_from_id( + storage.create_new_study_id(study_name='study_2')) pfnopt.minimize(objective_func, n_trials=10, storage=storage, study=study_uuid_2) # Run command. @@ -138,11 +139,13 @@ def get_row_elements(row_index): # Check study_uuid and n_trials for the first study. elms = get_row_elements(3) assert elms[0] == study_uuid_1 + assert len(elms[1]) == 0 assert elms[3] == '0' # Check study_uuid and n_trials for the second study. elms = get_row_elements(4) assert elms[0] == study_uuid_2 + assert elms[1] == 'study_2' assert elms[3] == '10' From dd4951ab6ada152097cb4475c74194ccef17b0bc Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 15 Aug 2018 14:58:55 +0900 Subject: [PATCH 05/21] Rmove an unused argument. Fix a comment. --- pfnopt/study.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pfnopt/study.py b/pfnopt/study.py index 448dc2d50d..692a137a05 100644 --- a/pfnopt/study.py +++ b/pfnopt/study.py @@ -52,7 +52,6 @@ def __init__( storage, # type: Union[None, str, storages.BaseStorage] sampler=None, # type: samplers.BaseSampler pruner=None, # type: pruners.BasePruner - study_name=None # type: str ): # type: (...) -> None @@ -326,7 +325,7 @@ def create_study( pruner: Pruner object that decides early stopping of unpromising trials. study_name: - Name of study. + A human-readable name of a study. Returns: A study object. From b927a20da5c35ca5f66c3f083fdb64ca2a602a9b Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 15 Aug 2018 15:00:05 +0900 Subject: [PATCH 06/21] Add a test case to check default value of study_name. --- tests/storages_tests/test_storages.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index 34c374cae4..26818954b6 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -72,6 +72,7 @@ def test_create_new_study_id(storage_init_func): summaries = storage.get_all_study_summaries() assert len(summaries) == 1 assert summaries[0].study_id == study_id + assert summaries[0].study_name is None @parametrize_storage From 751fd5ba8b95609ec8bbdcc60e1e261a3ffc76d2 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 15 Aug 2018 15:08:42 +0900 Subject: [PATCH 07/21] Fix an assertion of empty string. --- tests/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index e0bfe2c595..a677dcf5e8 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -139,7 +139,7 @@ def get_row_elements(row_index): # Check study_uuid and n_trials for the first study. elms = get_row_elements(3) assert elms[0] == study_uuid_1 - assert len(elms[1]) == 0 + assert elms[1] == '' assert elms[3] == '0' # Check study_uuid and n_trials for the second study. From f2f8ae7990eea0ee91554deed627fd18f0d8c8a3 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 15 Aug 2018 15:14:32 +0900 Subject: [PATCH 08/21] Update explanation of a CLI argument. --- pfnopt/cli.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pfnopt/cli.py b/pfnopt/cli.py index 763d50dba7..5aa537c1dc 100644 --- a/pfnopt/cli.py +++ b/pfnopt/cli.py @@ -44,7 +44,8 @@ def get_parser(self, prog_name): # type: (str) -> ArgumentParser parser = super(CreateStudy, self).get_parser(prog_name) - parser.add_argument('--name', default=None, help='Name of study.') + parser.add_argument('--name', default=None, + help='A human-readable name to distinguish a study from others.') return parser def take_action(self, parsed_args): From 00d679fc07e0ca1b8ec252bc54fda9ad85bb0a97 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 15 Aug 2018 15:58:40 +0900 Subject: [PATCH 09/21] Raise ValueError when study_name already exists. --- pfnopt/storages/rdb/storage.py | 4 +++- tests/storages_tests/rdb_tests/test_storage.py | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pfnopt/storages/rdb/storage.py b/pfnopt/storages/rdb/storage.py index 85c7a93251..fad1dd1e67 100644 --- a/pfnopt/storages/rdb/storage.py +++ b/pfnopt/storages/rdb/storage.py @@ -54,7 +54,9 @@ def create_new_study_id(self, study_name=None): study = models.StudyModel(study_uuid=study_uuid, study_name=study_name, task=structs.StudyTask.NOT_SET) session.add(study) - session.commit() + if not self._commit_or_rollback_on_integrity_error(session): + raise ValueError( + "study_name {} already exists. Please use a different name.".format(study_name)) # Set system attribute key and empty value. self.set_study_user_attr(study.study_id, SYSTEM_ATTRS_KEY, {}) diff --git a/tests/storages_tests/rdb_tests/test_storage.py b/tests/storages_tests/rdb_tests/test_storage.py index 4a82407902..14e6ebd42c 100644 --- a/tests/storages_tests/rdb_tests/test_storage.py +++ b/tests/storages_tests/rdb_tests/test_storage.py @@ -86,6 +86,16 @@ def test_create_new_study_id_duplicated_uuid(): assert mock_object.call_count == 3 +def test_create_new_study_id_duplicated_name(): + # type: () -> None + + storage = create_test_storage() + study_name = 'sample_study_name' + storage.create_new_study_id(study_name) + with pytest.raises(ValueError): + storage.create_new_study_id(study_name) + + def test_set_trial_param_to_check_distribution_json(): # type: () -> None From 8787c6f9961671f10b36d1561aabc316bc37aada Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Fri, 17 Aug 2018 10:59:05 +0900 Subject: [PATCH 10/21] Change type of StudyModel.study_name from Optional[str] to str. --- pfnopt/storages/base.py | 6 +++--- pfnopt/storages/in_memory.py | 11 +++++++---- pfnopt/storages/rdb/models.py | 2 +- pfnopt/storages/rdb/storage.py | 11 ++++++----- pfnopt/structs.py | 2 +- tests/storages_tests/test_storages.py | 6 ++---- tests/test_cli.py | 3 ++- 7 files changed, 22 insertions(+), 19 deletions(-) diff --git a/pfnopt/storages/base.py b/pfnopt/storages/base.py index bbc7347fb3..cf86dc2e78 100644 --- a/pfnopt/storages/base.py +++ b/pfnopt/storages/base.py @@ -12,6 +12,7 @@ from pfnopt import structs # NOQA SYSTEM_ATTRS_KEY = '__system__' +DEFAULT_STUDY_NAME_PREFIX = 'no-name-' @six.add_metaclass(abc.ABCMeta) @@ -62,14 +63,13 @@ def get_study_uuid_from_id(self, study_id): @abc.abstractmethod def get_study_id_from_name(self, study_name): - # type: (Optional[str]) -> int + # type: (str) -> int - # study_name can be None when it is specified by StudySummary.study_name. raise NotImplementedError @abc.abstractmethod def get_study_name_from_id(self, study_id): - # type: (int) -> Optional[str] + # type: (int) -> str raise NotImplementedError diff --git a/pfnopt/storages/in_memory.py b/pfnopt/storages/in_memory.py index c5846df818..dff42b7ef0 100644 --- a/pfnopt/storages/in_memory.py +++ b/pfnopt/storages/in_memory.py @@ -8,6 +8,7 @@ from pfnopt import distributions # NOQA from pfnopt.storages import base +from pfnopt.storages.base import DEFAULT_STUDY_NAME_PREFIX from pfnopt import structs @@ -23,7 +24,7 @@ def __init__(self): self.param_distribution = {} # type: Dict[str, distributions.BaseDistribution] self.task = structs.StudyTask.NOT_SET self.study_user_attrs = {} # type: Dict[str, Any] - self.study_name = None # type: Optional[str] + self.study_name = DEFAULT_STUDY_NAME_PREFIX + IN_MEMORY_STORAGE_STUDY_UUID # type: str self._lock = threading.Lock() @@ -41,7 +42,9 @@ def __setstate__(self, state): def create_new_study_id(self, study_name=None): # type: (Optional[str]) -> int - self.study_name = study_name + if study_name is not None: + self.study_name = study_name + self.study_user_attrs[base.SYSTEM_ATTRS_KEY] = {} return IN_MEMORY_STORAGE_STUDY_ID # TODO(akiba) @@ -74,7 +77,7 @@ def get_study_uuid_from_id(self, study_id): return IN_MEMORY_STORAGE_STUDY_UUID def get_study_id_from_name(self, study_name): - # type: (Optional[str]) -> int + # type: (str) -> int if study_name is None: raise ValueError("study_name is supposed to be str, not None.") @@ -84,7 +87,7 @@ def get_study_id_from_name(self, study_name): return IN_MEMORY_STORAGE_STUDY_ID def get_study_name_from_id(self, study_id): - # type: (int) -> Optional[str] + # type: (int) -> str self._check_study_id(study_id) return self.study_name diff --git a/pfnopt/storages/rdb/models.py b/pfnopt/storages/rdb/models.py index 2aa7311742..d314352468 100644 --- a/pfnopt/storages/rdb/models.py +++ b/pfnopt/storages/rdb/models.py @@ -29,7 +29,7 @@ class StudyModel(BaseModel): __tablename__ = 'studies' study_id = Column(Integer, primary_key=True) study_uuid = Column(String(255), unique=True) - study_name = Column(String(255), unique=True) + study_name = Column(String(255), unique=True, nullable=False) task = Column(Enum(StudyTask), nullable=False) @classmethod diff --git a/pfnopt/storages/rdb/storage.py b/pfnopt/storages/rdb/storage.py index fad1dd1e67..61fc61c565 100644 --- a/pfnopt/storages/rdb/storage.py +++ b/pfnopt/storages/rdb/storage.py @@ -13,6 +13,7 @@ from pfnopt import distributions from pfnopt import logging from pfnopt.storages.base import BaseStorage +from pfnopt.storages.base import DEFAULT_STUDY_NAME_PREFIX from pfnopt.storages.base import SYSTEM_ATTRS_KEY from pfnopt.storages.rdb import models from pfnopt import structs @@ -51,6 +52,9 @@ def create_new_study_id(self, study_name=None): if study is None: break + if study_name is None: + study_name = DEFAULT_STUDY_NAME_PREFIX + study_uuid + study = models.StudyModel(study_uuid=study_uuid, study_name=study_name, task=structs.StudyTask.NOT_SET) session.add(study) @@ -116,10 +120,7 @@ def get_study_uuid_from_id(self, study_id): return study.study_uuid def get_study_id_from_name(self, study_name): - # type: (Optional[str]) -> int - - if study_name is None: - raise ValueError("study_name is supposed to be str, not None.") + # type: (str) -> int session = self.scoped_session() @@ -128,7 +129,7 @@ def get_study_id_from_name(self, study_name): return study.study_id def get_study_name_from_id(self, study_id): - # type: (int) -> Optional[str] + # type: (int) -> str session = self.scoped_session() diff --git a/pfnopt/structs.py b/pfnopt/structs.py index f4bbf626ed..310d86bc43 100644 --- a/pfnopt/structs.py +++ b/pfnopt/structs.py @@ -47,7 +47,7 @@ class FrozenTrial( 'StudySummary', [('study_id', int), ('study_uuid', str), - ('study_name', Optional[str]), + ('study_name', str), ('task', StudyTask), ('best_trial', Optional[FrozenTrial]), ('user_attrs', Dict[str, Any]), diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index 26818954b6..712eded9ee 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -9,6 +9,7 @@ from pfnopt.distributions import CategoricalDistribution from pfnopt.distributions import LogUniformDistribution from pfnopt.distributions import UniformDistribution +from pfnopt.storages.base import DEFAULT_STUDY_NAME_PREFIX from pfnopt.storages.base import SYSTEM_ATTRS_KEY from pfnopt.storages import BaseStorage # NOQA from pfnopt.storages import InMemoryStorage @@ -72,7 +73,7 @@ def test_create_new_study_id(storage_init_func): summaries = storage.get_all_study_summaries() assert len(summaries) == 1 assert summaries[0].study_id == study_id - assert summaries[0].study_name is None + assert summaries[0].study_name.startswith(DEFAULT_STUDY_NAME_PREFIX) @parametrize_storage @@ -132,9 +133,6 @@ def test_get_study_id_from_name_and_get_study_name_from_id(storage_init_func): assert storage.get_study_name_from_id(summary.study_id) == summary.study_name assert storage.get_study_id_from_name(summary.study_name) == summary.study_id - with pytest.raises(ValueError): - storage.get_study_id_from_name(study_name=None) - @parametrize_storage def test_set_and_get_study_task(storage_init_func): diff --git a/tests/test_cli.py b/tests/test_cli.py index a677dcf5e8..884017c55a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -12,6 +12,7 @@ import pfnopt from pfnopt.cli import Studies +from pfnopt.storages.base import DEFAULT_STUDY_NAME_PREFIX from pfnopt.storages import RDBStorage from pfnopt.trial import Trial # NOQA @@ -139,7 +140,7 @@ def get_row_elements(row_index): # Check study_uuid and n_trials for the first study. elms = get_row_elements(3) assert elms[0] == study_uuid_1 - assert elms[1] == '' + assert elms[1].startswith(DEFAULT_STUDY_NAME_PREFIX) assert elms[3] == '0' # Check study_uuid and n_trials for the second study. From bc2e58a847637c014af7695e229ac25088453c1e Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Fri, 17 Aug 2018 11:21:40 +0900 Subject: [PATCH 11/21] Add --study_name option to pfnopt minimize command. --- pfnopt/cli.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pfnopt/cli.py b/pfnopt/cli.py index 5aa537c1dc..1327d5b6e5 100644 --- a/pfnopt/cli.py +++ b/pfnopt/cli.py @@ -44,8 +44,8 @@ def get_parser(self, prog_name): # type: (str) -> ArgumentParser parser = super(CreateStudy, self).get_parser(prog_name) - parser.add_argument('--name', default=None, - help='A human-readable name to distinguish a study from others.') + parser.add_argument('--study_name', default=None, + help='A human-readable name of a study to distinguish it from others.') return parser def take_action(self, parsed_args): @@ -53,7 +53,7 @@ def take_action(self, parsed_args): storage_url = get_storage_url(self.app_args.storage, self.app_args.config) storage = pfnopt.storages.RDBStorage(storage_url) - study_uuid = pfnopt.create_study(storage, study_name=parsed_args.name).study_uuid + study_uuid = pfnopt.create_study(storage, study_name=parsed_args.study_name).study_uuid print(study_uuid) @@ -148,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, + help='A human-readable name of a study to distinguish it from others.') parser.add_argument('file', help='Python script file where the objective function resides.') parser.add_argument('method', help='The method name of the objective function.') @@ -165,7 +167,7 @@ def take_action(self, parsed_args): storage_url = get_storage_url(self.app_args.storage, self.app_args.config) if parsed_args.create_study: - study = pfnopt.create_study(storage=storage_url) + study = pfnopt.create_study(storage=storage_url, study_name=parsed_args.study_name) else: study = pfnopt.Study(storage=storage_url, study_uuid=parsed_args.study) From b11925f901cdde6753027f7b0183f274e7a7da15 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Fri, 17 Aug 2018 15:57:18 +0900 Subject: [PATCH 12/21] Add tests of study_name to test_cli.py. --- tests/test_cli.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/test_cli.py b/tests/test_cli.py index 884017c55a..49d252108d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -80,6 +80,25 @@ def test_create_study_command(options): study_id = storage.get_study_id_from_uuid(study_uuid) assert study_id == 2 + # Check if a default value of study_name is stored in the storage. + assert storage.get_study_name_from_id(study_id).startswith(DEFAULT_STUDY_NAME_PREFIX) + + +def test_create_study_command_with_study_name(): + # type: () -> None + + with StorageConfigSupplier(TEST_CONFIG_TEMPLATE) as (storage_url, config_path): + storage = RDBStorage(storage_url) + study_name = 'test_study' + + # Create study with name. + command = ['pfnopt', 'create-study', '--storage', storage_url, '--study_name', study_name] + study_uuid = str(subprocess.check_output(command).decode().strip()) + + # Check if study_name is stored in the storage. + study_id = storage.get_study_id_from_uuid(study_uuid) + assert storage.get_study_name_from_id(study_id) == study_name + @pytest.mark.parametrize('options', [['storage'], ['config'], ['storage', 'config']]) def test_study_set_user_attr_command(options): @@ -203,6 +222,28 @@ def test_minimize_command(options): assert len(study.trials) == 10 assert 'x' in study.best_params + # Check if a default value of study_name is stored in the storage. + assert storage.get_study_name_from_id(study.study_id).startswith(DEFAULT_STUDY_NAME_PREFIX) + + +def test_minimize_command_with_study_name(): + # type: () -> None + + with StorageConfigSupplier(TEST_CONFIG_TEMPLATE) as (storage_url, config_path): + storage = RDBStorage(storage_url) + study_name = 'test_study' + + # Run minimize with study_name. + command = ['pfnopt', 'minimize', '--n-trials', '10', '--create-study', + __file__, 'objective_func', '--storage', storage_url, + '--study_name', study_name] + subprocess.check_call(command) + + # Check if study_name is stored in the storage. + studies = storage.get_all_study_summaries() + assert len(studies) == 1 + assert studies[0].study_name == study_name + def test_minimize_command_inconsistent_args(): # type: () -> None From cf1d89d9d66cf1b2292fa04c17c347410443e0f7 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 22 Aug 2018 14:42:59 +0900 Subject: [PATCH 13/21] Remove inappropriate None-check. --- pfnopt/storages/in_memory.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/pfnopt/storages/in_memory.py b/pfnopt/storages/in_memory.py index dff42b7ef0..65e68c0e6a 100644 --- a/pfnopt/storages/in_memory.py +++ b/pfnopt/storages/in_memory.py @@ -79,8 +79,6 @@ def get_study_uuid_from_id(self, study_id): def get_study_id_from_name(self, study_name): # type: (str) -> int - if study_name is None: - raise ValueError("study_name is supposed to be str, not None.") if study_name != self.study_name: raise ValueError("No such study {}.".format(study_name)) From 55441c4e97fb21abc0cd3cc34e1304c1f2dd1675 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 22 Aug 2018 14:44:37 +0900 Subject: [PATCH 14/21] Replace --study_name with --study-name to keep consistency of options. --- pfnopt/cli.py | 4 ++-- tests/test_cli.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pfnopt/cli.py b/pfnopt/cli.py index 1327d5b6e5..713bdeea8a 100644 --- a/pfnopt/cli.py +++ b/pfnopt/cli.py @@ -44,7 +44,7 @@ def get_parser(self, prog_name): # type: (str) -> ArgumentParser parser = super(CreateStudy, self).get_parser(prog_name) - parser.add_argument('--study_name', default=None, + parser.add_argument('--study-name', default=None, help='A human-readable name of a study to distinguish it from others.') return parser @@ -148,7 +148,7 @@ 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, + parser.add_argument('--study-name', default=None, help='A human-readable name of a study to distinguish it from others.') parser.add_argument('file', help='Python script file where the objective function resides.') diff --git a/tests/test_cli.py b/tests/test_cli.py index 49d252108d..9cb01b770d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -92,7 +92,7 @@ def test_create_study_command_with_study_name(): study_name = 'test_study' # Create study with name. - command = ['pfnopt', 'create-study', '--storage', storage_url, '--study_name', study_name] + command = ['pfnopt', 'create-study', '--storage', storage_url, '--study-name', study_name] study_uuid = str(subprocess.check_output(command).decode().strip()) # Check if study_name is stored in the storage. @@ -236,7 +236,7 @@ def test_minimize_command_with_study_name(): # Run minimize with study_name. command = ['pfnopt', 'minimize', '--n-trials', '10', '--create-study', __file__, 'objective_func', '--storage', storage_url, - '--study_name', study_name] + '--study-name', study_name] subprocess.check_call(command) # Check if study_name is stored in the storage. From 2eca0af3d781ec00cadbbda624218b2a8c508fa8 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 22 Aug 2018 14:46:05 +0900 Subject: [PATCH 15/21] Use StorageSupplier to provide storages for unit tests. --- tests/storages_tests/test_storages.py | 50 ++++++++++++++++++--------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index 712eded9ee..64098c2ecd 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -5,6 +5,7 @@ from typing import Dict # NOQA from typing import Optional # NOQA +import pfnopt from pfnopt.distributions import BaseDistribution # NOQA from pfnopt.distributions import CategoricalDistribution from pfnopt.distributions import LogUniformDistribution @@ -17,6 +18,7 @@ from pfnopt.structs import FrozenTrial from pfnopt.structs import StudyTask from pfnopt.structs import TrialState +from pfnopt.testing.storage import StorageSupplier EXAMPLE_SYSTEM_ATTRS = { 'dataset': 'MNIST', @@ -58,11 +60,28 @@ ) ] +STORAGE_MODES = [ + 'none', # We give `None` to storage argument, so InMemoryStorage is used. + 'new', # We always create a new sqlite DB file for each experiment. + 'common', # We use a sqlite DB file for the whole experiments. +] parametrize_storage = pytest.mark.parametrize( 'storage_init_func', [InMemoryStorage, lambda: RDBStorage('sqlite:///:memory:')]) +def setup_module(): + # type: () -> None + + StorageSupplier.setup_common_tempfile() + + +def teardown_module(): + # type: () -> None + + StorageSupplier.teardown_common_tempfile() + + @parametrize_storage def test_create_new_study_id(storage_init_func): # type: (Callable[[], BaseStorage]) -> None @@ -111,27 +130,26 @@ 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 -def test_get_study_id_from_name_and_get_study_name_from_id(storage_init_func): - # type: (Callable[[], BaseStorage]) -> None +@pytest.mark.parametrize('storage_mode', STORAGE_MODES) +def test_get_study_id_from_name_and_get_study_name_from_id(storage_mode): + # type: (str) -> None - storage = storage_init_func() + with StorageSupplier(storage_mode) as storage: - # Test not existing study. - with pytest.raises(ValueError): - storage.get_study_id_from_name('dummy-uuid') + # Add storage_mode to avoid duplication of study_name. + study_name = 'sample_study_name_for_' + storage_mode + study = pfnopt.create_study(storage=storage, study_name=study_name) - with pytest.raises(ValueError): - storage.get_study_name_from_id(-1) + # Test existing study. + assert study.storage.get_study_name_from_id(study.study_id) == study_name + assert study.storage.get_study_id_from_name(study_name) == study.study_id - study_name = 'sample_study_name' - # Test existing study. - study_id = storage.create_new_study_id(study_name) - summary = storage.get_all_study_summaries()[0] + # Test not existing study. + with pytest.raises(ValueError): + study.storage.get_study_id_from_name('dummy-name') - assert study_id == summary.study_id - assert storage.get_study_name_from_id(summary.study_id) == summary.study_name - assert storage.get_study_id_from_name(summary.study_name) == summary.study_id + with pytest.raises(ValueError): + study.storage.get_study_name_from_id(-1) @parametrize_storage From 5163fc5905b316e7ad470becd87d9012376b5f1e Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 22 Aug 2018 16:04:06 +0900 Subject: [PATCH 16/21] Add a TODO comment to improve code quality. --- tests/storages_tests/test_storages.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index 64098c2ecd..808cf33854 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -66,6 +66,7 @@ 'common', # We use a sqlite DB file for the whole experiments. ] +# TODO(Yanase): Use Replace StorageSupplier instead of @parametrize_storage. parametrize_storage = pytest.mark.parametrize( 'storage_init_func', [InMemoryStorage, lambda: RDBStorage('sqlite:///:memory:')]) From 82614b84da674cf523b4466bcb359ddfd84c6268 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Thu, 23 Aug 2018 16:30:31 +0900 Subject: [PATCH 17/21] Remove redundant none checking. --- pfnopt/cli.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pfnopt/cli.py b/pfnopt/cli.py index 713bdeea8a..a19ada020c 100644 --- a/pfnopt/cli.py +++ b/pfnopt/cli.py @@ -97,10 +97,9 @@ def take_action(self, parsed_args): rows = [] for s in summaries: - study_name = '' if s.study_name is None else s.study_name start = s.datetime_start.strftime(self._datetime_format) \ if s.datetime_start is not None else None - row = (s.study_uuid, study_name, s.task.name, s.n_trials, start) + row = (s.study_uuid, s.study_name, s.task.name, s.n_trials, start) rows.append(row) return self._study_list_header, tuple(rows) From dab306150cbb847b3824452376f6ab0395f9cd09 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Thu, 23 Aug 2018 16:31:25 +0900 Subject: [PATCH 18/21] Use StorageSupplier in test_create_new_study_id_with_name. --- tests/storages_tests/test_storages.py | 34 +++++++++++++++------------ 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index 808cf33854..1c80c6eb71 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -96,17 +96,19 @@ def test_create_new_study_id(storage_init_func): assert summaries[0].study_name.startswith(DEFAULT_STUDY_NAME_PREFIX) -@parametrize_storage -def test_create_new_study_id_with_name(storage_init_func): - # type: (Callable[[], BaseStorage]) -> None +@pytest.mark.parametrize('storage_mode', STORAGE_MODES) +def test_create_new_study_id_with_name(storage_mode): + # type: (str) -> None - study_name = 'sample_study_name' - storage = storage_init_func() - storage.create_new_study_id(study_name) + with StorageSupplier(storage_mode) as storage: - summaries = storage.get_all_study_summaries() - assert len(summaries) == 1 - assert summaries[0].study_name == study_name + # Generate unique study_name from the current function name and storage_mode. + function_name = test_create_new_study_id_with_name.__name__ + study_name = function_name + '/' + storage_mode + storage = pfnopt.storages.get_storage(storage) + study_id = storage.create_new_study_id(study_name) + + assert study_name == storage.get_study_name_from_id(study_id) @parametrize_storage @@ -137,20 +139,22 @@ def test_get_study_id_from_name_and_get_study_name_from_id(storage_mode): with StorageSupplier(storage_mode) as storage: - # Add storage_mode to avoid duplication of study_name. - study_name = 'sample_study_name_for_' + storage_mode + # Generate unique study_name from the current function name and storage_mode. + function_name = test_get_study_id_from_name_and_get_study_name_from_id.__name__ + study_name = function_name + '/' + storage_mode + storage = pfnopt.storages.get_storage(storage) study = pfnopt.create_study(storage=storage, study_name=study_name) # Test existing study. - assert study.storage.get_study_name_from_id(study.study_id) == study_name - assert study.storage.get_study_id_from_name(study_name) == study.study_id + assert storage.get_study_name_from_id(study.study_id) == study_name + assert storage.get_study_id_from_name(study_name) == study.study_id # Test not existing study. with pytest.raises(ValueError): - study.storage.get_study_id_from_name('dummy-name') + storage.get_study_id_from_name('dummy-name') with pytest.raises(ValueError): - study.storage.get_study_name_from_id(-1) + storage.get_study_name_from_id(-1) @parametrize_storage From 6037645b1065dcaecd08097367f15bc062134971 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Tue, 11 Sep 2018 09:41:17 +0900 Subject: [PATCH 19/21] Fix a comment. --- tests/storages_tests/test_storages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storages_tests/test_storages.py b/tests/storages_tests/test_storages.py index e997820039..76ec383faa 100644 --- a/tests/storages_tests/test_storages.py +++ b/tests/storages_tests/test_storages.py @@ -67,7 +67,7 @@ 'common', # We use a sqlite DB file for the whole experiments. ] -# TODO(Yanase): Use Replace StorageSupplier instead of @parametrize_storage. +# TODO(Yanase): Replace @parametrize_storage with StorageSupplier. parametrize_storage = pytest.mark.parametrize( 'storage_init_func', [InMemoryStorage, lambda: RDBStorage('sqlite:///:memory:')]) From 11c897578bfb61708c36a56656c210aec68b8767 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Wed, 12 Sep 2018 11:16:00 +0900 Subject: [PATCH 20/21] fixup! Merge branch 'master' into add-study-name. --- pfnopt/storages/rdb/storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pfnopt/storages/rdb/storage.py b/pfnopt/storages/rdb/storage.py index 046519b6d0..df8e6bd5be 100644 --- a/pfnopt/storages/rdb/storage.py +++ b/pfnopt/storages/rdb/storage.py @@ -61,7 +61,7 @@ def create_new_study_id(self, study_name=None): study = models.StudyModel(study_uuid=study_uuid, study_name=study_name, task=structs.StudyTask.NOT_SET) session.add(study) - if not self._commit_or_rollback_on_integrity_error(session): + if not self._commit_with_integrity_check(session): raise ValueError( "study_name {} already exists. Please use a different name.".format(study_name)) From 728f45dc7bd9866fc84c9b26618bd1cc33277895 Mon Sep 17 00:00:00 2001 From: Toshihiko Yanase Date: Thu, 13 Sep 2018 17:17:17 +0900 Subject: [PATCH 21/21] Remove --study-name argument from 'pfnopt minimize'. --- pfnopt/cli.py | 4 +--- tests/test_cli.py | 19 ------------------- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/pfnopt/cli.py b/pfnopt/cli.py index 8fa68ea802..3afc1af500 100644 --- a/pfnopt/cli.py +++ b/pfnopt/cli.py @@ -154,8 +154,6 @@ 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, - help='A human-readable name of a study to distinguish it from others.') parser.add_argument('file', help='Python script file where the objective function resides.') parser.add_argument('method', help='The method name of the objective function.') @@ -174,7 +172,7 @@ def take_action(self, parsed_args): config = pfnopt.config.load_pfnopt_config(self.app_args.config) storage_url = get_storage_url(self.app_args.storage, config) if parsed_args.create_study: - study = pfnopt.create_study(storage=storage_url, study_name=parsed_args.study_name) + study = pfnopt.create_study(storage=storage_url) else: study = pfnopt.Study(storage=storage_url, study_uuid=parsed_args.study) diff --git a/tests/test_cli.py b/tests/test_cli.py index 5b6e0b6719..b648e171b8 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -245,25 +245,6 @@ def test_minimize_command(options): assert storage.get_study_name_from_id(study.study_id).startswith(DEFAULT_STUDY_NAME_PREFIX) -def test_minimize_command_with_study_name(): - # type: () -> None - - with StorageConfigSupplier(TEST_CONFIG_TEMPLATE) as (storage_url, config_path): - storage = RDBStorage(storage_url) - study_name = 'test_study' - - # Run minimize with study_name. - command = ['pfnopt', 'minimize', '--n-trials', '10', '--create-study', - __file__, 'objective_func', '--storage', storage_url, - '--study-name', study_name] - subprocess.check_call(command) - - # Check if study_name is stored in the storage. - studies = storage.get_all_study_summaries() - assert len(studies) == 1 - assert studies[0].study_name == study_name - - def test_minimize_command_inconsistent_args(): # type: () -> None