-
Notifications
You must be signed in to change notification settings - Fork 10
maintenance/improve test coverage #213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
greenc-FNAL
wants to merge
57
commits into
Framework-R-D:main
Choose a base branch
from
greenc-FNAL:maintenance/improve-test-coverage
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
57 commits
Select commit
Hold shift + click to select a range
74f90b4
Improve test coverage of Python/C++ interface code
greenc-FNAL 7084b3e
Initial plan
Copilot 27c595b
Add Variant helper and address review comments
Copilot 2603252
Initial plan
Copilot 553529e
Fix code review comments
Copilot a2c1188
Fix ruff F722 and mypy errors in vectypes.py by using type aliases wi…
Copilot 70f9a04
Apply cmake-format fixes
github-actions[bot] 81ad563
Simplify metaclass implementation per code review feedback
Copilot dce3e7a
Apply Python linting fixes
github-actions[bot] 1f2b01c
Merge pull request #7 from greenc-FNAL/copilot/update-ruff-and-mypy-s…
greenc-FNAL 55313c7
Fix CodeQL alert
greenc-FNAL bf616d8
Apply clang-format fixes
github-actions[bot] 440c902
Fix Python tests and enforce NumPy requirement
greenc-FNAL 38a38fe
Apply cmake-format fixes
github-actions[bot] 690cb26
More tests to fill gaps
greenc-FNAL 1354192
Apply cmake-format fixes
github-actions[bot] 6b1f634
Apply Python linting fixes
github-actions[bot] b0b30b1
Address remaining `ruff` issues
greenc-FNAL 3c3043c
Per Gemini 3 Pro, get GIL when updating ref count
greenc-FNAL 90d2593
Attempt to address CI hangs in `py:badbool` and `py:raise` tests
greenc-FNAL ed5e23e
More coverage improvement
greenc-FNAL b1c55bf
Apply Python linting fixes
github-actions[bot] f8a3c8d
Apply cmake-format fixes
github-actions[bot] a9d5d70
Silence inapposite complaints; remove unused class
greenc-FNAL 3aeb5c5
More hang protection
greenc-FNAL 4ea5f61
Extra diagnostics to debug hangs during testing
greenc-FNAL 267b363
More debug logging
greenc-FNAL cb20f61
Remove `failing_test_wrap.sh` as unnecessary
greenc-FNAL 19e1941
Replace unsafe macro call with safe equivalent
greenc-FNAL 0bb0035
Remove all diagnostics to see if problems return
greenc-FNAL fc74e2c
Remove diagnostic deadends and other unneeded code
greenc-FNAL 9acd7c2
Apply clang-format fixes
github-actions[bot] 56d49ba
Apply cmake-format fixes
github-actions[bot] fa86b34
Armor-plate `WILL_FAIL` tests against false pass
greenc-FNAL 2cfaf5f
Remove possibly-problematic initialization check
greenc-FNAL 175adde
Apply cmake-format fixes
github-actions[bot] e406022
Further attempts to prevent stalls
greenc-FNAL 7ca7590
Remove diagnostic invocations from coverage workflow
greenc-FNAL 915a130
Encourage `ctest --test-timeout` to limit impact of stalling tests
greenc-FNAL 3238ce4
First pass at addressing review comments
greenc-FNAL c084a29
Restore array-bounds warning deactivation for GCC 15
greenc-FNAL 81d981a
Improve Python argument ordering stability
greenc-FNAL 756b1ac
Apply clang-format fixes
github-actions[bot] 7478c82
Make sure types agree with what's in vectypes.py (#10)
knoepfel 1358195
Apply cmake-format fixes
github-actions[bot] b286b63
Revert unwanted change per review
greenc-FNAL 66e5cec
Have CMake report module check results
greenc-FNAL bb27d97
Python AdjustAnnotations class improvements
greenc-FNAL 24c1ddc
Apply cmake-format fixes
github-actions[bot] 978ce88
Include Python files in coverage change detection
greenc-FNAL 73ee50a
Make sure non-test Python code is tested
greenc-FNAL 25e4b45
Apply Python linting fixes
github-actions[bot] 2ef9438
Apply cmake-format fixes
github-actions[bot] 4cd0c00
Address `ruff` issues
greenc-FNAL ec89fc4
Resolve issues with Python testing and coverage
greenc-FNAL 67802e9
Enable FORM by default in presets
greenc-FNAL 772cac3
Temporarily restore packaging workaround pending reconciliation
greenc-FNAL File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| # 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| """Phlex Python Utilities. | ||
|
|
||
| Call helpers and type annotation tools for the Phlex framework. | ||
| """ | ||
|
|
||
| import copy | ||
| from typing import Any, Callable | ||
|
|
||
|
|
||
| class AdjustAnnotations: | ||
| """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 = AdjustAnnotations(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 | ||
|
|
||
| # Expose __code__ from the underlying callable if available, to aid | ||
| # introspection (e.g. by C++ modulewrap). | ||
| self.__code__ = getattr(self.phlex_callable, "__code__", None) | ||
| self.__defaults__ = getattr(self.phlex_callable, "__defaults__", None) | ||
| self.__kwdefaults__ = getattr(self.phlex_callable, "__kwdefaults__", None) | ||
|
|
||
| def __call__(self, *args, **kwargs): | ||
| """Raises an error if called directly. | ||
|
|
||
| AdjustAnnotations 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"AdjustAnnotations '{self.__name__}' was called directly. " | ||
| f"The framework should extract phlex_callable instead." | ||
| ) | ||
| return self.phlex_callable(*args, **kwargs) # type: ignore |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks scary. I don't claim that it's wrong. But do we understand why these changes are required? |
||
| } | ||
|
|
||
| // clang-format off | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't some of these changes suggest the build directory may be at
/? Is this ever the case?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It pegs the ignore expression to the top-level directory, otherwise it matches in subdirectories also.