Initial Numba support#626
Open
wlav wants to merge 31 commits into
Open
Conversation
Contributor
|
31 fixed, 0 new since branch point (cb81c8b) ✅ 31 CodeQL alerts resolved since the previous PR commit
✅ 31 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
Contributor
Clang-Tidy Check Results55 new issue(s) introduced by this patch (5842 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 83.27% 83.21% -0.06%
==========================================
Files 162 172 +10
Lines 5912 6725 +813
Branches 670 825 +155
==========================================
+ Hits 4923 5596 +673
- Misses 796 851 +55
- Partials 193 278 +85
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Contributor
Clang-Tidy Check Results3 new issue(s) introduced by this patch (5777 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Contributor
Clang-Tidy Check Results2 new issue(s) introduced by this patch (5769 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Contributor
Author
|
@phlexbot header-guards-fix |
Contributor
|
Automatic header-guards fixes pushed (commit ea7dff7). |
Contributor
Clang-Tidy Check Results2 new issue(s) introduced by this patch (5767 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
beojan
reviewed
Jun 3, 2026
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Numba JIT ChangesNumba JIT cfunc Integration
Sequence Diagram(s)sequenceDiagram
rect rgba(30, 120, 200, 0.5)
Note over Phlex,ffi: Python callback path
Phlex->>modulewrap: md_transform(callable, input_selectors, ...)
modulewrap->>modulewrap: parse_args → input_selectors, nconcur
modulewrap->>modulewrap: is_numba_cfunc(callable)?
end
alt Numba cfunc detected
rect rgba(180, 80, 30, 0.5)
Note over modulewrap,ffi: JIT dispatch path
modulewrap->>jit_callback_impl: register jit_callback<RetT, N>
jit_callback_impl->>dcarg: build dcargs_t from typed inputs
jit_callback_impl->>dyncall: dyncall(fn_ptr, result, args)
dyncall->>ffi: ffi_prep_cif + ffi_call
ffi-->>jit_callback_impl: result dcarg
end
else Python callable
rect rgba(30, 160, 80, 0.5)
Note over modulewrap,CPython: Python dispatch path
modulewrap->>py_callback_impl: register py_callback<RetT, N>
py_callback_impl->>CPython: lifeline_transform + PyObject_Call
py_callback_impl->>dyncall: dyncall for return type extraction
dyncall->>ffi: ffi_prep_cif + ffi_call
ffi-->>py_callback_impl: result dcarg
end
end
modulewrap->>Phlex: insert_input_converters + insert_output_converter (ispy flag)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/python/src/modulewrap.cpp (1)
283-313:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
Py_DECREF(coll)— leaks the fast sequence.
PySequence_Fastreturns a new reference that must be decref'd. The similar functionvalidate_output(line 345) properly doesPy_DECREF(coll), but this function doesn't. Classic copy-paste gremlin situation.🐛 Proposed fix
if (PyErr_Occurred()) cargs.clear(); // error handled through Python + Py_DECREF(coll); return cargs; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/python/src/modulewrap.cpp` around lines 283 - 313, The validate_input function is missing a Py_DECREF call for the coll object returned by PySequence_Fast. PySequence_Fast creates a new reference that must be explicitly freed before the function returns. Add Py_DECREF(coll) before the final return statement in validate_input to properly release the reference, matching the correct pattern used in the similar validate_output function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/python/CMakeLists.txt`:
- Around line 10-11: The CMakeLists.txt file has formatting that does not
conform to the project's CMake style standards, causing the cmake-format-check
CI step to fail. Run the gersemi formatter on the plugins/python/CMakeLists.txt
file to automatically reformat it according to the project's CMake formatting
rules. This will fix the formatting issues in the find_package and
pkg_check_modules calls and allow the CI pipeline to pass.
In `@plugins/python/src/dyncall.cpp`:
- Around line 15-36: The from_str method in phlex::experimental::dcarg is
missing handling for four integer types that are supported elsewhere. Add four
new else if conditions to the from_str function to handle the type strings
"int8_t", "uint8_t", "int16_t", and "uint16_t", each returning a dcarg object
initialized with a zero value cast to the appropriate type. These should be
inserted before the final throw statement, following the same pattern as the
existing type cases like int32_t and uint64_t.
In `@plugins/python/src/dyncall.hpp`:
- Around line 26-34: The existing comment in the conditional type aliasing block
(lines with ifdef __APPLE__ and the typedef declarations for ph_long_t and
ph_ulong_t) describes this as a "temporary workaround" but lacks clarity on what
needs to be addressed long-term. Enhance the comment to include a specific TODO
or FIXME note that explains this code should be revisited once a proper
cross-platform type translation layer is implemented between C++ and Python,
making it clear what the eventual solution should address and when this
temporary fix can be removed.
- Around line 38-82: The template method get<T>() in the dcarg struct is missing
const qualification, preventing it from being called on const dcarg objects or
const references. Add the const qualifier to the get<T>() method signature to
allow read-only access to the value from const dcarg instances.
In `@plugins/python/src/modulewrap.cpp`:
- Around line 300-306: The comment on line 305 contains a typo referencing
"validate_selection" instead of the actual function name being called. In the
section where validate_selector function is invoked (around line 300), correct
the comment to reference validate_selector instead of validate_selection to
ensure the documentation accurately reflects the function being used.
- Around line 45-50: Fix the typo in the comment above the MAX_SUPPORTED_ARGS
constant declaration. Change "compile-timee" to "compile-time" by removing the
extra 'e' at the end of the word.
- Around line 1110-1117: In the code block where is_numba_cfunc(callable) is
checked, the pyaddr variable obtained from PyObject_GetAttrString is never
decremented, causing a memory leak. After using pyaddr with
PyLong_AsVoidPtr(pyaddr), add a Py_DECREF(pyaddr) call to properly release the
Python object reference. Place the Py_DECREF after the PyLong_AsVoidPtr call but
ensure it executes in both code paths (whether ccallf is set or not).
- Around line 1003-1010: The PyObject_GetAttrString function returns a new
reference to a PyObject that must be decremented to avoid a memory leak. In the
code block where is_numba_cfunc is true, after obtaining the pyaddr reference
and extracting its void pointer value with PyLong_AsVoidPtr, add a
Py_DECREF(pyaddr) call to release the reference. This should be done at the end
of the if (pyaddr) block after the ccallf assignment is complete to ensure the
reference is always released when pyaddr is not null.
- Around line 424-450: The PyObject reference obtained from
PyObject_GetAttrString(callable, "_sig") at the beginning of this block is not
being decreffed, causing a memory leak. While the child attributes ret and args
are properly decreffed at lines 439-440, the parent sig object is missing its
corresponding Py_XDECREF call. Add Py_XDECREF(sig) after the entire if-else
block that checks for the "_sig" attribute and its alternatives (including the
else block that handles the __call__ method) has completed, to properly release
the reference.
- Around line 147-156: The move constructor and move assignment operator in
py_callback_base are currently using default implementations, which shallow-copy
the m_callable pointer without nulling the source. This creates a latent
double-decref bug that will manifest when the destructor is fixed to properly
decref m_callable. Replace the defaulted move constructor
py_callback_base(py_callback_base&&) and move assignment operator
operator=(py_callback_base&&) with custom implementations that copy the source's
m_callable to the destination, then set the source's m_callable to nullptr to
prevent double-decref issues during cleanup.
In `@test/python/jited.py`:
- Line 25: The function PHLEX_REGISTER_ALGORITHMS lacks type hints on its
parameters and return type, which violates coding guidelines. Add type
annotations to the function signature for both the m and config parameters
(using Any if specific Phlex types are unavailable) and add a return type
annotation of None to clarify the registration hook contract. Consider adding
from __future__ import annotations at the top of the file if not already
present.
- Line 30: Fix the spelling error in the docstring around line 30 where "ouput"
is misspelled. Locate the word "ouput" in the docstring that describes producing
a sum and correct it to "output".
- Line 12: Remove the unused import statement for Variant from phlex at the top
of the file. The import `from phlex import Variant` is not referenced anywhere
in the code and violates the project's linting standards (Ruff F401). Delete
this import line entirely to resolve the linting violation.
- Line 61: The input_family parameter line exceeds the 99-character limit at
approximately 103 characters. Break this line into multiple lines by either
splitting the dictionary definition across multiple lines with proper
indentation, or by extracting the dictionary construction into a separate
variable defined above the function call. Ensure the reformatted code maintains
proper indentation and readability while keeping each line under 99 characters.
In `@test/python/pyjited.jsonnet`:
- Around line 1-18: The pyjited.jsonnet file is not formatted according to
jsonnetfmt standards, which will cause CI to fail. Run the jsonnetfmt formatter
with the in-place flag (-i) on this file to automatically reformat it to comply
with the required coding guidelines.
---
Outside diff comments:
In `@plugins/python/src/modulewrap.cpp`:
- Around line 283-313: The validate_input function is missing a Py_DECREF call
for the coll object returned by PySequence_Fast. PySequence_Fast creates a new
reference that must be explicitly freed before the function returns. Add
Py_DECREF(coll) before the final return statement in validate_input to properly
release the reference, matching the correct pattern used in the similar
validate_output function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce466e49-b930-4eca-9b30-237f5f09d379
📒 Files selected for processing (8)
plugins/python/CMakeLists.txtplugins/python/python/phlex/_typing.pyplugins/python/src/dyncall.cppplugins/python/src/dyncall.hppplugins/python/src/modulewrap.cpptest/python/CMakeLists.txttest/python/jited.pytest/python/pyjited.jsonnet
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Framework-R-D/action-configure-cmake(auto-detected)Framework-R-D/action-workflow-setup(auto-detected)Framework-R-D/action-complete-pr-comment(auto-detected)Framework-R-D/action-handle-fix-commit(auto-detected)
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (gcc, none)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: Analyze python with CodeQL
- GitHub Check: coverage
- GitHub Check: clang-tidy-check
🧰 Additional context used
📓 Path-based instructions (7)
**/*.jsonnet
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
jsonnetfmtfor consistent Jsonnet formatting; CI enforces thisUse
jsonnetfmtformatting for Jsonnet files; CI enforces this
Files:
test/python/pyjited.jsonnet
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
plugins/python/src/dyncall.hppplugins/python/src/dyncall.cppplugins/python/src/modulewrap.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
plugins/python/src/dyncall.hppplugins/python/src/dyncall.cppplugins/python/src/modulewrap.cpp
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
plugins/python/src/dyncall.hpp
plugins/python/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
plugins/python/**/*.cpp: Usestd::runtime_errorfor C++ runtime failures; propagate Python exceptions viaPyErr_SetString/PyErr_Format; returnnullptron error; callPyErr_Clear()when recovering in Python/C++ integration
Use manualPy_INCREF/Py_DECREFfor reference counting andPyGILRAIIRAII wrapper for GIL management in C++ code that interacts with Python
For GC-tracked Python types in C++: usePy_TPFLAGS_HAVE_GC, implementtp_traverseandtp_clear, callPyObject_GC_UnTrackbefore deallocation
Files:
plugins/python/src/dyncall.cppplugins/python/src/modulewrap.cpp
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use ruff for Python formatting and linting (configured inpyproject.toml); follow Google-style docstring conventions; use line length of 99 characters; use double quotes for strings
Usefrom __future__ import annotationsto enable deferred evaluation of type annotations (avoids forward-reference issues; Python >=3.12)
Type hints are recommended; mypy is configured inpyproject.toml
Avoid naming Python test scriptstypes.pyor other names that shadow standard library modules, as this causes obscure import errors
Python test scripts should follow the test structure pattern: C++ driver provides data streams, Jsonnet config wires the graph, and Python script implements algorithms
**/*.py: Enforce 99-character line limit and double quotes in Python via ruff configured inpyproject.toml
Use Google-style docstrings in Python code
Use type hints in Python code and configure mypy for type checking
Usefrom __future__ import annotationsin Python to enable deferred evaluation of type annotations
Use PEP 8 naming in Python:CapWordsfor classes,snake_casefor everything else
When using thereadtool for Python files, always use integer values foroffsetandlimitparameters, never float/double values
Files:
test/python/jited.pyplugins/python/python/phlex/_typing.py
test/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Name Python test files as
test_*.py, do NOT name files after stdlib modules (e.g.,types.py)
Files:
test/python/jited.py
🪛 Cppcheck (2.21.0)
plugins/python/src/dyncall.cpp
[style] 15-15: The function 'from_str' is never used.
(unusedFunction)
[style] 96-96: The function 'dyncall' is never used.
(unusedFunction)
🪛 GitHub Actions: wlav checking CMake format / 0_cmake-format-check.txt
plugins/python/CMakeLists.txt
[error] 1-1: gersemi --check failed: the file would be reformatted.
🪛 GitHub Actions: wlav checking CMake format / cmake-format-check
plugins/python/CMakeLists.txt
[error] 1-1: gersemi --check failed: file would be reformatted (CMakeLists.txt not following required formatting). Run 'gersemi' (or 'gersemi --write' if supported) to apply formatting.
🪛 GitHub Actions: wlav checking Python code / 1_python-check.txt
test/python/jited.py
[error] 12-12: ruff check failed (F401): phlex.Variant imported but unused. Remove unused import: phlex.Variant.
🪛 GitHub Actions: wlav checking Python code / python-check
test/python/jited.py
[error] 12-12: ruff check reported F401: phlex.Variant imported but unused. Remove unused import: phlex.Variant.
🔇 Additional comments (27)
plugins/python/CMakeLists.txt (1)
14-26: LGTM!plugins/python/src/dyncall.hpp (2)
1-25: LGTM!
84-97: LGTM!plugins/python/src/dyncall.cpp (3)
38-50: LGTM!
52-94: LGTM!
96-129: LGTM!test/python/CMakeLists.txt (2)
182-187: LGTM!
289-302: LGTM!test/python/jited.py (2)
47-62: LGTM!
14-22: No changes needed—this inconsistency is intentional. Thecppsource4pyprovider generates ul1 and ul2 such that they always sum to 100 (ul1 = id.number() % 101,ul2 = 100ul - (id.number() % 101)), while all other type pairs are designed to sum to 1 (or 1.0). The expected result of100for uint64 is correct.plugins/python/python/phlex/_typing.py (4)
17-21: LGTM!
23-28: LGTM!
123-127: LGTM!
50-65: Not an issue;nb_types.float64andnb_types.doubleare identical in Numba.Numba's type system treats
float64anddoubleas the same underlying type (both 64-bit IEEE 754 floats). When the identity check runs (nb_types.float64 is nb_types.double), it returnsTrue—they're literally the same object. So the single mapping fornb_types.doublealready covers any code that referencesnb_types.float64, and there's zero risk of aKeyErrorwhen processingnp.float64inputs from cfunc signatures.> Likely an incorrect or invalid review comment.plugins/python/src/modulewrap.cpp (13)
108-115: LGTM!
162-209: LGTM!
211-236: LGTM!
246-281: LGTM!
357-380: LGTM!
558-604: LGTM!
605-703: LGTM!
724-728: LGTM!
732-836: LGTM!
849-911: LGTM!
913-965: LGTM!
967-984: LGTM!
1316-1323: LGTM!
Contributor
Clang-Tidy Check Results2 new issue(s) introduced by this patch (5522 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Contributor
Clang-Tidy Check ResultsFound 5522 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This PR adds "native" Numba support to phlex.
Numba was usable out-of-the-box already, through Python, but even if the Numba function itself didn't use the GIL, it still needed it at the start of every call (through the wrapper) and all Python/C++ converters required them, too. The implementation in this PR allows for calling the Numba JITed C-function directly, using converters that do not involve Python, and a dispatch through libffi. That sets up a workflow that doesn't require the GIL anywhere beyond configuration/registration time, thus enabling strong scaling (as far as Phlex is concerned) during graph execution.
This PR is for transforms only (observers and providers to follow) and only supports builtin types. Arrays and PODs (by member copy) should be doable as well, but Numba does not support arbitrary C++ classes.
The code in the module wrapper was significantly refactored, to maximize code reuse.
Note: it requires libffi, which will be available on most Linux boxes, but probably needs to be installed with spack in other places, so the spack recipe would need updating as well.
Build System
libffidependency viaPkgConfigdiscovery and validationsrc/dyncall.cpptopymodulelibrary sourcespymodulewithPkgConfig::FFIalongside existing dependencies (Phlex, Python, NumPy)Type System & Type Mappings
_PY2CPP) with Numba-specific scalar types (boolean, integer, float, void)"float32"→"float"and"float64"→"double"to handle Numba's distinction betweenFloatandfloat32typesnumba.core.typeswith fallback when Numba unavailableCore Feature Implementation (Numba JIT Support)
New libffi-based dynamic calling interface:
phlex::experimental::dcargstruct: type-erased argument container backed bystd::variantof supported primitive/FFI typesdcarg::from_str()string-to-typed factory for runtime type conversionphlex::experimental::dyncall()function that dispatches C functions via libffiffi_call, supporting both fixed and variadic signaturesRefactored module wrapper code (
modulewrap.cpp):py_callback/callvapproach with newpy_callback_base,py_callback_impl, andjit_callback_implimplementationsMAX_SUPPORTED_ARGSlimit (3 arguments)is_numba_cfunchelper to detect NumbaCFunctypes via attribute lookup__annotations__unavailablevalidate_selectorhelperBASIC_CONVERTER,VECTOR_CONVERTER,NUMPY_ARRAY_CONVERTER) refactored to operate ondcargvalues with explicit Python list handling for numpy conversionGraph wiring changes:
insert_converternow passes hardcoded concurrency level (16) totransformparse_argsextended to accept optionalconcurrencyparameter and produceinput_selectors(instead ofinput_queries)md_transformandmd_observerewritten to useunroll_switchhelper for callback registration with 1-3 inputssc_provideoutput validation switched tovalidate_selectorTesting
py:jited) in CMake configuration, active whennumbais availabletest/python/jited.py: Test registration module defining supported numeric type pairs and registering Numbacfuncalgorithms as transformation nodes with corresponding observer nodestest/python/pyjited.jsonnet: Configuration file for Numba test driver with layer generation, event layer, and Python module enablingCI & Security
.github/actions/prepare-check-outputs/action.yaml:get-pr-infoanddetect-act-env.github/actions/prepare-fix-outputs/action.yaml:get-pr-info.github/actions/run-change-detection/action.yaml:detect-relevant-changes.github/actions/workflow-setup/action.yaml:prepare-check-outputs,prepare-fix-outputs, andrun-change-detectionAPI Alignment