Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ All Markdown files must strictly follow these markdownlint rules:
- **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++ $\leftrightarrow$ Python conversion.
- **Type Conversion**: `plugins/python/src/modulewrap.cpp` handles C++ Python conversion.
- **Mechanism**: Uses string comparison of type names (e.g., `"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`.
Expand Down
26 changes: 11 additions & 15 deletions plugins/python/src/modulewrap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
#include <stdexcept>
#include <vector>

// static std::mutex g_py_mutex;

#ifdef PHLEX_HAVE_NUMPY
#define NO_IMPORT_ARRAY
#define PY_ARRAY_UNIQUE_SYMBOL phlex_ARRAY_API
Expand Down Expand Up @@ -111,7 +109,6 @@ namespace {
static_assert(sizeof...(Args) == N, "Argument count mismatch");

PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);

PyObject* result = PyObject_CallFunctionObjArgs(
(PyObject*)m_callable, lifeline_transform(args.get())..., nullptr);
Expand All @@ -134,7 +131,6 @@ namespace {
static_assert(sizeof...(Args) == N, "Argument count mismatch");

PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);

PyObject* result =
PyObject_CallFunctionObjArgs((PyObject*)m_callable, (PyObject*)args.get()..., nullptr);
Expand Down Expand Up @@ -372,7 +368,6 @@ namespace {
static PyObjectPtr vint_to_py(std::shared_ptr<std::vector<int>> const& v)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
if (!v)
return PyObjectPtr();
PyObject* list = PyList_New(v->size());
Expand All @@ -395,7 +390,6 @@ namespace {
static PyObjectPtr vuint_to_py(std::shared_ptr<std::vector<unsigned int>> const& v)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
if (!v)
return PyObjectPtr();
PyObject* list = PyList_New(v->size());
Expand All @@ -418,7 +412,6 @@ namespace {
static PyObjectPtr vlong_to_py(std::shared_ptr<std::vector<long>> const& v)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
if (!v)
return PyObjectPtr();
PyObject* list = PyList_New(v->size());
Expand All @@ -441,7 +434,6 @@ namespace {
static PyObjectPtr vulong_to_py(std::shared_ptr<std::vector<unsigned long>> const& v)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
if (!v)
return PyObjectPtr();
PyObject* list = PyList_New(v->size());
Expand Down Expand Up @@ -497,7 +489,6 @@ namespace {
static std::shared_ptr<std::vector<int>> py_to_vint(PyObjectPtr pyobj)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
auto vec = std::make_shared<std::vector<int>>();
PyObject* obj = pyobj.get();

Expand Down Expand Up @@ -537,7 +528,6 @@ namespace {
static std::shared_ptr<std::vector<unsigned int>> py_to_vuint(PyObjectPtr pyobj)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
auto vec = std::make_shared<std::vector<unsigned int>>();
PyObject* obj = pyobj.get();

Expand Down Expand Up @@ -577,7 +567,6 @@ namespace {
static std::shared_ptr<std::vector<long>> py_to_vlong(PyObjectPtr pyobj)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
auto vec = std::make_shared<std::vector<long>>();
PyObject* obj = pyobj.get();

Expand Down Expand Up @@ -617,7 +606,6 @@ namespace {
static std::shared_ptr<std::vector<unsigned long>> py_to_vulong(PyObjectPtr pyobj)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
auto vec = std::make_shared<std::vector<unsigned long>>();
PyObject* obj = pyobj.get();

Expand Down Expand Up @@ -657,7 +645,6 @@ namespace {
static std::shared_ptr<std::vector<float>> py_to_vfloat(PyObjectPtr pyobj)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
auto vec = std::make_shared<std::vector<float>>();
PyObject* obj = pyobj.get();

Expand Down Expand Up @@ -697,7 +684,6 @@ namespace {
static std::shared_ptr<std::vector<double>> py_to_vdouble(PyObjectPtr pyobj)
{
PyGILRAII gil;
// std::lock_guard<std::mutex> lock(g_py_mutex);
auto vec = std::make_shared<std::vector<double>>();
PyObject* obj = pyobj.get();

Expand Down Expand Up @@ -863,8 +849,18 @@ static PyObject* parse_args(PyObject* args,
return nullptr;
}

// special case of Phlex Variant wrapper
PyObject* wrapped_callable = PyObject_GetAttrString(callable, "phlex_callable");
if (wrapped_callable) {
// PyObject_GetAttrString returns a new reference, which we return
callable = wrapped_callable;
} else {
// No wrapper, use the original callable with incremented reference count
PyErr_Clear();
Py_INCREF(callable);
}

// no common errors detected; actual registration may have more checks
Py_INCREF(callable);
return callable;
}

Expand Down
3 changes: 0 additions & 3 deletions test/python/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,6 @@ except Exception:
)
list(APPEND ACTIVE_PY_CPHLEX_TESTS py:failure)

message(STATUS "Python_SITELIB: ${Python_SITELIB}")
message(STATUS "Python_SITEARCH: ${Python_SITEARCH}")
set(TEST_PYTHONPATH ${CMAKE_CURRENT_SOURCE_DIR})
# Always add site-packages to PYTHONPATH for tests, as embedded python might
# not find them especially in spack environments where they are in
Expand All @@ -154,7 +152,6 @@ except Exception:
# Keep this for backward compatibility or if it adds something else
endif()
set(TEST_PYTHONPATH ${TEST_PYTHONPATH}:$ENV{PYTHONPATH})
message(STATUS "TEST_PYTHONPATH: ${TEST_PYTHONPATH}")

set_tests_properties(
${ACTIVE_PY_CPHLEX_TESTS}
Expand Down
25 changes: 20 additions & 5 deletions test/python/adder.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,33 @@
real. It serves as a "Hello, World" equivalent for running Python code.
"""

from typing import Protocol, TypeVar

def add(i: int, j: int) -> int:
from variant import Variant


class AddableProtocol[T](Protocol):
"""Typer bound for any types that can be added."""
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in docstring: "Typer bound" should be "Type bound".

Suggested change
"""Typer bound for any types that can be added."""
"""Type bound for any types that can be added."""

Copilot uses AI. Check for mistakes.

def __add__(self, other: T) -> T: # noqa: D105
...


Addable = TypeVar('Addable', bound=AddableProtocol)


def add(i: Addable, j: Addable) -> Addable:
"""Add the inputs together and return the sum total.

Use the standard `+` operator to add the two inputs together
to arrive at their total.

Args:
i (int): First input.
j (int): Second input.
i (Addable): First input.
j (Addable): Second input.

Returns:
int: Sum of the two inputs.
Addable: Sum of the two inputs.

Examples:
>>> add(1, 2)
Expand All @@ -40,4 +54,5 @@ def PHLEX_REGISTER_ALGORITHMS(m, config):
Returns:
None
"""
m.transform(add, input_family=config["input"], output_products=config["output"])
int_adder = Variant(add, {"i": int, "j": int, "return": int}, "iadd")
m.transform(int_adder, input_family=config["input"], output_products=config["output"])
79 changes: 79 additions & 0 deletions test/python/variant.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
"""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 copy
from typing import Any, Callable


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")
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example code uses lowercase variant instead of the actual class name Variant. This would cause a NameError if someone tried to run this example.

Suggested change
>>> int_adder = variant(add, {"i": int, "j": int, "return": int}, "iadd")
>>> int_adder = Variant(add, {"i": int, "j": int, "return": int}, "iadd")

Copilot uses AI. Check for mistakes.
"""

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
self.__annotations__ = annotations
self.__name__ = name
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
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file ends with a trailing blank line (line 80). According to the project's text formatting standards, all text files must have their final line be non-empty and terminated with a single newline character, leaving no trailing blank lines. Remove the blank line at the end of the file.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +12 to +79
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new Variant class lacks test coverage. While adder.py demonstrates basic usage, there are no tests verifying:

  • That phlex_callable is correctly extracted and used by the framework
  • That the call method raises AssertionError when called directly
  • That clone functionality (shallow and deep copy) works correctly
  • That allow_call parameter works when set to True

Consider adding dedicated test cases for the Variant class to ensure the wrapper behaves correctly in various scenarios.

Copilot uses AI. Check for mistakes.
7 changes: 0 additions & 7 deletions test/python/verify_extended.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
"""Observers to check for various types in tests."""

import sys


class VerifierInt:
"""Verify int values."""
Expand Down Expand Up @@ -42,7 +40,6 @@ def __init__(self, sum_total: int):

def __call__(self, value: "long") -> None: # type: ignore # noqa: F821
"""Check if value matches expected sum."""
print(f"VerifierLong: value={value}, expected={self._sum_total}")
assert value == self._sum_total


Expand All @@ -57,7 +54,6 @@ def __init__(self, sum_total: int):

def __call__(self, value: "unsigned long") -> None: # type: ignore # noqa: F722
"""Check if value matches expected sum."""
print(f"VerifierULong: value={value}, expected={self._sum_total}")
assert value == self._sum_total


Expand All @@ -72,7 +68,6 @@ def __init__(self, sum_total: float):

def __call__(self, value: "float") -> None:
"""Check if value matches expected sum."""
sys.stderr.write(f"VerifierFloat: value={value}, expected={self._sum_total}\n")
assert abs(value - self._sum_total) < 1e-5


Expand All @@ -87,7 +82,6 @@ def __init__(self, sum_total: float):

def __call__(self, value: "double") -> None: # type: ignore # noqa: F821
"""Check if value matches expected sum."""
print(f"VerifierDouble: value={value}, expected={self._sum_total}")
assert abs(value - self._sum_total) < 1e-5


Expand All @@ -102,7 +96,6 @@ def __init__(self, expected: bool):

def __call__(self, value: bool) -> None:
"""Check if value matches expected."""
print(f"VerifierBool: value={value}, expected={self._expected}")
assert value == self._expected


Expand Down
Loading