allow "suffix" be optional in product queries for Python algorithms

plugins/python/src/modulewrap.cpp

@@ -19,7 +19,7 @@
 // followed by the intended call, and another from Python -> C++.
 //
 // Since product_query inputs, list the creator name, the suffix can remain
-// the same through out the chain (as does the layer), distinguishing the
+// the same throughout the chain (as does the layer), distinguishing the
 // stage with the creator name (and thus the node names) only.
 //
 // The chain is as follows (last step not added for observers):
@@ -32,6 +32,10 @@
 //  Python -> C++:   creator: py_<name>
 //                   name:    <name>
 //                   output:  <output>
+//
+// For now, each input will have its own converter, even if multiple nodes
+// need that same input translated. This simplifies memory management, but
+// can cause a performance bottleneck (since all require the GIL).
 
 using namespace phlex::experimental;
 using phlex::concurrency;
@@ -225,27 +229,30 @@ namespace {
 
       PyObject* pyc = PyDict_GetItemString(item, "creator");
       if (!pyc || !PyUnicode_Check(pyc)) {
-        PyErr_Format(PyExc_ValueError, "missing \"creator\" for input specification");
+        PyErr_Format(PyExc_TypeError, "missing \"creator\" or not a string");
         break;
       }
       char const* c = PyUnicode_AsUTF8(pyc);
 
       PyObject* pyl = PyDict_GetItemString(item, "layer");
       if (!pyl || !PyUnicode_Check(pyl)) {
-        PyErr_Format(PyExc_ValueError, "missing \"layer\" for input specification");
+        PyErr_Format(PyExc_TypeError, "missing \"layer\" or not a string");
         break;
       }
       char const* l = PyUnicode_AsUTF8(pyl);
 
+      std::optional<identifier> s;
       PyObject* pys = PyDict_GetItemString(item, "suffix");
-      if (!pys || !PyUnicode_Check(pys)) {
-        PyErr_Format(PyExc_ValueError, "missing \"suffix\" for input specification");
-        break;
-      }
-      char const* s = PyUnicode_AsUTF8(pys);
+      if (pys) {
+        if (!PyUnicode_Check(pys)) {
+          PyErr_Format(PyExc_TypeError, "provided \"suffix\" is not a string");
+          break;
+        }
+        s = identifier(PyUnicode_AsUTF8(pys));
+      } else
+        PyErr_Clear();
 
-      cargs.push_back(
-        product_query{.creator = identifier(c), .layer = identifier(l), .suffix = identifier(s)});
+      cargs.push_back(product_query{.creator = identifier(c), .layer = identifier(l), .suffix = s});
     }
 
     if (PyErr_Occurred())
@@ -680,7 +687,8 @@ static bool insert_input_converters(py_phlex_module* mod,
     auto const& inp_type = input_types[i];
 
     std::string const& pyname = input_converter_name(cname, i);
-    std::string output = "py_" + std::string{static_cast<std::string_view>(*inp_pq.suffix)};
+    std::string output =
+      "py_" + (inp_pq.suffix ? std::string{static_cast<std::string_view>(*inp_pq.suffix)} : "");
 
     if (inp_type == "bool")
       insert_converter(mod, pyname, bool_to_py, inp_pq, output);
@@ -773,7 +781,8 @@ static PyObject* md_transform(py_phlex_module* mod, PyObject* args, PyObject* kw
 
   auto pq0 = input_queries[0];
   std::string c0 = input_converter_name(cname, 0);
-  std::string suff0 = "py_" + std::string{static_cast<std::string_view>(*pq0.suffix)};
+  std::string suff0 =
+    "py_" + (pq0.suffix ? std::string{static_cast<std::string_view>(*pq0.suffix)} : "");
 
   switch (input_queries.size()) {
   case 1: {
@@ -787,9 +796,9 @@ static PyObject* md_transform(py_phlex_module* mod, PyObject* args, PyObject* kw
   case 2: {
     auto* pyc = new py_callback_2{callable};
     auto pq1 = input_queries[1];
-    std::string suff1 = "py_" + std::string{static_cast<std::string_view>(*pq1.suffix)};
-
     std::string c1 = input_converter_name(cname, 1);
+    std::string suff1 =
+      "py_" + (pq1.suffix ? std::string{static_cast<std::string_view>(*pq1.suffix)} : "");
     mod->ph_module->transform(pyname, *pyc, concurrency::serial)
       .input_family(
         product_query{.creator = identifier(c0), .layer = pq0.layer, .suffix = identifier(suff0)},
@@ -801,10 +810,12 @@ static PyObject* md_transform(py_phlex_module* mod, PyObject* args, PyObject* kw
     auto* pyc = new py_callback_3{callable};
     auto pq1 = input_queries[1];
     std::string c1 = input_converter_name(cname, 1);
-    std::string suff1 = "py_" + std::string{static_cast<std::string_view>(*pq1.suffix)};
+    std::string suff1 =
+      "py_" + (pq1.suffix ? std::string{static_cast<std::string_view>(*pq1.suffix)} : "");
     auto pq2 = input_queries[2];
     std::string c2 = input_converter_name(cname, 2);
-    std::string suff2 = "py_" + std::string{static_cast<std::string_view>(*pq2.suffix)};
+    std::string suff2 =
+      "py_" + (pq2.suffix ? std::string{static_cast<std::string_view>(*pq2.suffix)} : "");
     mod->ph_module->transform(pyname, *pyc, concurrency::serial)
       .input_family(
         product_query{.creator = identifier(c0), .layer = pq0.layer, .suffix = identifier(suff0)},
@@ -894,7 +905,8 @@ static PyObject* md_observe(py_phlex_module* mod, PyObject* args, PyObject* kwds
   // register Python observer
   auto pq0 = input_queries[0];
   std::string c0 = input_converter_name(cname, 0);
-  std::string suff0 = "py_" + std::string{static_cast<std::string_view>(*pq0.suffix)};
+  std::string suff0 =
+    "py_" + (pq0.suffix ? std::string{static_cast<std::string_view>(*pq0.suffix)} : "");
 
   switch (input_queries.size()) {
   case 1: {
@@ -908,7 +920,8 @@ static PyObject* md_observe(py_phlex_module* mod, PyObject* args, PyObject* kwds
     auto* pyc = new py_callback_2v{callable};
     auto pq1 = input_queries[1];
     std::string c1 = input_converter_name(cname, 1);
-    std::string suff1 = "py_" + std::string{static_cast<std::string_view>(*pq1.suffix)};
+    std::string suff1 =
+      "py_" + (pq1.suffix ? std::string{static_cast<std::string_view>(*pq1.suffix)} : "");
     mod->ph_module->observe(cname, *pyc, concurrency::serial)
       .input_family(
         product_query{.creator = identifier(c0), .layer = pq0.layer, .suffix = identifier(suff0)},
@@ -919,10 +932,12 @@ static PyObject* md_observe(py_phlex_module* mod, PyObject* args, PyObject* kwds
     auto* pyc = new py_callback_3v{callable};
     auto pq1 = input_queries[1];
     std::string c1 = input_converter_name(cname, 1);
-    std::string suff1 = "py_" + std::string{static_cast<std::string_view>(*pq1.suffix)};
+    std::string suff1 =
+      "py_" + (pq1.suffix ? std::string{static_cast<std::string_view>(*pq1.suffix)} : "");
     auto pq2 = input_queries[2];
     std::string c2 = input_converter_name(cname, 2);
-    std::string suff2 = "py_" + std::string{static_cast<std::string_view>(*pq2.suffix)};
+    std::string suff2 =
+      "py_" + (pq2.suffix ? std::string{static_cast<std::string_view>(*pq2.suffix)} : "");
     mod->ph_module->observe(cname, *pyc, concurrency::serial)
       .input_family(
         product_query{.creator = identifier(c0), .layer = pq0.layer, .suffix = identifier(suff0)},

test/python/CMakeLists.txt

@@ -177,6 +177,9 @@ target_link_libraries(cppsource4py PRIVATE phlex::module)
 add_test(NAME py:add COMMAND phlex::phlex -c ${CMAKE_CURRENT_SOURCE_DIR}/pyadd.jsonnet)
 list(APPEND ACTIVE_PY_CPHLEX_TESTS py:add)
 
+add_test(NAME py:suffix COMMAND phlex::phlex -c ${CMAKE_CURRENT_SOURCE_DIR}/pysuffix.jsonnet)
+list(APPEND ACTIVE_PY_CPHLEX_TESTS py:suffix)
+
 add_test(NAME py:config COMMAND phlex::phlex -c ${CMAKE_CURRENT_SOURCE_DIR}/pyconfig.jsonnet)
 list(APPEND ACTIVE_PY_CPHLEX_TESTS py:config)
 

test/python/pyadd.jsonnet

@@ -38,5 +38,15 @@
       ],
       sum_total: 1,
     },
+    pyverify_nosuff: {
+      py: 'verify',
+      input: [
+        {
+          creator: 'iadd',
+          layer: 'event',
+        },
+      ],
+      sum_total: 1,
+    },
   },
 }

test/python/pysuffix.jsonnet

@@ -0,0 +1,18 @@
+{
+  driver: {
+    cpp: 'generate_layers',
+    layers: {
+      event: { parent: 'job', total: 10, starting_number: 1 },
+    },
+  },
+  sources: {
+    provider: {
+      cpp: 'cppsource4py',
+    },
+  },
+  modules: {
+    pysuffix: {
+      py: 'suffix',
+    },
+  },
+}

test/python/suffix.py

@@ -0,0 +1,137 @@
+"""A bunch of observers taking different numbers of arguments.
+
+Simple observers to make sure that taking different numbers of
+inputs are thoroughly tested.
+"""
+
+
+
+def constant_one(i: int) -> int:
+    """Constant 1."""
+    return 1
+
+def constant_two(i: int) -> int:
+    """Constant 2."""
+    return 2
+
+def constant_three(i: int) -> int:
+    """Constant 3."""
+    return 3
+
+def constant_four(i: int) -> int:
+    """Constant 4."""
+    return 4
+
+def observe_one(i: int) -> None:
+    """Observe i; expect 1."""
+    assert i == 1
+
+def observe_two(i: int, j: int) -> None:
+    """Observe (i, j); expect (1, 2)."""
+    assert i == 1
+    assert j == 2
+
+def observe_three(i: int, j: int, k: int) -> None:
+    """Observe (i, j, k); expect (1, 2, 3)."""
+    assert i == 1
+    assert j == 2
+    assert k == 3
+
+def observe_four(i: int, j: int, k: int, ll: int) -> None:
+    """Observe (i, j, k, l); expect (1, 2, 3, 4)."""
+    assert i == 1
+    assert j == 2
+    assert k == 3
+    assert ll == 4
+
+
+def PHLEX_REGISTER_ALGORITHMS(m, config):
+    """Register some helpers and observers to test.
+
+    Use the standard Phlex `observe` registration to insert nodes in
+    the execution graph that receives values and verifies them.
+
+    Args:
+        m (internal): Phlex registrar representation.
+        config (internal): Phlex configuration representation.
+
+    Returns:
+        None
+    """
+    # tests for error handling of input specifications
+    for input_query in ({"creator": 42, "layer": "event", "suffix": "i"},
+                        {"creator": "input", "layer": 42, "suffix": "i"},
+                        {"layer": "event", "suffix": "i"},
+                        {"creator": "input", "suffix": "i"}):
+        try:
+            m.transform(constant_one, input_family=[input_query], output_products=["output_one"])
+            assert not "supposed to be here"
+        except TypeError as e:
+            # test for the one generic part in all these errors
+            assert "or not a string" in str(e)
+
+    # transforms with suffix to be used without suffix
+    m.transform(constant_one, input_family=[
+       {"creator": "input", "layer": "event", "suffix": "i"},
+       ],
+       output_products=["output_one"])
+    m.transform(constant_two, input_family=[
+       {"creator": "input", "layer": "event", "suffix": "i"},
+       ],
+       output_products=["output_two"])
+    m.transform(constant_three, input_family=[
+       {"creator": "input", "layer": "event", "suffix": "i"},
+       ],
+       output_products=["output_three"])
+
+    # observers without suffix
+
+    # test for failing suffix (incorrect type: not a string)
+    try:
+        m.observe(observe_one, input_family=[
+           {"creator": "constant_one", "layer": "event", "suffix": 42},
+           ])
+        assert not "supposed to be here"
+    except TypeError as e:
+        assert "is not a string" in str(e)
+        m.observe(observe_one, input_family=[
+           {"creator": "constant_one", "layer": "event"},
+           ])
+
+    # regular
+    m.observe(observe_two, input_family=[
+       {"creator": "constant_one", "layer": "event"},
+       {"creator": "constant_two", "layer": "event"},
+       ])
+    m.observe(observe_three, input_family=[
+       {"creator": "constant_one", "layer": "event"},
+       {"creator": "constant_two", "layer": "event"},
+       {"creator": "constant_three", "layer": "event"},
+       ])
+
+    # test for unsupported number of arguments (remove test once support
+    # to arbitrary number of arguments becomes available
+    try:
+        m.observe(observe_three, input_family=[
+           {"creator": "constant_one", "layer": "event"},
+           {"creator": "constant_two", "layer": "event"},
+           {"creator": "constant_three", "layer": "event"},
+           {"creator": "constant_four", "layer": "event"},
+           ])
+        assert not "supposed to be here"
+    except TypeError:
+        pass # noqa
+
+    # observers with suffix
+    m.observe(observe_one, name="observe_one_ws", input_family=[
+       {"creator": "constant_one", "layer": "event", "suffix": "output_one"},
+       ])
+    m.observe(observe_two, name="observe_two_ws", input_family=[
+       {"creator": "constant_one", "layer": "event", "suffix": "output_one"},
+       {"creator": "constant_two", "layer": "event", "suffix": "output_two"},
+       ])
+    m.observe(observe_three, name="observe_three_ws", input_family=[
+       {"creator": "constant_one", "layer": "event", "suffix": "output_one"},
+       {"creator": "constant_two", "layer": "event", "suffix": "output_two"},
+       {"creator": "constant_three", "layer": "event", "suffix": "output_three"},
+       ])