From eff98da68acc7b0fe23c88e871936d63bb65a97a Mon Sep 17 00:00:00 2001 From: Xavier Bouthillier Date: Tue, 19 Apr 2022 16:06:39 -0400 Subject: [PATCH 1/4] Improve prior build error message If there is an error during the attempt of building a builtin prior (methods implemented in SpaceBuilder), the builder assumes the expression is not a builtin prior and then tries to create a scipy.stat prior which also fails. The error message is then that the name of the expression is not a valid prior. We should instead explicitly verify if the name of the prior is one that is builtin and then try to build it without silencing error messages. This way we get the correct error message for the prior. --- src/orion/algo/space.py | 6 +++--- src/orion/core/io/space_builder.py | 18 +++++++++++++----- tests/unittests/core/io/test_space_builder.py | 11 +++++++++++ 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/orion/algo/space.py b/src/orion/algo/space.py index 31ce2192a..84059ace8 100644 --- a/src/orion/algo/space.py +++ b/src/orion/algo/space.py @@ -880,13 +880,13 @@ class Fidelity(Dimension): # pylint:disable=super-init-not-called def __init__(self, name, low, high, base=2): if low <= 0: - raise AttributeError("Minimum resources must be a positive number.") + raise ValueError("Minimum resources must be a positive number.") elif low > high: - raise AttributeError( + raise ValueError( "Minimum resources must be smaller than maximum resources." ) if base < 1: - raise AttributeError("Base should be greater than or equal to 1") + raise ValueError("Base should be greater than or equal to 1") self.name = name self.low = int(low) self.high = int(high) diff --git a/src/orion/core/io/space_builder.py b/src/orion/core/io/space_builder.py index 1b4ee5795..c73607957 100644 --- a/src/orion/core/io/space_builder.py +++ b/src/orion/core/io/space_builder.py @@ -193,6 +193,16 @@ def loguniform(self, *args, **kwargs): klass = _real_or_int(kwargs) return klass(name, "reciprocal", *args, **kwargs) + def _build_custom(self, expression, globals_, locals_): + name = expression.split("(")[0] + if not hasattr(self, name): + return None + try: + dimension = eval("self." + expression, globals_, {"self": self}) + return dimension + except AttributeError as error: + raise AttributeError(f"Parameter '{name}': {error}") from error + def _build(self, name, expression): """Build a `Dimension` object using a string as its `name` and another string, `expression`, from configuration as a "function" to create it. @@ -205,12 +215,10 @@ def _build(self, name, expression): import numpy as np globals_["np"] = np - try: - dimension = eval("self." + expression, globals_, {"self": self}) - + locals_ = {"self": self} + dimension = self._build_custom(expression, globals_, locals_) + if dimension is not None: return dimension - except AttributeError: - pass # If not found in the methods of `DimensionBuilder`. # try to see if it is legit scipy stuff and call a `Dimension` diff --git a/tests/unittests/core/io/test_space_builder.py b/tests/unittests/core/io/test_space_builder.py index f4dce060d..be1fa0eae 100644 --- a/tests/unittests/core/io/test_space_builder.py +++ b/tests/unittests/core/io/test_space_builder.py @@ -127,6 +127,17 @@ def test_build_fails_because_of_ValueError_on_init(self, dimbuilder): assert "Parameter" in str(exc.value) assert "size" in str(exc.value.__cause__) + def test_build_fails_because_of_invalid_arguments_for_custom_prior( + self, dimbuilder + ): + """Build fails because ValueError happens on init of builtin prior.""" + with pytest.raises(ValueError) as exc: + dimbuilder.build("yolo2", "fidelity(0, 10, base=1)") + assert "Parameter" in str(exc.value) + assert "Minimum resources must be a positive number." in str( + exc.value.__cause__ + ) + def test_build_gaussian(self, dimbuilder): """Check that gaussian/normal/norm is built into reciprocal correctly.""" dim = dimbuilder.build("yolo", "gaussian(3, 5)") From 35c3162a82799123d817ea49c68bf74241faeee0 Mon Sep 17 00:00:00 2001 From: Xavier Bouthillier Date: Tue, 17 May 2022 13:54:54 -0400 Subject: [PATCH 2/4] Use passed locals --- src/orion/core/io/space_builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/orion/core/io/space_builder.py b/src/orion/core/io/space_builder.py index c73607957..de15baf47 100644 --- a/src/orion/core/io/space_builder.py +++ b/src/orion/core/io/space_builder.py @@ -198,7 +198,7 @@ def _build_custom(self, expression, globals_, locals_): if not hasattr(self, name): return None try: - dimension = eval("self." + expression, globals_, {"self": self}) + dimension = eval("self." + expression, globals_, locals_) return dimension except AttributeError as error: raise AttributeError(f"Parameter '{name}': {error}") from error From da023634c37eff8733e8add5c183dfa1534a1b5b Mon Sep 17 00:00:00 2001 From: Xavier Bouthillier Date: Tue, 5 Jul 2022 16:16:36 -0400 Subject: [PATCH 3/4] Work with proper Exceptions in space build --- src/orion/algo/space.py | 6 +++--- src/orion/core/io/space_builder.py | 6 +++--- tests/unittests/algo/test_space.py | 14 +++++++------- tests/unittests/core/io/test_space_builder.py | 2 +- tests/unittests/core/test_branch_config.py | 4 ++-- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/orion/algo/space.py b/src/orion/algo/space.py index 84059ace8..aeb8b5962 100644 --- a/src/orion/algo/space.py +++ b/src/orion/algo/space.py @@ -130,17 +130,17 @@ def __init__(self, name, prior, *args, **kwargs): def validate(self): """Validate dimension arguments""" if "random_state" in self._kwargs or "seed" in self._kwargs: - raise ValueError( + raise TypeError( "random_state/seed cannot be set in a " "parameter's definition! Set seed globally!" ) if "discrete" in self._kwargs: - raise ValueError( + raise TypeError( "Do not use kwarg 'discrete' on `Dimension`, " "use pure `_Discrete` class instead!" ) if "size" in self._kwargs: - raise ValueError("Use 'shape' keyword only instead of 'size'.") + raise TypeError("Use 'shape' keyword only instead of 'size'.") if ( self.default_value is not self.NO_DEFAULT_VALUE diff --git a/src/orion/core/io/space_builder.py b/src/orion/core/io/space_builder.py index de15baf47..650bcddd9 100644 --- a/src/orion/core/io/space_builder.py +++ b/src/orion/core/io/space_builder.py @@ -252,9 +252,9 @@ def build(self, name, expression): """ try: dimension = self._build(name, expression) - except ValueError as exc: - raise TypeError( - "Parameter '{}': Incorrect arguments.".format(name) + except (TypeError, ValueError) as exc: + raise type(exc)( + f"Parameter '{name}' has incorrect arguments: {str(exc)}" ) from exc except IndexError as exc: error_msg = ( diff --git a/tests/unittests/algo/test_space.py b/tests/unittests/algo/test_space.py index 1e49b0b83..25a80114c 100644 --- a/tests/unittests/algo/test_space.py +++ b/tests/unittests/algo/test_space.py @@ -96,17 +96,17 @@ def test_shaped_instance(self, seed): def test_ban_size_kwarg(self): """Should not be able to use 'size' kwarg.""" - with pytest.raises(ValueError): + with pytest.raises(TypeError): Dimension("yolo", "norm", 0.9, size=(3, 2)) def test_ban_seed_kwarg(self): """Should not be able to use 'seed' kwarg.""" - with pytest.raises(ValueError): + with pytest.raises(TypeError): Dimension("yolo", "norm", 0.9, seed=8) def test_ban_rng_kwarg(self): """Should not be able to use 'random_state' kwarg.""" - with pytest.raises(ValueError): + with pytest.raises(TypeError): Dimension("yolo", "norm", 0.9, random_state=8) def test_with_predefined_dist(self, seed): @@ -118,7 +118,7 @@ def test_with_predefined_dist(self, seed): def test_ban_discrete_kwarg(self): """Do not allow use for 'discrete' kwarg, because now there's `_Discrete`.""" - with pytest.raises(ValueError) as exc: + with pytest.raises(TypeError) as exc: Dimension("yolo", "uniform", -3, 4, shape=(4, 4), discrete=True) assert "pure `_Discrete`" in str(exc.value) @@ -699,13 +699,13 @@ def test_fidelity_set_base(self): def test_min_resources(self): """Test that an error is raised if min is smaller than 1""" - with pytest.raises(AttributeError) as exc: + with pytest.raises(ValueError) as exc: Fidelity("epoch", 0, 2) assert "Minimum resources must be a positive number." == str(exc.value) def test_min_max_resources(self): """Test that an error is raised if min is larger than max""" - with pytest.raises(AttributeError) as exc: + with pytest.raises(ValueError) as exc: Fidelity("epoch", 3, 2) assert "Minimum resources must be smaller than maximum resources." == str( exc.value @@ -713,7 +713,7 @@ def test_min_max_resources(self): def test_base(self): """Test that an error is raised if base is smaller than 1""" - with pytest.raises(AttributeError) as exc: + with pytest.raises(ValueError) as exc: Fidelity("epoch", 1, 2, 0) assert "Base should be greater than or equal to 1" == str(exc.value) diff --git a/tests/unittests/core/io/test_space_builder.py b/tests/unittests/core/io/test_space_builder.py index be1fa0eae..d2db33d4a 100644 --- a/tests/unittests/core/io/test_space_builder.py +++ b/tests/unittests/core/io/test_space_builder.py @@ -186,7 +186,7 @@ def test_build_choices(self, dimbuilder): assert dim._prior_name == "Distribution" assert isinstance(dim.prior, dists.rv_discrete) - with pytest.raises(TypeError) as exc: + with pytest.raises(ValueError) as exc: dimbuilder.build("yolo2", "choices({'adfa': 0.1, 3: 0.4})") assert "Parameter" in str(exc.value) assert "sum" in str(exc.value.__cause__) diff --git a/tests/unittests/core/test_branch_config.py b/tests/unittests/core/test_branch_config.py index 31be02f20..d36f18efd 100644 --- a/tests/unittests/core/test_branch_config.py +++ b/tests/unittests/core/test_branch_config.py @@ -772,9 +772,9 @@ def test_add_bad_default(self, parent_config, new_config_with_w): -1 ] = "-w_d~+normal(0,1,default_value='a')" backward.populate_space(new_config_with_w) - with pytest.raises(TypeError) as exc: + with pytest.raises(ValueError) as exc: detect_conflicts(parent_config, new_config_with_w) - assert "Parameter '/w_d': Incorrect arguments." in str(exc.value) + assert "Parameter '/w_d' has incorrect arguments: a is not" in str(exc.value) def test_add_changed(self, parent_config, changed_config): """Test if changed dimension conflict is automatically resolved""" From c667aedee413641e95d22360698318149f06e470 Mon Sep 17 00:00:00 2001 From: Xavier Bouthillier Date: Wed, 6 Jul 2022 12:57:24 -0400 Subject: [PATCH 4/4] Work with proper Exception in space builder --- tests/unittests/core/test_transformer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unittests/core/test_transformer.py b/tests/unittests/core/test_transformer.py index 55e0d6026..b357e6265 100644 --- a/tests/unittests/core/test_transformer.py +++ b/tests/unittests/core/test_transformer.py @@ -825,7 +825,7 @@ def test_validate(self, tdim, tdim2): tdim2.original_dimension._default_value = "bad-default" # It does not pass - with pytest.raises(ValueError) as exc: + with pytest.raises(TypeError) as exc: tdim.validate() assert "Use 'shape' keyword only instead of 'size'." in str(exc.value)