From c7639b3f3f185b1b0ff7ceaddf06c2ab86f10bdb Mon Sep 17 00:00:00 2001 From: Contramundum Date: Fri, 1 Sep 2023 14:54:59 +0900 Subject: [PATCH 1/5] Change API --- optuna_dashboard/preferential/_study.py | 31 +++++++------------------ 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/optuna_dashboard/preferential/_study.py b/optuna_dashboard/preferential/_study.py index 16c81b66..a7f494d4 100644 --- a/optuna_dashboard/preferential/_study.py +++ b/optuna_dashboard/preferential/_study.py @@ -255,26 +255,6 @@ class PreferentialStudy: raise RuntimeError("Unexpected trial type") storage.set_trial_system_attr(trial_id, _SYSTEM_ATTR_COMPARISON_READY, True) - @property - def n_generate(self) -> int: - """Return the number of trials that should be generated and shown to user. - - :func:`~optuna_dashboard.preferential.PreferentialStudy.should_generate` returns - :obj:`True` if the number of trials not reported bad and not skipped are less than - :attr:`~optuna_dashboard.preferential.PreferentialStudy.n_generate`. - """ - system_attrs = self._study._storage.get_study_system_attrs(self._study._study_id) - return get_n_generate(system_attrs) - - def set_n_generate(self, n_generate: int) -> None: - """Set the number of trials that should be generated and shown to user. - - :func:`~optuna_dashboard.preferential.PreferentialStudy.should_generate` returns - :obj:`True` if the number of trials not reported bad and not skipped are less than - :attr:`~optuna_dashboard.preferential.PreferentialStudy.n_generate`. - """ - return set_n_generate(self._study._study_id, self._study._storage, n_generate) - def should_generate(self) -> bool: """Return whether the generator should generate a new trial now. @@ -283,7 +263,7 @@ class PreferentialStudy: to generate a new trial if this method returns :obj:`True`, and to wait for human evaluation if this method returns :obj:`False`. """ - return len(self.best_trials) < self.n_generate + return len(self.best_trials) < get_n_generate(self._study._study_id, self._study._storage) def get_best_trials(study_id: int, storage: optuna.storages.BaseStorage) -> list[FrozenTrial]: @@ -306,6 +286,7 @@ def get_best_trials(study_id: int, storage: optuna.storages.BaseStorage) -> list def create_study( *, + n_generate: int, storage: str | optuna.storages.BaseStorage | None = None, sampler: BaseSampler | None = None, study_name: str | None = None, @@ -325,6 +306,12 @@ def create_study( trial = study.ask() Args: + n_generate: + The number of active trials to keep. + :func:`~optuna_dashboard.preferential.PreferentialStudy.should_generate` returns + :obj:`True` if the number of trials not reported bad and not skipped are less than + ``n_generate``. + storage: Database URL. If this argument is set to None, in-memory storage is used, and the :class:`~optuna_dashboard.preferential.PreferentialStudy` will not be persistent. @@ -360,7 +347,7 @@ def create_study( study._storage.set_study_system_attr( study._study_id, _SYSTEM_ATTR_PREFERENTIAL_STUDY, True ) - set_n_generate(study._study_id, study._storage, 4) # Default n_generate + set_n_generate(study._study_id, study._storage, n_generate) return PreferentialStudy(study) except optuna.exceptions.DuplicatedStudyError: From 64193747f24569f3bb1c332d990d585f4b2c53c9 Mon Sep 17 00:00:00 2001 From: Contramundum Date: Fri, 1 Sep 2023 14:55:06 +0900 Subject: [PATCH 2/5] Change API --- examples/preferential-optimization/generator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/preferential-optimization/generator.py b/examples/preferential-optimization/generator.py index 1a498fe7..e94f1d05 100644 --- a/examples/preferential-optimization/generator.py +++ b/examples/preferential-optimization/generator.py @@ -23,6 +23,7 @@ os.makedirs(artifact_path, exist_ok=True) def main() -> NoReturn: study = create_study( + n_generate=5, study_name="Preferential Optimization", storage=STORAGE_URL, sampler=PreferentialGPSampler(), From 7b71ae5695118b023589a06581d168014ced656b Mon Sep 17 00:00:00 2001 From: Contramundum Date: Fri, 1 Sep 2023 15:55:46 +0900 Subject: [PATCH 3/5] Fix test --- python_tests/preferential/test_study.py | 56 +++++++++++-------- .../preferential/test_system_attrs.py | 2 +- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/python_tests/preferential/test_study.py b/python_tests/preferential/test_study.py index 3e0b011c..3de57dae 100644 --- a/python_tests/preferential/test_study.py +++ b/python_tests/preferential/test_study.py @@ -25,7 +25,7 @@ from ..storage_supplier import StorageSupplier @parametrize_storages def test_study_set_and_get_user_attrs(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) study.set_user_attr("dataset", "MNIST") assert study.user_attrs["dataset"] == "MNIST" @@ -34,7 +34,7 @@ def test_study_set_and_get_user_attrs(storage_supplier: Callable[[], StorageSupp @parametrize_storages def test_report_and_get_preferences(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) assert len(study.preferences) == 0 for _ in range(2): @@ -51,7 +51,9 @@ def test_report_and_get_preferences(storage_supplier: Callable[[], StorageSuppli def test_study_pickle() -> None: - study_1 = create_study() + study_1 = create_study( + n_generate=4, + ) for _ in range(10): study_1.ask() assert len(study_1.trials) == 10 @@ -69,13 +71,17 @@ def test_study_pickle() -> None: def test_create_study(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: # Test creating a new study. - study = create_study(storage=storage, load_if_exists=False) + study = create_study(n_generate=4, storage=storage, load_if_exists=False) # Test `load_if_exists=True` with existing study. - create_study(study_name=study.study_name, storage=storage, load_if_exists=True) + create_study( + n_generate=4, study_name=study.study_name, storage=storage, load_if_exists=True + ) with pytest.raises(DuplicatedStudyError): - create_study(study_name=study.study_name, storage=storage, load_if_exists=False) + create_study( + n_generate=4, study_name=study.study_name, storage=storage, load_if_exists=False + ) @parametrize_storages @@ -92,7 +98,7 @@ def test_load_study(storage_supplier: Callable[[], StorageSupplier]) -> None: load_study(study_name=study_name, storage=storage) # Create a new study. - created_study = create_study(study_name=study_name, storage=storage) + created_study = create_study(n_generate=4, study_name=study_name, storage=storage) # Test loading an existing study. loaded_study = load_study(study_name=study_name, storage=storage) @@ -108,7 +114,7 @@ def test_load_study_study_name_none(storage_supplier: Callable[[], StorageSuppli study_name = str(uuid.uuid4()) - _ = create_study(study_name=study_name, storage=storage) + _ = create_study(n_generate=4, study_name=study_name, storage=storage) loaded_study = load_study(study_name=None, storage=storage) @@ -116,7 +122,7 @@ def test_load_study_study_name_none(storage_supplier: Callable[[], StorageSuppli study_name = str(uuid.uuid4()) - _ = create_study(study_name=study_name, storage=storage) + _ = create_study(n_generate=4, study_name=study_name, storage=storage) # Ambiguous study. with pytest.raises(ValueError): @@ -131,7 +137,7 @@ def test_delete_study(storage_supplier: Callable[[], StorageSupplier]) -> None: delete_study(study_name="invalid-study-name", storage=storage) # Test deleting an existing study. - study = create_study(storage=storage, load_if_exists=False) + study = create_study(n_generate=4, storage=storage, load_if_exists=False) delete_study(study_name=study.study_name, storage=storage) # Test failed to delete the study which is already deleted. @@ -141,7 +147,7 @@ def test_delete_study(storage_supplier: Callable[[], StorageSupplier]) -> None: def test_copy_study() -> None: with StorageSupplier("sqlite") as from_storage, StorageSupplier("sqlite") as to_storage: - from_study = create_study(storage=from_storage) + from_study = create_study(n_generate=4, storage=from_storage) from_study.set_user_attr("baz", "qux") for _ in range(3): trial = from_study.ask() @@ -165,8 +171,8 @@ def test_copy_study() -> None: def test_copy_study_to_study_name() -> None: with StorageSupplier("sqlite") as from_storage, StorageSupplier("sqlite") as to_storage: - from_study = create_study(study_name="foo", storage=from_storage) - _ = create_study(study_name="foo", storage=to_storage) + from_study = create_study(n_generate=4, study_name="foo", storage=from_storage) + _ = create_study(n_generate=4, study_name="foo", storage=to_storage) with pytest.raises(DuplicatedStudyError): copy_study( @@ -188,7 +194,7 @@ def test_copy_study_to_study_name() -> None: @parametrize_storages def test_add_trial(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) assert len(study.trials) == 0 trial = create_trial(value=0) @@ -198,7 +204,9 @@ def test_add_trial(storage_supplier: Callable[[], StorageSupplier]) -> None: def test_add_trial_invalid_values_length() -> None: - study = create_study() + study = create_study( + n_generate=4, + ) trial = create_trial(values=[0, 0]) with pytest.raises(ValueError): study.add_trial(trial) @@ -207,7 +215,7 @@ def test_add_trial_invalid_values_length() -> None: @parametrize_storages def test_add_trials(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) assert len(study.trials) == 0 study.add_trials([]) @@ -220,7 +228,7 @@ def test_add_trials(storage_supplier: Callable[[], StorageSupplier]) -> None: assert trial.number == i assert trial.value == i - other_study = create_study(storage=storage) + other_study = create_study(n_generate=4, storage=storage) other_study.add_trials(study.trials) assert len(other_study.trials) == 3 for i, trial in enumerate(other_study.trials): @@ -231,7 +239,7 @@ def test_add_trials(storage_supplier: Callable[[], StorageSupplier]) -> None: @parametrize_storages def test_get_trials(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) for _ in range(5): trial = study.ask() trial.suggest_int("x", 1, 5) @@ -256,7 +264,7 @@ def test_get_trials(storage_supplier: Callable[[], StorageSupplier]) -> None: @parametrize_storages def test_get_trials_state_option(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) for _ in range(3): trial = study.ask() study.mark_comparison_ready(trial) @@ -286,7 +294,9 @@ def test_get_trials_state_option(storage_supplier: Callable[[], StorageSupplier] def test_ask() -> None: - study = create_study() + study = create_study( + n_generate=4, + ) trial = study.ask() assert isinstance(trial, Trial) @@ -298,7 +308,9 @@ def test_ask_fixed_search_space() -> None: "y": distributions.CategoricalDistribution(["bacon", "spam"]), } - study = create_study() + study = create_study( + n_generate=4, + ) trial = study.ask(fixed_distributions=fixed_distributions) params = trial.params @@ -312,7 +324,7 @@ def test_report_preferences_from_another_process() -> None: with StorageSupplier("sqlite") as storage: # Create a study and ask for a new trial. - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) study.ask() study.ask() diff --git a/python_tests/preferential/test_system_attrs.py b/python_tests/preferential/test_system_attrs.py index 10448d48..d36a3fef 100644 --- a/python_tests/preferential/test_system_attrs.py +++ b/python_tests/preferential/test_system_attrs.py @@ -13,7 +13,7 @@ from ..storage_supplier import StorageSupplier @parametrize_storages def test_report_and_get_preferences(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = optuna.create_study(storage=storage) + study = optuna.create_study(n_generate=4, storage=storage) study.ask() study.ask() From 07ea8a642a86f2e2bf002935d9bd49c2f197644e Mon Sep 17 00:00:00 2001 From: Contramundum Date: Mon, 4 Sep 2023 15:25:19 +0900 Subject: [PATCH 4/5] Fix test --- optuna_dashboard/preferential/_study.py | 2 +- python_tests/preferential/test_system_attrs.py | 2 +- python_tests/test_serializers.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/optuna_dashboard/preferential/_study.py b/optuna_dashboard/preferential/_study.py index a7f494d4..ce691f42 100644 --- a/optuna_dashboard/preferential/_study.py +++ b/optuna_dashboard/preferential/_study.py @@ -263,7 +263,7 @@ class PreferentialStudy: to generate a new trial if this method returns :obj:`True`, and to wait for human evaluation if this method returns :obj:`False`. """ - return len(self.best_trials) < get_n_generate(self._study._study_id, self._study._storage) + return len(self.best_trials) < get_n_generate(self._study.system_attrs) def get_best_trials(study_id: int, storage: optuna.storages.BaseStorage) -> list[FrozenTrial]: diff --git a/python_tests/preferential/test_system_attrs.py b/python_tests/preferential/test_system_attrs.py index d36a3fef..10448d48 100644 --- a/python_tests/preferential/test_system_attrs.py +++ b/python_tests/preferential/test_system_attrs.py @@ -13,7 +13,7 @@ from ..storage_supplier import StorageSupplier @parametrize_storages def test_report_and_get_preferences(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: - study = optuna.create_study(n_generate=4, storage=storage) + study = optuna.create_study(storage=storage) study.ask() study.ask() diff --git a/python_tests/test_serializers.py b/python_tests/test_serializers.py index a75db32d..a90e0de7 100644 --- a/python_tests/test_serializers.py +++ b/python_tests/test_serializers.py @@ -24,7 +24,7 @@ def test_serialize_dict() -> None: def test_get_study_detail_is_preferential() -> None: storage = optuna.storages.InMemoryStorage() - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) study_summaries = get_study_summaries(storage) assert len(study_summaries) == 1 @@ -46,7 +46,7 @@ def test_get_study_detail_is_not_preferential() -> None: def test_get_study_summary_is_preferential() -> None: storage = optuna.storages.InMemoryStorage() - create_study(storage=storage) + create_study(n_generate=4, storage=storage) study_summaries = get_study_summaries(storage) assert len(study_summaries) == 1 From 7608f1bb9605ef3d52c6d5e09226d2ec110a21a8 Mon Sep 17 00:00:00 2001 From: Contramundum Date: Mon, 4 Sep 2023 18:21:38 +0900 Subject: [PATCH 5/5] Fix test --- python_tests/test_api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python_tests/test_api.py b/python_tests/test_api.py index ae50e29a..c0304a37 100644 --- a/python_tests/test_api.py +++ b/python_tests/test_api.py @@ -102,7 +102,7 @@ class APITestCase(TestCase): def test_get_best_trials_of_preferential_study(self) -> None: storage = optuna.storages.InMemoryStorage() - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) for _ in range(3): trial = study.ask() study.mark_comparison_ready(trial) @@ -125,7 +125,7 @@ class APITestCase(TestCase): def test_report_preference(self) -> None: storage = optuna.storages.InMemoryStorage() - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) for _ in range(3): trial = study.ask() study.mark_comparison_ready(trial) @@ -153,7 +153,7 @@ class APITestCase(TestCase): def test_skip_trial(self) -> None: storage = optuna.storages.InMemoryStorage() - study = create_study(storage=storage) + study = create_study(n_generate=4, storage=storage) trials: list[optuna.Trial] = [] for _ in range(3): trial = study.ask()