Initial Numba support#626
Open
wlav wants to merge 18 commits into
Open
Conversation
Contributor
✅ 7 CodeQL alerts resolved compared to main
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 ❌ Your patch check has failed because the patch coverage (58.64%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 82.66% 81.33% -1.34%
==========================================
Files 161 165 +4
Lines 5920 6016 +96
Branches 681 703 +22
==========================================
- Hits 4894 4893 -1
- Misses 806 885 +79
- Partials 220 238 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 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 |
Contributor
Clang-Tidy Check ResultsFound 5765 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
beojan
reviewed
Jun 3, 2026
| } | ||
|
|
||
| template <size_t N, typename Cf> | ||
| static bool unroll_switch(size_t rt_size, Cf&& func) |
Contributor
There was a problem hiding this comment.
I think this function is essentially mp_with_index from MP11. Using that should hopefully shut up clang-tidy.
I wouldn't be surprised if the whole n_arg logic could be expressed somewhat concisely in MP11 but I'd have to think about it a bit more.
Contributor
Author
There was a problem hiding this comment.
Looks like it, but the fact remains that clang-tidy is wrong b/c it simply doesn't analyze the predicate deeply enough. Note that I considered removing the forward, but func being a lambda function itself, I'm not sure whether that's appropriate (it would be for just a function pointer) and anyway, then you get a different clang-tidy warning (can't have a move without a forward; it does still run fine, of course, since it's only used once).
Aside, the implementation of mp_with_index simply spells everything out up to an index of 16 using template specializations. :} Likely more efficient (not sure what the compiler makes of it), but also fugly (which is fine if hidden in the boost header; not so much if I take the same approach in modulewrap.cpp). It would be clang-tidy approved, however.
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
3 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.