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 Study.study_name #157

Merged
merged 25 commits into from
Sep 14, 2018
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8c65be2
Add study_name attribute to Study.
toshihikoyanase Aug 13, 2018
cbd26db
Add --name option to create-study subcommand of CLI.
toshihikoyanase Aug 13, 2018
21de4e0
Remove an unused variable.
toshihikoyanase Aug 13, 2018
45aa758
Merge branch 'master' into add-study-name
toshihikoyanase Aug 15, 2018
89030d5
Replace 'None' with a empty string when study_name is None.
toshihikoyanase Aug 15, 2018
dd4951a
Rmove an unused argument. Fix a comment.
toshihikoyanase Aug 15, 2018
b927a20
Add a test case to check default value of study_name.
toshihikoyanase Aug 15, 2018
751fd5b
Fix an assertion of empty string.
toshihikoyanase Aug 15, 2018
f2f8ae7
Update explanation of a CLI argument.
toshihikoyanase Aug 15, 2018
00d679f
Raise ValueError when study_name already exists.
toshihikoyanase Aug 15, 2018
8787c6f
Change type of StudyModel.study_name from Optional[str] to str.
toshihikoyanase Aug 17, 2018
bc2e58a
Add --study_name option to pfnopt minimize command.
toshihikoyanase Aug 17, 2018
b11925f
Add tests of study_name to test_cli.py.
toshihikoyanase Aug 17, 2018
cf1d89d
Remove inappropriate None-check.
toshihikoyanase Aug 22, 2018
55441c4
Replace --study_name with --study-name to keep consistency of options.
toshihikoyanase Aug 22, 2018
2eca0af
Use StorageSupplier to provide storages for unit tests.
toshihikoyanase Aug 22, 2018
5163fc5
Add a TODO comment to improve code quality.
toshihikoyanase Aug 22, 2018
82614b8
Remove redundant none checking.
toshihikoyanase Aug 23, 2018
dab3061
Use StorageSupplier in test_create_new_study_id_with_name.
toshihikoyanase Aug 23, 2018
eb1bd2f
Merge master to add-study-name.
toshihikoyanase Sep 7, 2018
6037645
Fix a comment.
toshihikoyanase Sep 11, 2018
dfb05b3
Merge branch 'master' into add-study-name.
toshihikoyanase Sep 12, 2018
11c8975
fixup! Merge branch 'master' into add-study-name.
toshihikoyanase Sep 12, 2018
728f45d
Remove --study-name argument from 'pfnopt minimize'.
toshihikoyanase Sep 13, 2018
d0202c3
Merge branch 'master' into add-study-name.
toshihikoyanase Sep 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 9 additions & 4 deletions pfnopt/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

help='A human-readable name of a study to distinguish it from others.')
return parser

def take_action(self, parsed_args):
# type: (Namespace) -> None

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.study_name).study_uuid
print(study_uuid)


Expand Down Expand Up @@ -79,7 +81,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
Expand All @@ -95,9 +97,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.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)
Expand Down Expand Up @@ -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.

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.')
Expand All @@ -162,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)

Expand Down
17 changes: 15 additions & 2 deletions pfnopt/storages/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from pfnopt import structs # NOQA

SYSTEM_ATTRS_KEY = '__system__'
DEFAULT_STUDY_NAME_PREFIX = 'no-name-'


@six.add_metaclass(abc.ABCMeta)
Expand All @@ -20,8 +21,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

Expand Down Expand Up @@ -60,6 +61,18 @@ def get_study_uuid_from_id(self, study_id):

raise NotImplementedError

@abc.abstractmethod
def get_study_id_from_name(self, study_name):
# type: (str) -> int

raise NotImplementedError

@abc.abstractmethod
def get_study_name_from_id(self, study_id):
# type: (int) -> str

raise NotImplementedError

@ abc.abstractmethod
def get_study_task(self, study_id):
# type: (int) -> structs.StudyTask
Expand Down
26 changes: 24 additions & 2 deletions pfnopt/storages/in_memory.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -23,6 +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 = DEFAULT_STUDY_NAME_PREFIX + IN_MEMORY_STORAGE_STUDY_UUID # type: str

self._lock = threading.Lock()

Expand All @@ -37,8 +39,11 @@ 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

if study_name is not None:
self.study_name = study_name

self.study_user_attrs[base.SYSTEM_ATTRS_KEY] = {}

Expand Down Expand Up @@ -71,6 +76,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: (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.

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) -> str

self._check_study_id(study_id)
return self.study_name

def get_study_task(self, study_id):
# type: (int) -> structs.StudyTask

Expand All @@ -97,6 +118,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),
Expand Down
21 changes: 20 additions & 1 deletion pfnopt/storages/rdb/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.'

Expand All @@ -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, nullable=False)
task = Column(Enum(StudyTask), nullable=False)

@classmethod
Expand Down Expand Up @@ -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]
Expand Down
34 changes: 30 additions & 4 deletions pfnopt/storages/rdb/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -40,8 +41,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()

Expand All @@ -51,9 +52,15 @@ def create_new_study_id(self):
if study is None:
break

study = models.StudyModel(study_uuid=study_uuid, task=structs.StudyTask.NOT_SET)
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)
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, {})
Expand Down Expand Up @@ -112,6 +119,24 @@ def get_study_uuid_from_id(self, study_id):

return study.study_uuid

def get_study_id_from_name(self, study_name):
# type: (str) -> int

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) -> 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

Expand Down Expand Up @@ -172,6 +197,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),
Expand Down
1 change: 1 addition & 0 deletions pfnopt/structs.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class FrozenTrial(
'StudySummary',
[('study_id', int),
('study_uuid', str),
('study_name', str),
('task', StudyTask),
('best_trial', Optional[FrozenTrial]),
('user_attrs', Dict[str, Any]),
Expand Down
5 changes: 4 additions & 1 deletion pfnopt/study.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,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

Expand All @@ -323,14 +324,16 @@ def create_study(
Sampler object that implements background algorithm for value suggestion.
pruner:
Pruner object that decides early stopping of unpromising trials.
study_name:
A human-readable name of a study.

Returns:
A study object.

"""

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)


Expand Down
13 changes: 13 additions & 0 deletions tests/storages_tests/rdb_tests/test_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -150,6 +160,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.
Expand All @@ -159,6 +170,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.
Expand All @@ -168,6 +180,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,
Expand Down