Make Metric Name Space checks less broad (#5042)

Summary:
Pull Request resolved: #5042

Isolate checks for spaces in metric names to only areas where they are relevant (e.g. parsing string constraints for experiment creation)

Reviewed By: ItsMrLin

Differential Revision: D96814558

fbshipit-source-id: b5a0ae8f850f5e4d3960ec2da3522600726b7597

Author: eonofrey

ax/core/tests/test_outcome_constraint.py

@@ -165,6 +165,7 @@ def setUp(self) -> None:
         self.minimize_metric = Metric(name="bar", lower_is_better=True)
         self.maximize_metric = Metric(name="baz", lower_is_better=False)
         self.ambiguous_metric = Metric(name="buz")
+        self.bound = 0.5
 
     def test_Init(self) -> None:
         with warnings.catch_warnings(record=True) as w:

ax/service/tests/test_instantiation_utils.py

@@ -34,17 +34,19 @@ def test_parameter_type_validation(self) -> None:
             InstantiationBase._get_parameter_type(list)
 
     def test_make_search_space(self) -> None:
-        with self.assertRaisesRegex(ValueError, "cannot contain spaces"):
-            InstantiationBase.make_search_space(
-                parameters=[
-                    {
-                        "name": "x space 1",
-                        "type": "range",
-                        "bounds": [0.0, 1.0],
-                    }
-                ],
-                parameter_constraints=None,
-            )
+        # Parameter names with spaces should be allowed in search spaces
+        # (only rejected in constraint string parsing).
+        ss = InstantiationBase.make_search_space(
+            parameters=[
+                {
+                    "name": "x space 1",
+                    "type": "range",
+                    "bounds": [0.0, 1.0],
+                }
+            ],
+            parameter_constraints=None,
+        )
+        self.assertIn("x space 1", ss.parameters)
 
     def test_constraint_from_str(self) -> None:
         x1 = RangeParameter(
@@ -96,7 +98,7 @@ def test_constraint_from_str(self) -> None:
             "x1 + x2 + x3 <= 3", {"x1": x1, "x2": x2, "x3": x3}
         )
 
-        with self.assertRaisesRegex(AssertionError, "Outcome constraint 'm1"):
+        with self.assertRaisesRegex(ValueError, "Outcome constraint 'm1"):
             InstantiationBase.outcome_constraint_from_str("m1 == 2*m2")
 
         self.assertEqual(three_val_constaint.bound, 3.0)
@@ -191,6 +193,29 @@ def test_constraint_from_str(self) -> None:
                 "x1 + x2 / 2.0 + x3 >= 3", {"x1": x1, "x2": x2, "x3": x3}
             )
 
+    def test_spaces_in_metric_and_parameter_names(self) -> None:
+        # Metric and parameter names with spaces are allowed everywhere
+        # except in constraint string parsing, where split() would break.
+        metric = InstantiationBase._make_metric(name="my metric")
+        self.assertEqual(metric.name, "my metric")
+
+        experiment = InstantiationBase.make_experiment(
+            parameters=[{"name": "x", "type": "range", "bounds": [0, 1]}],
+            tracking_metric_names=["my metric", "another metric"],
+        )
+        tracking_metric_names = [m.name for m in experiment.tracking_metrics]
+        self.assertIn("my metric", tracking_metric_names)
+        self.assertIn("another metric", tracking_metric_names)
+
+        # Constraint string parsing rejects spaces (can't tokenize correctly).
+        x1 = RangeParameter(
+            name="x 1", parameter_type=ParameterType.FLOAT, lower=0.1, upper=4.0
+        )
+        with self.assertRaisesRegex(ValueError, "cannot contain spaces"):
+            InstantiationBase.constraint_from_str("x 1 <= 3", {"x 1": x1})
+        with self.assertRaisesRegex(ValueError, "cannot contain spaces"):
+            InstantiationBase.outcome_constraint_from_str("my metric <= 3")
+
     def test_add_tracking_metrics(self) -> None:
         experiment = InstantiationBase.make_experiment(
             parameters=[{"name": "x", "type": "range", "bounds": [0, 1]}],
@@ -212,8 +237,12 @@ def test_make_objectives(self) -> None:
         with self.assertRaisesRegex(ValueError, "specify 'minimize' or 'maximize'"):
             InstantiationBase.make_objectives({"branin": "unknown"})
 
-        with self.assertRaisesRegex(ValueError, "cannot contain spaces"):
-            InstantiationBase.make_objectives({"branin space": "maximize"})
+        # Metric names with spaces should be allowed in objectives
+        # (only rejected in constraint string parsing).
+        objectives_with_spaces = InstantiationBase.make_objectives(
+            {"branin space": "maximize"}
+        )
+        self.assertEqual(objectives_with_spaces[0].metric_names[0], "branin space")
 
         objectives = InstantiationBase.make_objectives(
             {"branin": "minimize", "currin": "maximize"}

ax/service/utils/instantiation.py

@@ -174,12 +174,6 @@ def _make_metric(
         for_opt_config: bool = False,
         metric_definitions: dict[str, dict[str, Any]] | None = None,
     ) -> Metric:
-        if " " in name:
-            raise ValueError(
-                "Metric names cannot contain spaces when used with AxClient. Got "
-                f"{name!r}."
-            )
-
         metric_definitions = metric_definitions or {}
 
         metric_class, kwargs = cls._get_deserialized_metric_kwargs(
@@ -374,12 +368,6 @@ def parameter_from_json(
                 "'bool' or 'str'."
             )
 
-        if " " in name:
-            raise ValueError(
-                "Parameter names cannot contain spaces when used with AxClient. Got "
-                f"{name!r}."
-            )
-
         if parameter_class == "range":
             return cls._make_range_param(
                 name=name,
@@ -440,6 +428,12 @@ def constraint_from_str(
             An instantiated ParameterConstraint, either an OrderConstraint or a
             ParameterConstraint, representing a linear constraint.
         """
+        for param_name in parameters:
+            if " " in param_name:
+                raise ValueError(
+                    "Parameter names cannot contain spaces when used in "
+                    f"constraint strings. Got {param_name!r}."
+                )
         tokens = representation.split()
         try:
             float(tokens[-1])
@@ -482,11 +476,13 @@ def outcome_constraint_from_str(
     ) -> OutcomeConstraint:
         """Parse string representation of an outcome constraint."""
         tokens = representation.split()
-        assert len(tokens) == 3 and tokens[1] in COMPARISON_OPS, (
-            f"Outcome constraint '{representation}' should be of "
-            "form `metric_name >= x`, where x is a "
-            "float bound and comparison operator is >= or <=."
-        )
+        if len(tokens) != 3 or tokens[1] not in COMPARISON_OPS:
+            raise ValueError(
+                f"Outcome constraint '{representation}' should be of "
+                "form `metric_name >= x`, where x is a float bound and "
+                "comparison operator is >= or <=. Note that metric names "
+                "cannot contain spaces in constraint strings."
+            )
         op = COMPARISON_OPS[tokens[1]]
         rel = False
         try: