Improve test coverage for Python support #213

knoepfel · 2026-01-15T16:47:22Z

Don't some of these changes suggest the build directory may be at /? Is this ever the case?

It pegs the ignore expression to the top-level directory, otherwise it matches in subdirectories also.

-Original file line number
+Diff line change
@@ Expand Up / @@ -76,7 +76,9 @@ runs: @@
               if git push origin HEAD:${{ inputs.pr-info-ref }}; then
                 echo "Push successful on attempt $i."
                 COMMIT_SHA=$(git rev-parse HEAD)
+                COMMIT_SHA_SHORT=$(git rev-parse --short HEAD)
                 echo "commit_sha=$COMMIT_SHA" >> "$GITHUB_OUTPUT"
+                echo "commit_sha_short=$COMMIT_SHA_SHORT" >> "$GITHUB_OUTPUT"
                 echo "pushed=true" >> "$GITHUB_OUTPUT"
                 exit 0
               fi
@@ Expand All / @@ -100,7 +102,7 @@ runs: @@
           uses: thollander/actions-comment-pull-request@24bffb9b452ba05a4f3f77933840a6a841d1b32b # v3.0.1
           with:
             message: |
-              Automatic ${{ inputs.tool }} fixes pushed (commit ${{ steps.commit_and_push.outputs.commit_sha }}).
+              Automatic ${{ inputs.tool }} fixes pushed (commit ${{ steps.commit_and_push.outputs.commit_sha_short || steps.commit_and_push.outputs.commit_sha }}).
               ⚠️ **Note:** Some issues may require manual review and fixing.
         - name: Create patch
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     - **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**:
+. Run tests with `LLVM_PROFILE_FILE` set (e.g., `export LLVM_PROFILE_FILE="profraw/%m-%p.profraw"`).
+. Merge profiles: `llvm-profdata merge -sparse profraw/*.profraw -o coverage.profdata`.
+. 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`.

-Original file line number
+Diff line change
@@ Expand Up / @@ -62,6 +62,7 @@ jobs: @@
             file-type: |
               cpp
               cmake
+              python
         - name: Report detection outcome
           run: |
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -43,7 +43,7 @@ jobs: @@
           (
             github.event_name == 'issue_comment' &&
             github.event.issue.pull_request &&
-            contains(fromJSON('["OWNER", "COLLABORATOR"]'), github.event.comment.author_association) &&
+            contains(fromJSON('["OWNER", "COLLABORATOR", "MEMBER"]'), github.event.comment.author_association) &&
             (
               startsWith(github.event.comment.body, format('@{0}bot format', github.event.repository.name)) ||
               startsWith(github.event.comment.body, format('@{0}bot jsonnet-format-fix', github.event.repository.name))
@@ Expand All / @@ -64,9 +64,6 @@ jobs: @@
         name: Apply Jsonnet formatting
         needs: pre-check
         if: needs.pre-check.result == 'success'
-        container:
-          image: public.ecr.aws/bitnami/jsonnet:latest
-          options: --user root
         steps:
           - name: Checkout code
@@ Expand All / @@ -79,9 +76,11 @@ jobs: @@
           - name: Apply Jsonnet formatting
             id: lint
-            working-directory: ${{ env.local_checkout_path }}
+            env:
+              CHECKOUT_PATH: ${{ env.local_checkout_path }}
             run: |
-              find . \( -name "*.jsonnet" -o -name "*.libsonnet" \) -print0 | xargs -0 -r jsonnetfmt -i
+              docker run --rm -v "${{ github.workspace }}:/work" -w /work --user root public.ecr.aws/bitnami/jsonnet:latest \
+                sh -c "find \"$CHECKOUT_PATH\" \( -name '*.jsonnet' -o -name '*.libsonnet' \) -print0 | xargs -0 -r jsonnetfmt -i"
             continue-on-error: true
           - name: Handle fix commit
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -43,6 +43,7 @@ jobs: @@
           (
             github.event_name == 'issue_comment' &&
             github.event.issue.pull_request &&
+            contains(fromJSON('["OWNER", "COLLABORATOR", "MEMBER"]'), github.event.comment.author_association) &&
             (
               startsWith(github.event.comment.body, '@phlexbot format') ||
               startsWith(github.event.comment.body, '@phlexbot markdown-fix') ||
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve test coverage for Python support #213

Uh oh!

Diff view

Diff view

There are no files selected for viewing

knoepfel Jan 15, 2026

Uh oh!

greenc-FNAL Jan 15, 2026

Uh oh!

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ -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
@@ Expand Down Expand Up / @@ -45,4 +49,4 @@ __pycache__/ @@
     .DS_Store
     # act (local workflow testing)
     .act-artifacts/
-    .secrets
+    .secrets

-Original file line number
+Diff line change
@@ Expand Up / @@ -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)
@@ Expand All / @@ -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()
@@ Expand Down Expand Up / @@ -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()
@@ Expand All / @@ -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()
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -4,6 +4,7 @@ @@
                 "name": "default",
                 "hidden": false,
                 "cacheVariables": {
+                    "PHLEX_USE_FORM": "ON",
                     "CMAKE_EXPORT_COMPILE_COMMANDS": "YES",
                     "CMAKE_CXX_STANDARD": "20",
                     "CMAKE_CXX_STANDARD_REQUIRED": "YES",
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -28,6 +28,10 @@ namespace phlex::experimental { @@
                               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;
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -73,6 +73,10 @@ namespace phlex::experimental { @@
               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);
           }
         }
@@ Expand All / @@ -93,6 +97,9 @@ namespace phlex::experimental { @@
         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());
           }
         }
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     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
+    )

-Original file line number
+Diff line change
@@ -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:
+. **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.
+. **C++ Driver**: The executable that:
+        - Parses the Jsonnet configuration.
+        - Initializes the Phlex core.
+        - Loads the Python interpreter and the specified plugin.
+. **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
+. **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.
+. **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.

Improve test coverage for Python support #213

Are you sure you want to change the base?

Uh oh!

Improve test coverage for Python support #213

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

knoepfel Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

greenc-FNAL Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!