Hparams: Treat Nan/Infinity numeric hparam values as unset values. (#6496)

## Motivation for features / changes

There are cases where users might log hparam numeric values that are NaN
or Infinity. Recall, we use proto's Value message for logging and
storing hparam values. While processing these values in the python layer
of the Hparams plugin, we may run these Value message through
json_format.MessageToJson, at which point the json_format library raises
an error.

The limitation is documented at
https://protobuf.dev/reference/protobuf/google.protobuf/#value.
"[A]ttempting to serialize NaN or Infinity results in error."

## Technical description of changes

We now will try to avoid calling MessageToJson on these problematic
hparam values. We instead chose to treat these as "unset" values --
equivalent to having not set an hparam value at all. It will mean the
values will not contribute to the calculation of discrete domains. And
it means that the values will appear blank in the hparams UI rather than
appearing as "NaN".

## Alternate designs / implementations considered (or N/A)

Given infinite amount of time:
* Fix json_format to handle NaN and Infinity :O.
* Change the hparams api.proto to use something other than proto's Value
for representing hparam values.

Other reasonable options:
* Change the NaN numeric value to "NaN" string. This was my first choice
but it unfortunately leads to other complications - for instance the
plugin will raise errors when it encounters a "NaN" string for an hparam
that is supposed to have a numeric value.

Author: bmd3k

tensorboard/plugins/hparams/BUILD

@@ -29,6 +29,7 @@ py_library(
         "download_data.py",
         "get_experiment.py",
         "hparams_plugin.py",
+        "json_format_compat.py",
         "list_metric_evals.py",
         "list_session_groups.py",
         "metrics.py",
@@ -115,6 +116,15 @@ py_test(
     ],
 )
 
+py_test(
+    name = "json_format_compat_test",
+    size = "small",
+    srcs = [
+        "json_format_compat_test.py",
+    ],
+    deps = [":hparams_plugin"],
+)
+
 py_binary(
     name = "hparams_demo",
     srcs = ["hparams_demo.py"],

tensorboard/plugins/hparams/backend_context.py

@@ -22,6 +22,7 @@
 
 from tensorboard.data import provider
 from tensorboard.plugins.hparams import api_pb2
+from tensorboard.plugins.hparams import json_format_compat
 from tensorboard.plugins.hparams import metadata
 from google.protobuf import json_format
 from tensorboard.plugins.scalar import metadata as scalar_metadata
@@ -282,11 +283,6 @@ def _compute_hparam_info_from_values(self, name, values):
         # If all values have the same type, then that is the type used.
         # Otherwise, the returned type is DATA_TYPE_STRING.
         result = api_pb2.HParamInfo(name=name, type=api_pb2.DATA_TYPE_UNSET)
-        distinct_values = set(
-            _protobuf_value_to_string(v)
-            for v in values
-            if _protobuf_value_type(v)
-        )
         for v in values:
             v_type = _protobuf_value_type(v)
             if not v_type:
@@ -304,6 +300,11 @@ def _compute_hparam_info_from_values(self, name, values):
             return None
 
         if result.type == api_pb2.DATA_TYPE_STRING:
+            distinct_values = set(
+                _protobuf_value_to_string(v)
+                for v in values
+                if _can_be_converted_to_string(v)
+            )
             result.domain_discrete.extend(distinct_values)
 
         if result.type == api_pb2.DATA_TYPE_BOOL:
@@ -453,6 +454,12 @@ def _find_longest_parent_path(path_set, path):
     return path
 
 
+def _can_be_converted_to_string(value):
+    if not _protobuf_value_type(value):
+        return False
+    return json_format_compat.is_serializable_value(value)
+
+
 def _protobuf_value_type(value):
     """Returns the type of the google.protobuf.Value message as an
     api.DataType.

tensorboard/plugins/hparams/backend_context_test.py

@@ -344,6 +344,33 @@ def test_experiment_with_bool_types(self):
         _canonicalize_experiment(actual_exp)
         self.assertProtoEquals(expected_exp, actual_exp)
 
+    def test_experiment_with_string_domain_and_invalid_number_values(self):
+        self.session_1_start_info_ = """
+            hparams:[
+              {key: 'maybe_invalid' value: {string_value: 'force_to_string_type'}}
+            ]
+        """
+        self.session_2_start_info_ = """
+            hparams:[
+              {key: 'maybe_invalid' value: {number_value: NaN}}
+            ]
+        """
+        self.session_3_start_info_ = """
+            hparams:[
+              {key: 'maybe_invalid' value: {number_value: Infinity}}
+            ]
+        """
+        expected_hparam_info = """
+            name: 'maybe_invalid'
+            type: DATA_TYPE_STRING
+            domain_discrete: {
+              values: [{string_value: 'force_to_string_type'}]
+            }
+        """
+        actual_exp = self._experiment_from_metadata()
+        self.assertLen(actual_exp.hparam_infos, 1)
+        self.assertProtoEquals(expected_hparam_info, actual_exp.hparam_infos[0])
+
     def test_experiment_without_any_hparams(self):
         request_ctx = context.RequestContext()
         actual_exp = self._experiment_from_metadata()

tensorboard/plugins/hparams/json_format_compat.py

@@ -0,0 +1,38 @@
+# Copyright 2023 The TensorFlow Authors. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ==============================================================================
+
+import math
+
+
+def is_serializable_value(value):
+    """Returns whether a protobuf Value will be serializable by MessageToJson.
+
+    The json_format documentation states that "attempting to serialize NaN or
+    Infinity results in error."
+
+    https://protobuf.dev/reference/protobuf/google.protobuf/#value
+
+    Args:
+      value: A value of type protobuf.Value.
+
+    Returns:
+      True if the Value should be serializable without error by MessageToJson.
+      False, otherwise.
+    """
+    if not value.HasField("number_value"):
+        return True
+
+    number_value = value.number_value
+    return not math.isnan(number_value) and not math.isinf(number_value)

tensorboard/plugins/hparams/json_format_compat_test.py

@@ -0,0 +1,64 @@
+# Copyright 2023 The TensorFlow Authors. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from absl.testing import absltest
+from google.protobuf import struct_pb2
+from tensorboard.plugins.hparams import json_format_compat
+
+
+class TestCase(absltest.TestCase):
+    def test_real_value_is_serializable(self):
+        self.assertTrue(
+            json_format_compat.is_serializable_value(
+                struct_pb2.Value(number_value=1.0)
+            )
+        )
+        self.assertTrue(
+            json_format_compat.is_serializable_value(
+                struct_pb2.Value(string_value="nan")
+            )
+        )
+        self.assertTrue(
+            json_format_compat.is_serializable_value(
+                struct_pb2.Value(bool_value=False)
+            )
+        )
+
+    def test_empty_value_is_serializable(self):
+        self.assertTrue(
+            json_format_compat.is_serializable_value(struct_pb2.Value())
+        )
+
+    def test_nan_value_is_not_serializable(self):
+        self.assertFalse(
+            json_format_compat.is_serializable_value(
+                struct_pb2.Value(number_value=float("nan"))
+            )
+        )
+
+    def test_infinity_value_is_not_serializable(self):
+        self.assertFalse(
+            json_format_compat.is_serializable_value(
+                struct_pb2.Value(number_value=float("inf"))
+            )
+        )
+        self.assertFalse(
+            json_format_compat.is_serializable_value(
+                struct_pb2.Value(number_value=float("-inf"))
+            )
+        )
+
+
+if __name__ == "__main__":
+    absltest.main()

tensorboard/plugins/hparams/list_session_groups.py

@@ -26,6 +26,7 @@
 from tensorboard.data import provider
 from tensorboard.plugins.hparams import api_pb2
 from tensorboard.plugins.hparams import error
+from tensorboard.plugins.hparams import json_format_compat
 from tensorboard.plugins.hparams import metadata
 from tensorboard.plugins.hparams import metrics
 
@@ -246,6 +247,13 @@ def _add_session(self, session, start_info, groups_by_name):
             # There doesn't seem to be a way to initialize a protobuffer map in the
             # constructor.
             for (key, value) in start_info.hparams.items():
+                if not json_format_compat.is_serializable_value(value):
+                    # NaN number_value cannot be serialized by higher level layers
+                    # that are using json_format.MessageToJson(). To workaround
+                    # the issue we do not copy them to the session group and
+                    # effectively treat them as "unset".
+                    continue
+
                 group.hparams[key].CopyFrom(value)
             groups_by_name[group_name] = group
 

tensorboard/plugins/hparams/list_session_groups_test.py

@@ -1172,6 +1172,137 @@ def test_sort_one_column_with_missing_values(self):
             expected_total_size=3,
         )
 
+    def _mock_list_tensors_invalid_number_values(
+        self, ctx, *, experiment_id, plugin_name, run_tag_filter
+    ):
+        hparams_content = {
+            "session_1": {
+                metadata.SESSION_START_INFO_TAG: self._serialized_plugin_data(
+                    DATA_TYPE_SESSION_START_INFO,
+                    """
+                    hparams:{ key: 'maybe_bad' value: { number_value: 1 } }
+                    group_name: 'group_1'
+                    """,
+                )
+            },
+            "session_2": {
+                metadata.SESSION_START_INFO_TAG: self._serialized_plugin_data(
+                    DATA_TYPE_SESSION_START_INFO,
+                    """
+                    hparams:{ key: 'maybe_bad' value: { number_value: nan } }
+                    group_name: 'group_2'
+                    """,
+                ),
+            },
+            "session_3": {
+                metadata.SESSION_START_INFO_TAG: self._serialized_plugin_data(
+                    DATA_TYPE_SESSION_START_INFO,
+                    """
+                    hparams:{ key: 'maybe_bad' value: { number_value: -infinity } }
+                    group_name: 'group_3'
+                    """,
+                ),
+            },
+            "session_4": {
+                metadata.SESSION_START_INFO_TAG: self._serialized_plugin_data(
+                    DATA_TYPE_SESSION_START_INFO,
+                    """
+                    hparams:{ key: 'maybe_bad' value: { number_value: 4.0 } }
+                    group_name: 'group_4'
+                    """,
+                ),
+            },
+        }
+        result = {}
+        for (run, tag_to_content) in hparams_content.items():
+            result.setdefault(run, {})
+            for (tag, content) in tag_to_content.items():
+                t = provider.TensorTimeSeries(
+                    max_step=0,
+                    max_wall_time=0,
+                    plugin_content=content,
+                    description="",
+                    display_name="",
+                )
+                result[run][tag] = t
+        return result
+
+    def test_hparams_with_invalid_number_values(self):
+        self._mock_tb_context.data_provider.list_tensors.side_effect = (
+            self._mock_list_tensors_invalid_number_values
+        )
+        request = """
+            start_index: 0
+            slice_size: 10
+            allowed_statuses: [STATUS_UNKNOWN]
+        """
+        groups = self._run_handler(request).session_groups
+        self.assertLen(groups, 4)
+        self.assertEqual(1, groups[0].hparams.get("maybe_bad").number_value)
+        self.assertEqual(None, groups[1].hparams.get("maybe_bad"))
+        self.assertEqual(None, groups[2].hparams.get("maybe_bad"))
+        self.assertEqual(4, groups[3].hparams.get("maybe_bad").number_value)
+
+    def test_sort_hparams_with_invalid_number_values(self):
+        self._mock_tb_context.data_provider.list_tensors.side_effect = (
+            self._mock_list_tensors_invalid_number_values
+        )
+        self._verify_handler(
+            request="""
+                start_index: 0
+                slice_size: 10
+                allowed_statuses: [STATUS_UNKNOWN]
+                col_params: {
+                  hparam: 'maybe_bad'
+                  order: ORDER_DESC
+                }
+            """,
+            expected_session_group_names=[
+                "group_4",
+                "group_1",
+                "group_2",
+                "group_3",
+            ],
+            expected_total_size=4,
+        )
+
+    def test_filter_hparams_include_invalid_number_values(self):
+        self._mock_tb_context.data_provider.list_tensors.side_effect = (
+            self._mock_list_tensors_invalid_number_values
+        )
+        self._verify_handler(
+            request="""
+                start_index: 0
+                slice_size: 10
+                allowed_statuses: [STATUS_UNKNOWN]
+                col_params: {
+                  hparam: 'maybe_bad'
+                  order: ORDER_DESC
+                  filter_interval: { min_value: 2.0 max_value: 10.0 }
+                }
+            """,
+            expected_session_group_names=["group_4", "group_2", "group_3"],
+            expected_total_size=3,
+        )
+
+    def test_filer_hparams_exclude_invalid_number_values(self):
+        self._mock_tb_context.data_provider.list_tensors.side_effect = (
+            self._mock_list_tensors_invalid_number_values
+        )
+        self._verify_handler(
+            request="""
+                start_index: 0
+                slice_size: 10
+                allowed_statuses: [STATUS_UNKNOWN]
+                col_params: {
+                  hparam: 'maybe_bad'
+                  exclude_missing_values: true
+                }
+            """,
+            expected_session_group_names=["group_1", "group_4"],
+            expected_total_size=2,
+        )
+
     def test_experiment_without_any_hparams(self):
         self._mock_tb_context.data_provider.list_tensors.side_effect = None
         self._hyperparameters = []