Repository: simonsobs/ocs

Issue:
NaNs / infs in session.data cause trouble in ocs-web
NaNs/infs are not compliant JSON, but are passed through from Agents to ocs-web anyway.  This causes ocs-web to totally choke on the session.data ... which is difficult to avoid.

I don't think there's a way to catch this or change how JSON is parsed by autobahn.

Two possible solutions:
1. Convert NaNs/infs to None in ocs_agent.OpSession; this will produce compliant JSON.
2. Reject NaNs/infs entirely -- i.e. forbid them forcibly and raise an exception

I think solution (1) is slightly better, but it should be noted that some Python based clients may be affected by this.  First, one can not distinguish between NaN, inf, -inf.  I think that's ok ... usually we're just trying to flag invalid data, using NaN.

Another consequence of using "None" (which renders as "null" in JSON) is that lists received by Python clients will not as easily be convertible into arrays.  E.g. if the agent sends [1.0, 2.0, None], and the client tries to turn that into a numpy array:
```
>>> np.array([1.0, 2.0, None])
array([1.0, 2.0, None], dtype=object)
```
(that's bad -- it's of dtype=object).

But a work-around on the client side is to specify dtype:
```
>>> np.array([1.,2.,None], dtype=float)
array([ 1.,  2., nan])
```

I think this is an acceptable price to pay, to have ocs-web work and also allow NaNs.

Discussion:
mhasself: Thanks @BrianJKoopman  -- I pulled in your fix, fixed the docs as you requested, and added one more test case.

Patch:
diff --git a/docs/developer/session-data.rst b/docs/developer/session-data.rst
index 480a0c6..5aa89d7 100644
--- a/docs/developer/session-data.rst
+++ b/docs/developer/session-data.rst
@@ -147,6 +147,9 @@ This block formats the latest values for each "channel" into a dictionary and
 stores it in ``session.data``.
 
 
+Structure and Content
+---------------------
+
 The structure of the ``data`` entry is not strictly defined, but
 please observe the following guidelines:
 
@@ -170,6 +173,30 @@ please observe the following guidelines:
     object to break. Changes that do take place should be announced in the
     change logs of new OCS versions.
 
+
+There are some restrictions on what data can be carried in
+``session.data``:
+
+- The session.data will ultimately be converted and transported using
+  JSON, so some containers will be automatically converted into
+  JSON-compatible forms.  Specifically note that:
+
+  - dict keys will be converted to strings.
+  - there is no distinction between lists and tuples.
+  - there is no (standardized) support for non-finite floats such as
+    inf, -inf, or NaN.
+
+- If your session.data contains any NaN, they will be converted to
+  None (which is transported as JSON null).
+- If your session.data contains inf/-inf, or other JSON-encodable
+  entities, OCS will raise an error.  To have those transported as
+  None/null, you should convert inf/-inf to NaN before storing the
+  data in session.data.
+- If your session.data includes numpy arrays (or scalars), these will
+  be converted to serializable types automatically using
+  ``numpy.tolist``.
+
+
 Client Access
 -------------
 Once your Agent is storing information in the ``session.data`` object you
diff --git a/ocs/ocs_agent.py b/ocs/ocs_agent.py
index 8230daf..af5134f 100644
--- a/ocs/ocs_agent.py
+++ b/ocs/ocs_agent.py
@@ -17,6 +17,8 @@ from autobahn.wamp.exception import ApplicationError, TransportLost
 from autobahn.exception import Disconnected
 from .ocs_twisted import in_reactor_context
 
+import json
+import math
 import time
 import datetime
 import socket
@@ -1055,7 +1057,9 @@ class OpSession:
         data : dict
           This is an area for the Operation code to store custom
           information for Control Clients to consume.  See notes
-          below.
+          below.  This structure will be tested for strict JSON
+          compliance, and certain translations performed (such as
+          converting NaN to None/null).
         messages : list
           A buffer of messages posted by the Operation.  Each element
           of the list is a tuple, (timestamp, message) where timestamp
@@ -1073,6 +1077,43 @@ class OpSession:
         advice on structuring your Agent session data.
 
         """
+        def json_safe(data, check_ok=False):
+            """Convert data so it can be serialized and decoded on
+            the other end.  This includes:
+
+            - Converting numpy arrays and scalars to generic lists and
+              Python basic types.
+
+            - Converting NaN to None (although crossbar handles
+              NaN/inf, web browsers may fail to deserialize the
+              invalid JSON this requires).
+
+            In the case of inf/-inf, a ValueError is raised.
+
+            """
+            if check_ok:
+                output = json_safe(data)
+                json.dumps(output, allow_nan=False)
+                return output
+            if isinstance(data, dict):
+                return {k: json_safe(v) for k, v in data.items()}
+            if isinstance(data, (list, tuple)):
+                return [json_safe(x) for x in data]
+            if hasattr(data, 'dtype'):
+                # numpy arrays and scalars.
+                return json_safe(data.tolist())
+            if isinstance(data, (str, int, bool)):
+                return data
+            if isinstance(data, float):
+                if math.isnan(data):
+                    return None
+                if not math.isfinite(data):
+                    raise ValueError('Session.data cannot store inf/-inf; '
+                                     'please convert to NaN.')
+            # This could still be something weird but json.dumps will
+            # probably reject it!
+            return data
+
         return {'session_id': self.session_id,
                 'op_name': self.op_name,
                 'op_code': self.op_code.value,
@@ -1080,7 +1121,7 @@ class OpSession:
                 'success': self.success,
                 'start_time': self.start_time,
                 'end_time': self.end_time,
-                'data': self.data,
+                'data': json_safe(self.data, True),
                 'messages': self.messages}
 
     @property


Test patch:
diff --git a/tests/test_ocs_agent.py b/tests/test_ocs_agent.py
index 547ce15..d300276 100644
--- a/tests/test_ocs_agent.py
+++ b/tests/test_ocs_agent.py
@@ -2,6 +2,7 @@ import ocs
 from ocs.ocs_agent import (
     OCSAgent, AgentTask, AgentProcess,
     ParamError, ParamHandler, param,
+    OpSession
 )
 from ocs.base import OpCode
 
@@ -10,6 +11,18 @@ from unittest.mock import MagicMock
 import pytest
 import pytest_twisted
 
+import json
+import math
+import numpy as np
+
+
+def create_session(op_name):
+    """Create an OpSession with a mocked app for testing."""
+    mock_app = MagicMock()
+    session = OpSession(1, op_name, app=mock_app)
+
+    return session
+
 
 def tfunc(session, a):
     """Test function to call as a mocked OCS Task. We double it as the start
@@ -438,6 +451,45 @@ def test_status_no_session(mock_agent):
     assert res[2] == {}
 
 
+@pytest.mark.parametrize("key,value,expected", [('a', 1, 1),
+                                                ('b', 'string', 'string'),
+                                                ('c', [1, 2., 'blech'], [1, 2., 'blech']),
+                                                ('d', [1., 2., math.nan], [1., 2., None]),
+                                                ('e', np.int64(10), 10),
+                                                ('f', np.array([10, 20, 30]), [10, 20, 30]),
+                                                ('g', np.array([1., 2., math.nan]), [1., 2., None]),
+                                                ('h', {'x': math.nan}, {'x': None})])
+def test_session_data_good(key, value, expected):
+    """Test that session.data is encoded as expected and can be
+    JSON-serialized.
+
+    """
+    session = create_session('test_encoding')
+    session.data = {key: value}
+
+    encoded = session.encoded()
+    print(encoded['data'])
+
+    data = encoded['data']
+    assert (key in data)
+    assert data[key] == expected
+
+    json.dumps(data, allow_nan=False)
+
+
+@pytest.mark.parametrize("key,value", [('fail_a', math.inf),
+                                       ('fail_b', [1., 2., -math.inf]),
+                                       ('fail_c', {'x': math.inf})])
+def test_session_data_bad(key, value):
+    """Test that invalid session.data raises an error."""
+
+    session = create_session('test_encoding')
+    session.data = {key: value}
+
+    with pytest.raises(ValueError):
+        session.encoded()
+
+
 #
 # Tests for the @param decorator
 #