-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Remove OptunaConfig
.
#653
Remove OptunaConfig
.
#653
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tests/test_cli.py
Outdated
class StorageConfigSupplier(object): | ||
def __init__(self, config_template): | ||
# type: (str) -> None | ||
class StorageSupplier(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we no longer need tempfile_config
, can't we delete StorageSupplier
in this file and use optuna.testing.storage.StorageSupplier
instead?
For example, we could modify test_create_study_command
as follows:
def test_create_study_command():
# type: () -> None
with StorageSupplier('new') as storage:
storage_url = str(storage.engine.url)
# Create study.
command = ['optuna', 'create-study', '--storage', storage_url]
subprocess.check_call(command)
# Command output should be in name string format (no-name + UUID).
study_name = str(subprocess.check_output(command).decode().strip())
name_re = r'^no-name-[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$'
assert re.match(name_re, study_name) is not None
# study_name should be stored in storage.
study_id = storage.get_study_id_from_name(study_name)
assert study_id == 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing it out. I fixed it based on your example code in this commit 5b13c76.
def test_create_study_command():
# type: () -> None
with StorageSupplier('new') as storage:
+ assert isinstance(storage, RDBStorage)
storage_url = str(storage.engine.url)
I needed to add an assersion that confirms storage
is an instance of RDBStorage
because optuna.testing.storages.StorageSupplier
can returns None
or InMemoryStorage
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Optuna's config file is used to specify default storage in
$HOME/.optuna.yml
. This PR removes it because of the following reasons:optuna
commands.$HOME/.optuna.yml
is ignored if users execute the code by Python interpreter.I'd like to remove the current configuration method and prepare for the discussion to improve Optuna's configuration.