-
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
56
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
56 commits
Select commit
Hold shift + click to select a range
e64bb97
Improve test coverage of Python/C++ interface code
greenc-FNAL 4e0957f
Initial plan
Copilot d3bd321
Add Variant helper and address review comments
Copilot 26fa647
Fix code review comments
Copilot 1abca07
Apply cmake-format fixes
github-actions[bot] ba1377a
Apply Python linting fixes
github-actions[bot] e644d7c
Initial plan
Copilot e2cb860
Fix ruff F722 and mypy errors in vectypes.py by using type aliases wi…
Copilot 49739fe
Simplify metaclass implementation per code review feedback
Copilot 7d71301
Fix CodeQL alert
greenc-FNAL adbeaea
Apply clang-format fixes
github-actions[bot] f7e4a81
Fix Python tests and enforce NumPy requirement
greenc-FNAL ce1c0ac
Apply cmake-format fixes
github-actions[bot] c6db4ac
More tests to fill gaps
greenc-FNAL 1ce1292
Apply cmake-format fixes
github-actions[bot] 52fa79b
Apply Python linting fixes
github-actions[bot] fe5971a
Address remaining `ruff` issues
greenc-FNAL 12a20b5
Per Gemini 3 Pro, get GIL when updating ref count
greenc-FNAL 93d4528
Attempt to address CI hangs in `py:badbool` and `py:raise` tests
greenc-FNAL ae6f729
More coverage improvement
greenc-FNAL 36aa482
Apply Python linting fixes
github-actions[bot] c6914d7
Apply cmake-format fixes
github-actions[bot] b5a5c4b
Silence inapposite complaints; remove unused class
greenc-FNAL 9d1ffc5
More hang protection
greenc-FNAL 8112bc6
Extra diagnostics to debug hangs during testing
greenc-FNAL 04ec9f8
More debug logging
greenc-FNAL b231116
Remove `failing_test_wrap.sh` as unnecessary
greenc-FNAL 63c06bf
Replace unsafe macro call with safe equivalent
greenc-FNAL a14544d
Remove all diagnostics to see if problems return
greenc-FNAL 30cda8e
Remove diagnostic deadends and other unneeded code
greenc-FNAL cb3017a
Apply clang-format fixes
github-actions[bot] f8346fd
Apply cmake-format fixes
github-actions[bot] d72842b
Armor-plate `WILL_FAIL` tests against false pass
greenc-FNAL 0cf0b27
Remove possibly-problematic initialization check
greenc-FNAL cc5aa40
Apply cmake-format fixes
github-actions[bot] e8b0bf9
Further attempts to prevent stalls
greenc-FNAL e5d1508
Remove diagnostic invocations from coverage workflow
greenc-FNAL 6461df8
Encourage `ctest --test-timeout` to limit impact of stalling tests
greenc-FNAL 2839756
First pass at addressing review comments
greenc-FNAL 25ad760
Restore array-bounds warning deactivation for GCC 15
greenc-FNAL 039db91
Improve Python argument ordering stability
greenc-FNAL d26a3ce
Apply clang-format fixes
github-actions[bot] ef6ef47
Make sure types agree with what's in vectypes.py (#10)
knoepfel dad36de
Apply cmake-format fixes
github-actions[bot] 1931812
Revert unwanted change per review
greenc-FNAL 87a8983
Have CMake report module check results
greenc-FNAL 5cc2d6d
Python AdjustAnnotations class improvements
greenc-FNAL 004e6aa
Apply cmake-format fixes
github-actions[bot] 630e126
Include Python files in coverage change detection
greenc-FNAL 5f36485
Make sure non-test Python code is tested
greenc-FNAL af9c2a0
Apply Python linting fixes
github-actions[bot] 0edd4d2
Apply cmake-format fixes
github-actions[bot] fa4b955
Address `ruff` issues
greenc-FNAL a4c4009
Resolve issues with Python testing and coverage
greenc-FNAL 11d811e
Enable FORM by default in presets
greenc-FNAL 6487281
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.