Integrate Variant helper from PR #245 for type-specific Python algorithm registration#6
Conversation
- Added variant.py helper from PR Framework-R-D#245 - Modified modulewrap.cpp to recognize Variant wrapper via phlex_callable - Updated adder.py to use Variant helper for type-specific registration - Removed debug print statements from verify_extended.py - Removed commented-out mutex code from modulewrap.cpp - Removed debug message() calls from CMakeLists.txt - Fixed LaTeX syntax in copilot-instructions.md (use Unicode ↔) Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Fixed docstring in adder.py to reference 'Addable' instead of 'Number' - Fixed error message in variant.py to use correct class name 'Variant' - Added clarifying comments in modulewrap.cpp about reference counting Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR integrates the Variant helper from PR Framework-R-D#245 to enable type-specific Python algorithm registration while addressing outstanding code cleanup items from PR Framework-R-D#213 review comments.
Changes:
- Adds new
test/python/variant.pyproviding a wrapper class for registering generic Python algorithms with specific C++ type annotations - Modifies
plugins/python/src/modulewrap.cppto detect and unwrap Variant instances via thephlex_callableattribute - Updates
test/python/adder.pyto demonstrate Variant usage with a generic add function registered as int-specificiadd - Removes debug print statements, unused imports, and commented-out mutex code throughout the codebase
- Replaces LaTeX symbols with Unicode in documentation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/python/variant.py |
New wrapper class providing custom __annotations__ and __name__ for type-specific registration |
test/python/adder.py |
Demonstrates Variant usage by converting generic add() to int-specific iadd |
plugins/python/src/modulewrap.cpp |
Adds Variant unwrapping logic in parse_args() and removes commented-out mutex code |
test/python/verify_extended.py |
Removes debug print statements and unused sys import |
test/python/CMakeLists.txt |
Removes debug message() calls |
.github/copilot-instructions.md |
Replaces LaTeX $\leftrightarrow$ with Unicode ↔ |
| f"Variant '{self.__name__}' was called directly. " | ||
| f"The framework should extract phlex_callable instead." | ||
| ) | ||
| return self.phlex_callable(*args, **kwargs) # type: ignore |
There was a problem hiding this comment.
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.
| >>> def add(i: Number, j: Number) -> Number: | ||
| ... return i + j | ||
| ... | ||
| >>> int_adder = variant(add, {"i": int, "j": int, "return": int}, "iadd") |
There was a problem hiding this comment.
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.
| >>> int_adder = variant(add, {"i": int, "j": int, "return": int}, "iadd") | |
| >>> int_adder = Variant(add, {"i": int, "j": int, "return": int}, "iadd") |
|
|
||
|
|
||
| class AddableProtocol[T](Protocol): | ||
| """Typer bound for any types that can be added.""" |
There was a problem hiding this comment.
Typo in docstring: "Typer bound" should be "Type bound".
| """Typer bound for any types that can be added.""" | |
| """Type bound for any types that can be added.""" |
| 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 | ||
| 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 |
There was a problem hiding this comment.
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.
885a947
into
maintenance/improve-test-coverage
|
@copilot I have merged this PR into Framework-R-D#213, and I'm currently hitting Python linter issues. Please analyze the latest state of Framework-R-D#213 and address the reported |
PR Framework-R-D#245 introduced a
Varianthelper to register generic Python algorithms with specific C++ type annotations. This PR integrates that functionality into the Python/C++ interface improvements from PR Framework-R-D#213.Changes
Core Integration
test/python/variant.py- Wrapper class providing custom__annotations__and__name__for type-specific registrationplugins/python/src/modulewrap.cpp- Detect and unwrap Variant viaphlex_callableattribute inparse_args()test/python/adder.py- Demonstrate usage with genericadd()function registered as int-specificiaddCode Cleanup (Outstanding PR Framework-R-D#213 Review Comments)
modulewrap.cpp(GIL already provides thread safety)verify_extended.pymessage()calls fromCMakeLists.txt$\leftrightarrow$with Unicode↔incopilot-instructions.mdExample Usage
The C++ side extracts
phlex_callablewhen present, enabling the same Python function to be registered multiple times with different type signatures.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.