Improve test coverage for Python support

.github/copilot-instructions.md

@@ -168,3 +168,60 @@ All Markdown files must strictly follow these markdownlint rules:
 - **MD034**: No bare URLs (for example, use a markdown link like `[text](destination)` instead of a plain URL)
 - **MD036**: Use # headings, not **Bold:** for titles
 - **MD040**: Always specify code block language (for example, use '```bash', '```python', '```text', etc.)
+
+## Development & Testing Workflows
+
+### Build and Test
+
+- **Environment**: Always source `setup-env.sh` before building or testing. This applies to all environments (Dev Container, local machine, HPC).
+- **Configuration**:
+  - **Presets**: Prefer `CMakePresets.json` workflows (e.g., `cmake --preset default`).
+  - **Generator**: Prefer `Ninja` over `Makefiles` when available (`-G Ninja`).
+- **Build**:
+  - **Parallelism**: Always use multiple cores. Ninja does this by default. For `make`, use `cmake --build build -j $(nproc)`.
+- **Test**:
+  - **Parallelism**: Run tests in parallel using `ctest -j $(nproc)` or `ctest --parallel <N>`.
+  - **Selection**: Run specific tests with `ctest -R "regex"` (e.g., `ctest -R "py:*"`).
+  - **Debugging**: Use `ctest --output-on-failure` to see logs for failed tests.
+  - **Guard against known or suspected stalling tests**: Use `ctest --test-timeout` to set the per-test time limit (e.g. `90`) for 90s, *vs* the default of 1500s.
+
+### Python Integration
+
+- **Naming**: Avoid naming Python test scripts `types.py` or other names that shadow standard library modules. This causes obscure import errors (e.g., `ModuleNotFoundError: No module named 'numpy'`).
+- **PYTHONPATH**: Only include paths that contain user Python modules loaded by Phlex (for example, the source directory and any build output directory that houses generated modules). Do not append system/Spack/venv `site-packages`; `pymodule.cpp` handles CMAKE_PREFIX_PATH and virtual-environment path adjustments.
+- **Test Structure**:
+  - **C++ Driver**: Provides data streams (e.g., `test/python/driver.cpp`).
+  - **Jsonnet Config**: Wires the graph (e.g., `test/python/pytypes.jsonnet`).
+  - **Python Script**: Implements algorithms (e.g., `test/python/test_types.py`).
+- **Type Conversion**: `plugins/python/src/modulewrap.cpp` handles C++ ↔ Python conversion.
+  - **Mechanism**: Uses substring matching on type names (for example, `"float64]]"`). This is brittle.
+  - **Requirement**: Ensure converters exist for all types used in tests (e.g., `float`, `double`, `unsigned int`, and their vector equivalents).
+  - **Warning**: Exact type matches are required. `numpy.float32` != `float`.
+
+### Coverage Analysis
+
+- **Tooling**: The project uses LLVM source-based coverage.
+- **Requirement**: The `phlex` binary must catch exceptions in `main` to ensure coverage data is flushed to disk even when tests fail/crash.
+- **Generation**:
+  - **CMake Targets**: `coverage-xml`, `coverage-html` (if configured).
+  - **Manual**:
+    1. Run tests with `LLVM_PROFILE_FILE` set (e.g., `export LLVM_PROFILE_FILE="profraw/%m-%p.profraw"`).
+    2. Merge profiles: `llvm-profdata merge -sparse profraw/*.profraw -o coverage.profdata`.
+    3. Generate report: `llvm-cov show -instr-profile=coverage.profdata -format=html ...`
+
+### Local GitHub Actions Testing (`act`)
+
+- **Tool**: Use `act` to run GitHub Actions workflows locally.
+- **Configuration**: Ensure `.actrc` exists in the workspace root with the following content to use a compatible runner image:
+
+  ```text
+  -P ubuntu-latest=catthehacker/ubuntu:act-latest
+  ```
+
+- **Usage**:
+  - List jobs: `act -l`
+  - Run specific job: `act -j <job_name>` (e.g., `act -j python-check`)
+  - Run specific event: `act pull_request`
+- **Troubleshooting**:
+  - **Docker Socket**: `act` requires access to the Docker socket. In dev containers, this may require specific mount configurations or permissions.
+  - **Artifacts**: `act` creates a `phlex-src` directory (or similar) for checkout. Ensure this is cleaned up or ignored by tools like `mypy`.

.gitignore

@@ -1,23 +1,27 @@
 # Build directories
 build/
+build-cov/
 _build/
 *.dir/
-phlex-src
-phlex-build/
-CMakeCache.txt
+/phlex-src/
+/phlex-build/
+/CMakeCache.txt
 CMakeFiles/
-_deps/
+/_deps/
 _codeql_detected_source_root
 
 # CMake user-specific presets (not generated by Spack)
-CMakeUserPresets.json
+/CMakeUserPresets.json
 
 # Coverage reports
-coverage.xml
-coverage.info
-coverage-html/
-.coverage-generated/
-.coverage-artifacts/
+/coverage.profdata
+/coverage_*.txt
+/coverage.xml
+/coverage.info
+/coverage-html/
+/profraw/
+/.coverage-generated/
+/.coverage-artifacts/
 *.gcda
 *.gcno
 *.gcov
@@ -45,5 +49,5 @@ __pycache__/
 .DS_Store
 # act (local workflow testing)
 .act-artifacts/
-.secretsactionlint
+.secrets
 actionlint

CMakeLists.txt

@@ -49,6 +49,11 @@ project(phlex VERSION 0.1.0 LANGUAGES CXX)
 cet_cmake_env()
 # ##############################################################################
 
+# Set CI/test timeouts to a conservative value to avoid long stalls in CI.
+# Use cache variables so generated CTest/Dart files pick this up when configured.
+set(DART_TESTING_TIMEOUT 90 CACHE STRING "Timeout (s) for Dart/CTest runs")
+set(CTEST_TEST_TIMEOUT 90 CACHE STRING "Per-test timeout (s) for CTest")
+
 # Make tools available
 FetchContent_MakeAvailable(Catch2 GSL mimicpp)
 
@@ -70,13 +75,13 @@ add_compile_options(
 )
 
 if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-  if(
-    CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "14.1"
-    AND CMAKE_COMPILER_VERSION VERSION_LESS "15"
-  )
-    # GCC 14.1 issues many false positives re. array-bounds and
-    # stringop-overflow
-    add_compile_options(-Wno-array-bounds -Wno-stringop-overflow)
+  if(CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "14.1")
+    if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "15")
+      add_compile_options(-Wno-stringop-overflow)
+    endif()
+    if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "16")
+      add_compile_options(-Wno-array-bounds)
+    endif()
   endif()
 endif()
 
@@ -108,7 +113,8 @@ if(ENABLE_TSAN)
       -g
       -O1
       # Ensure no optimizations interfere with TSan
-      "$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fno-omit-frame-pointer -fno-optimize-sibling-calls>"
+      "$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fno-omit-frame-pointer>"
+      "$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fno-optimize-sibling-calls>"
     )
     add_link_options(-fsanitize=thread)
   else()
@@ -130,7 +136,8 @@ if(ENABLE_ASAN)
       -g
       -O1
       # Ensure no optimizations interfere with ASan
-      "$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fno-omit-frame-pointer -fno-optimize-sibling-calls>"
+      "$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fno-omit-frame-pointer>"
+      "$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fno-optimize-sibling-calls>"
     )
     add_link_options(-fsanitize=address)
   else()

CMakePresets.json

@@ -4,6 +4,7 @@
             "name": "default",
             "hidden": false,
             "cacheVariables": {
+                "PHLEX_USE_FORM": "ON",
                 "CMAKE_EXPORT_COMPILE_COMMANDS": "YES",
                 "CMAKE_CXX_STANDARD": "23",
                 "CMAKE_CXX_STANDARD_REQUIRED": "YES",

phlex/core/edge_maker.cpp

@@ -28,6 +28,10 @@
                           provider.full_name(),
                           node_name,
                           port.product_label.to_string());
+            if (port.port == nullptr) {
+              throw std::runtime_error("Unexpected null port while connecting provider " +
+                                       provider.full_name() + " to node " + node_name);
+            }
             make_edge(provider.sender(), *(port.port));
             found_match = true;
             break;

phlex/core/edge_maker.hpp

@@ -73,6 +73,10 @@
           continue;
         }
 
+        if (producer->port == nullptr or receiver_port == nullptr) {
+          throw std::runtime_error("Unexpected null port while connecting " +
+                                   producer->node.full() + " to " + node_name);
+        }
         make_edge(*producer->port, *receiver_port);
       }
     }
@@ -93,6 +97,9 @@
     for (auto const& [output_name, output_node] : outputs) {
       make_edge(source, output_node->port());
       for (auto const& named_port : producers_.values()) {
+        if (named_port.to_output == nullptr) {
+          throw std::runtime_error("Unexpected null output port for " + named_port.node.full());
+        }
         make_edge(*named_port.to_output, output_node->port());
       }
     }

plugins/python/CMakeLists.txt

@@ -21,3 +21,8 @@ target_link_libraries(pymodule PRIVATE phlex::module Python::Python Python::NumP
 target_compile_definitions(pymodule PRIVATE NPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION)
 
 install(TARGETS pymodule LIBRARY DESTINATION lib)
+
+install(
+  DIRECTORY python/phlex
+  DESTINATION lib/python${Python_VERSION_MAJOR}.${Python_VERSION_MINOR}/site-packages
+)

plugins/python/README.md

@@ -0,0 +1,57 @@
+# Phlex Python Plugin Architecture
+
+This directory contains the C++ source code for the Phlex Python plugin, which enables Phlex to execute Python code as part of its computation graph.
+
+## Architecture Overview
+
+The integration is built on the **Python C API** (not `pybind11`) to maintain strict control over the interpreter lifecycle and memory management.
+
+### 1. The "Type Bridge" (`modulewrap.cpp`)
+
+The core of the integration is the type conversion layer in `src/modulewrap.cpp`. This layer is responsible for:
+
+- Converting Phlex `Product` objects (C++) into Python objects (e.g., `PyObject*`, `numpy.ndarray`).
+- Converting Python return values back into Phlex `Product` objects.
+
+**Critical Implementation Detail:**
+The type mapping relies on **string comparison** of type names.
+
+- **Mechanism**: The C++ code checks whether `type_name()` contains `"float64]]"` to identify a 2D array of doubles.
+- **Brittleness**: This is a fragile contract. If the type name changes (e.g., `numpy` changes its string representation) or if a user provides a slightly different type (e.g., `float` vs `np.float32`), the bridge may fail.
+- **Extension**: When adding support for new types, you must explicitly add converters in `modulewrap.cpp` for both scalar and vector/array versions.
+
+### 2. Hybrid Configuration
+
+Phlex uses a hybrid configuration model involving three languages:
+
+1. **Jsonnet** (`*.jsonnet`): Defines the computation graph structure. It specifies:
+    - The nodes in the graph.
+    - The Python module/class to load for specific nodes.
+    - Configuration parameters passed to the Python object.
+2. **C++ Driver**: The executable that:
+    - Parses the Jsonnet configuration.
+    - Initializes the Phlex core.
+    - Loads the Python interpreter and the specified plugin.
+3. **Python Code** (`*.py`): Implements the algorithmic logic.
+
+### 3. Environment & Testing
+
+Because the Python interpreter is embedded within the C++ application, the runtime environment is critical.
+
+- **PYTHONPATH**: Must be set correctly to include:
+  - The build directory (for generated modules).
+  - The source directory (for user scripts).
+  - Do not append system/Spack `site-packages`; `pymodule.cpp` adjusts `sys.path` based on `CMAKE_PREFIX_PATH` and active virtual environments.
+- **Naming Collisions**:
+  - **Warning**: Do not name test files `types.py`, `test.py`, `code.py`, or other names that shadow standard library modules.
+  - **Consequence**: Shadowing can cause obscure failures in internal libraries (e.g., `numpy` failing to import because it tries to import `types` from the standard library but gets your local file instead).
+
+## Development Guidelines
+
+1. **Adding New Types**:
+    - Update `src/modulewrap.cpp` to handle the new C++ type.
+    - Add a corresponding test case in `test/python/` to verify the round-trip conversion.
+2. **Testing**:
+    - Use `ctest` to run tests.
+    - Tests are integration tests: they run the full C++ application which loads the Python script.
+    - Debugging: Use `ctest --output-on-failure` to see Python exceptions.

plugins/python/python/phlex/__init__.py

@@ -0,0 +1,109 @@
+"""Annotation helper for C++ typing variants.
+
+Python algorithms are generic, like C++ templates, but the Phlex registration
+process requires a single unique signature. These helpers generate annotated
+functions for registration with the proper C++ types.
+"""
+
+import collections
+import copy
+import inspect
+from typing import Any, Callable
+
+
+class MissingAnnotation(Exception):
+    """Exception noting the missing of an argument in the provied annotations."""
+
+    def __init__(self, arg: str):
+        """Construct exception from the name of the argument without annotation."""
+        self.arg = arg
+
+    def __str__(self):
+        """Report the argument that is missing an annotation."""
+        return "argument '%s' is not annotated" % self.arg
+
+
+class Variant:
+    """Wrapper to associate custom annotations with a callable.
+
+    This class wraps a callable and provides custom ``__annotations__`` and
+    ``__name__`` attributes, allowing the same underlying function or callable
+    object to be registered multiple times with different type annotations.
+
+    By default, the provided callable is kept by reference, but can be cloned
+    (e.g. for callable instances) if requested.
+
+    Phlex will recognize the "phlex_callable" data member, allowing an unwrap
+    and thus saving an indirection. To detect performance degradation, the
+    wrapper is not callable by default.
+
+    Attributes:
+        phlex_callable (Callable): The underlying callable (public).
+        __annotations__ (dict): Type information of arguments and return product.
+        __name__ (str): The name associated with this variant.
+
+    Examples:
+        >>> def add(i: Number, j: Number) -> Number:
+        ...     return i + j
+        ...
+        >>> int_adder = Variant(add, {"i": int, "j": int, "return": int}, "iadd")
+    """
+
+    def __init__(
+        self,
+        f: Callable,
+        annotations: dict[str, str | type | Any],
+        name: str,
+        clone: bool | str = False,
+        allow_call: bool = False,
+    ):
+        """Annotate the callable F.
+
+        Args:
+            f (Callable): Annotable function.
+            annotations (dict): Type information of arguments and return product.
+            name (str): Name to assign to this variant.
+            clone (bool|str): If True (or "deep"), creates a shallow (deep) copy
+                of the callable.
+            allow_call (bool): Allow this wrapper to forward to the callable.
+        """
+        if clone == "deep":
+            self.phlex_callable = copy.deepcopy(f)
+        elif clone:
+            self.phlex_callable = copy.copy(f)
+        else:
+            self.phlex_callable = f
+
+        # annotions are expected as an ordinary dict and should be ordered, but
+        # we do not require it, so re-order based on the function's co_varnames
+        self.__annotations__ = collections.OrderedDict()
+
+        sig = inspect.signature(self.phlex_callable)
+        for k, v in sig.parameters.items():
+            try:
+                self.__annotations__[k] = annotations[k]
+            except KeyError as e:
+                if v.default is inspect.Parameter.empty:
+                    raise MissingAnnotation(k) from e
+
+        self.__annotations__["return"] = annotations.get("return", None)
+
+        self.__name__ = name
+        self.__code__ = getattr(self.phlex_callable, "__code__", None)
+        self.__defaults__ = getattr(self.phlex_callable, "__defaults__", None)
+        self._allow_call = allow_call
+
+    def __call__(self, *args, **kwargs):
+        """Raises an error if called directly.
+
+        Variant instances should not be called directly. The framework should
+        extract ``phlex_callable`` instead and call that.
+
+        Raises:
+            AssertionError: To indicate incorrect usage, unless overridden.
+        """
+        assert self._allow_call, (
+            f"Variant '{self.__name__}' was called directly. "
+            f"The framework should extract phlex_callable instead."
+        )
+        return self.phlex_callable(*args, **kwargs)  # type: ignore

plugins/python/src/configwrap.cpp

@@ -131,7 +131,7 @@ static PyObject* pcm_subscript(py_config_map* pycmap, PyObject* pykey)
       }
     }
   } catch (std::runtime_error const&) {
-    PyErr_Format(PyExc_TypeError, "property \"%s\" does not exist", ckey.c_str());
+    PyErr_Format(PyExc_KeyError, "property \"%s\" does not exist", ckey.c_str());
   }
 
   // cache if found

plugins/python/src/lifelinewrap.cpp

@@ -31,9 +31,14 @@ static int ll_clear(py_lifeline_t* pyobj)
 
 static void ll_dealloc(py_lifeline_t* pyobj)
 {
+  // This type participates in GC; untrack before clearing references so the
+  // collector does not traverse a partially torn-down object during dealloc.
+  PyObject_GC_UnTrack(pyobj);
   Py_CLEAR(pyobj->m_view);
   typedef std::shared_ptr<void> generic_shared_t;
   pyobj->m_source.~generic_shared_t();
+  // Use tp_free to pair with tp_alloc for GC-tracked Python objects.
+  Py_TYPE(pyobj)->tp_free((PyObject*)pyobj);
 }
 
 // clang-format off