-
Notifications
You must be signed in to change notification settings - Fork 199
[WIP] Support passing package name as pythran signature #2371
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
base: master
Are you sure you want to change the base?
Conversation
|
@ev-br : This is a work-in progress, can you have a look at the commit message and confirm it matches your needs? |
c9b1075 to
9bc3cdf
Compare
CodSpeed Performance ReportMerging #2371 will not alter performanceComparing Summary
|
9bc3cdf to
e7e4918
Compare
|
Yes, it looks just like what a doctor's ordered! The |
e7e4918 to
d9e3913
Compare
^^! my bad I meant |
|
@ev-br btw, pythran will still fail in the following case: Would that be an issue in you scenario? I guess that's something I can fix, but it won't be trivial |
I'm not entirely sure how pythran deals with helper functions (what must be annotated and what is inferred automagically), but AFAIU, yes, something like this will be a blocker. The target I'm after is to pythranize https://github.com/scipy/scipy/blob/main/scipy/interpolate/_rbfinterp_xp.py#L213C1-L215C3 The meat of the matter is this: where Also, does this part of the commit message mean there's a runtime check of the
|
Nothing needs to be annotated for non exported functions. I'll think of a way to deal with that.
Yes! |
d9e3913 to
08c5c4d
Compare
a8997b6 to
490caf6
Compare
a4bc83a to
fcd10e4
Compare
|
@ev-br : updated approach, still work in progress, but should be fairly stable now. It works on your reproducer with a slight |
2fb26de to
261d434
Compare
|
Great! Tested it locally --- just two glitches:
Ah, this numpy 2.0 / array api spelling would actually be nice to support (I know I keep and keep asking things...). Split it off to #2379 for convenience. |
261d434 to
70d1b66
Compare
|
Patch rebased on #2379 and updated, it now compiles your original source ! |
|
@ev-br can you confirm the last iteration is functional? |
|
Great! Testing it now..... It builds! I'm now testing it in https://github.com/scipy/scipy/compare/main...ev-br:rbf_xp_dedupe?expand=1 A first hiccup: What happens here: |
32d2205 to
01b7582
Compare
|
Can you give it a try with the latest version of the branch and using |
01b7582 to
a971d4c
Compare
|
Erm... Something's amiss with the last version: If I delete the offending line (which is a comment but OK), I get the same error on the next line. The file is from this branch: https://github.com/scipy/scipy/compare/main...ev-br:rbf_xp_dedupe?expand=1 |
|
Also, should it be (Pdb) p xp.name The latter definitely starts looking a bit strange for hardcoding a filesystem path into the source. EDIT: all in all, I'd question the value of the |
1065ebe to
2201042
Compare
|
Patch updated, there were quite a few flaws. Thanks for the quick feedback! |
|
Thank you, will test-drive it! ETA likely a couple of days though (such as is the time of the year) |
2201042 to
d8dee70
Compare
|
With d8dee70, I get a bizzare Also, I think I can now work around |
d8dee70 to
278a39e
Compare
|
On Mon, Dec 15, 2025 at 04:06:25AM -0800, Evgeni Burovski wrote:
Also, I think I can now work around array_api_compat.numpy vs pristine numpy,
so if it continues to be a brittle pain in the neck, we can roll back to it
being just numpy (sorry for asking to a wild goose chase).
Should be good now (?)
|
Confirmed, thanks! With 278a39e scipy builds again (and fails all around, but that's on me for now at least). I'll keep experimenting on the user side. |
|
Great! I leave that PR open for a while, just in case we need to adjust based on
your feedback.
|
|
I seem to be missing something simple: I should be able to compile |
|
Oh, I see the same error with so it's not specific to this branch. Now I'm seriously confused --- how come it compiles code which promemently features EDIT: since it seems to be a pre-existing issue / a feature request, please let me know if you want me to take it to a separate issue. |
#2381 should do the trick (the implementation is a bit slow though, I need to investigate that particular aspect) |
278a39e to
61ee9d2
Compare
|
I'm happy to confirm that #2381 did the trick! With scipy builds and passes (most) tests. There are several small hiccups I need to fix, and then rerun benchmarks. The ball is squarely in my court. Thank you! |
|
#2381 merged then, even though I'll have to improve performance for it later. I'll update this PR, improve it slightly (mostly testing for regression etc) then merge it, if that's fine with you? |
We must keep exactly the same character count for it to work correctly.
61ee9d2 to
519308d
Compare
|
Thank you for being so responsive! A direct channel to the dev is priceless for a user. Your plan is totally fine for me! Performance will be a concern, definitely (us, we hates them perf regressionses). I'll report what I'll find for the scipy usage. Here or elsewhere? Whichever is more convenient. |
|
Thanks for the kind words 🙇
Reporting here is fine.
|
|
Okay, first crude benchmark: the user code which motivated this feature request is about 2x slower. While the following is certainly out of scope for this feature (which is great regardless), I'll ask nonetheless. Mainly to gauge if what I observe is fixable or not at all. And if it is fixable, roughly what it needs from the compiler and from the downstream dev (me). Basically, the change is from the style of (https://github.com/scipy/scipy/blob/v1.16.2/scipy/interpolate/_rbfinterp_pythran.py#L210) So instead of filling a preallocated array, it relies on numpy vectorized constructs. In case a profile is useful, here's one: So questions:
Like I was saying, the feature is great regardless of whether this particular case takes a prohibitive perf hit. |
|
How strange: but maybe you have different kind of input parameter? |
This patch introduces a new argument type of the form `<name> pkg`, for instance `numpy pkg`.
It's a special function export type which, if used, must be used
consistently across all overloads for that exported function.
To support that change, a new pre-processing step has been added. It
performs package inference, and tries hard to statically resolve a
package name based on the lookup made on it. If a single package matches
the constraints, that package is picked. Otherwise nothing is done.
The pythran signature just adds an extra constraint.
For instance,
#pythran export foo(float, numpy pkg)
def foo(x, np):
return np.cos(x)
is equivalent to the following:
#pythran export foo(float, numpy pkg)
def foo(x, _):
import numpy as np
return np.cos(x)
The exported foo function still has two parameters and the second
parameter is going to check that the passed argument is a module named
'numpy'.
Fix #2367
519308d to
060e174
Compare
This patch introduces a new argument type of the form
<name> pkg, for instancenumpy pkg.It's a special function export type which, if used, must be used
consistently across all overloads for that exported function.
It is resolved before any compilation step and is syntactically
equivalent to statically resolving the given argument to the given
package. For instance,
is equivalent to the following:
the exported foo function still has two parameters and the second
parameter is going to check that the passed argument is a module named
'numpy'.
Fix #2367