-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add Pythran support to build, convert two functions #3226
Add Pythran support to build, convert two functions #3226
Conversation
This comment has been minimized.
This comment has been minimized.
ab86c34
to
25ba8b9
Compare
@jni / @stefanv : short status update. I updated kernel code to be pep8 compilant (wasn't the case for original cython code). Due to a mvsc update, some kernels no longer compile under windows but that's fixed in pythran master version. Until then, I'd like some guidance on integration of pythran kernel into asv-powered benchmarks! |
8887f76
to
e7df48c
Compare
For the record: I just opened serge-sans-paille/pythran#955 to make sure all scikit-image kernels are and remain correctly compiled by pythran. I'll feed the kernels with default values to make sure the behavior is unchanged, this is probably related to the bencmakrs task (I need guidance for valid / reasonable input) |
For the record, serge-sans-paille/pythran#955 now validates ok on windows. |
@jni / @stefanv : all scikit-image kernels have been added to the pythran validation suite and compile OK on linux and windows! It's all in master branch, so next step on my side would be to make a Pythran release compatible with scikit-image, but I'd like to be sure there is no other blocker before. |
@serge-sans-paille so cool! On this end we would need benchmarks to all the bits that we're touching. Now on master we have a benchmark framework using airspeed velocity. But I don't want to push this work on you — this is something we will get to eventually. On the logistics of actually getting this merged, we tend to be quite conservative, so there's a good chance that what we actually need to do is to use Cython's Pure Python mode to have these things compilable with either Pythran or Cython. (Or alternatively keep the existing Cython around and use some kind of setup.py flag to determine compilation with one lib or the other.) This is a major project but one that I'm excited to see completed. It would also open the door to using Numba optionally. |
That looks better than Pure Python mode, performance wise. I can handle that task. |
@jni just pushed a tentative cython fallback, but as stated in the commit message, I'm not sure that's the way we should take. But a Python fallback would be extreemly slow too... |
@serge-sans-paille it's a very clean extension, thank you! I absolutely sympathise with not wanting to maintain two separate code bases, believe me. =) But the reality is that we are quite risk averse at scikit-image, with good reason. So switching all-out to Pythran, which is a much younger project than Cython with a smaller community, is probably not feasible in the short term. But a hybrid approach might be feasible, and might be a good way for the core team to get practice working with Pythran, get confident in using it, and eventually decide to take the leap. The alternative is to leave this PR open for a long time, which is its own maintenance burden. There's again another possibility, which is to wait until we go 3.6+, and then we can use this for Cython: https://github.com/AlanCristhian/statically I also presume something like this would be possible with Pythran, too? Actually a decorator is much more Pythonic than a comment. @stefanv @soupault @emmanuelle do you guys have any input here? To summarise:
|
OK, this totally makes sense. If I understand correctly, I won't need to play with the benchmarks, so my part of the job is almost done :-) Concerning the hybrid path, It's probably better to give an option to the user to control which back-end to use, a flag to setup.py is feasible, or maybe an environment variable (easier to setup :-))? I'll probably do a pythran release in august, that would include all the patches required for the scikit-image integration. |
@jni up! Any step missing on my side? |
@serge-sans-paille very cool. =) What do you think about the idea of implementing Pythran as a decorator instead of a comment + external command? |
@jni. I'm not a big fan of this idea, because pythran is an ahead of time compiler: compile speed has never been a real focus, and it's not rare to have a compile step that takes 5 to 10 seconds. Even if cached, this is not acceptable for a decorator. Moreover, Pythran only handles full modules. Being able to extract the relevant subpart of the module is an interesting subject and we may want to support that at some point, but it's definitly not possible now. |
@serge-sans-paille very good points! How about the middle ground: use Python 3.5+ type annotations, if available, instead of comments. But build still happens ahead of time. |
@jni. Yeah, that's in the plan, once Python2.7 support is dropped. |
Is that a blocking feature for that PR? |
Not quite blocking, just some extra friction. The major friction of course is the multiple parallel implementations. |
8712bd1
to
edc6d08
Compare
hi @jni. Just updated the integration step, with an optional env variable to disable pythran to lower the friction. |
edc6d08
to
6ae16a5
Compare
(rebased on master) |
f7f50e0
to
3cbf5e5
Compare
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.
I don't understand what is up with github. It seems that this is redoing a few typo PRs that were done this last week.
.appveyor.yml
Outdated
@@ -1,5 +1,7 @@ | |||
# AppVeyor.com is a Continuous Integration service to build and run tests under | |||
# Windows | |||
image: | |||
- Visual Studio 2017 |
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.
I'm not sure conda is running Visual Studio 2017. Can you confirm that we would be able to ship this package with conda?
doc/Makefile
Outdated
@@ -3,8 +3,8 @@ | |||
|
|||
# You can set these variables from the command line. | |||
PYTHON ?= python | |||
SPHINXOPTS ?= -j 1 | |||
SPHINXBUILD ?= python $(shell which sphinx-build) | |||
SPHINXOPTS ?= -j $(shell nproc) |
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.
This doesn't really belong in this PR and seems to have been done in #3478 whats up with github?
@@ -334,7 +334,7 @@ Rewriting commit history | |||
|
|||
Do this only for your own feature branches. | |||
|
|||
There's an embarassing typo in a commit you made? Or perhaps the you | |||
There's an embarrassing typo in a commit you made? Or perhaps the you |
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.
Haven't these been merged already? Whats up with github? #3480
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.
I messed up my rebase. Not a github thing ^^!
skimage/_build.py
Outdated
@@ -35,8 +38,7 @@ def cython(pyx_files, working_path=''): | |||
except ImportError: | |||
# If cython is not found, the build will make use of | |||
# the distributed .c files if present | |||
c_files = [f.replace('.pyx.in', '.c').replace('.pyx', '.c') for f in pyx_files] | |||
for cfile in [os.path.join(working_path, f) for f in c_files]: | |||
for cfile in abs_c_files: |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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.
nevermind, this is a different thing.
So I am listening to your talk from 2013 (I guess that is what I found on Youtube) and it seems pretty awesome!!!
The ability to use AVX instructions from native python is awesome!
|
The pythran package on conda is too old :-/
5044db1
to
661a3f2
Compare
@jni I've been able to reproduce the issue locally, and it seems to be codna specific. At least using virtualenv as asv backend works locally. To my great shame, I cannot find how to update the label, if you could retrigger it? |
you just use the labels menu to the right and remove the "run-benchmark" label and then add it again to restart. I went ahead and did it just now, but something is odd because it completed almost instantly without doing anything: https://github.com/scikit-image/scikit-image/actions/runs/1454438133.
it looks like after the commit switching from conda->virtualenv it was unable to find Python |
I pushed a couple of commits that installs Python 3.7 and |
@grlee77 ok, seems I don't have the rights to change the labels, thanks for doing it. Do you know if there's any advantage in using mamba/conda env vs. virtualenv as asv backend? Access to more binary packages like compilers etc? |
Did you get an invite to join the triage team? (I think @jni said he added you to it). I would have thought accepting that would have given you permission to add/remove labels.
I don't think it should matter too much in our case. The dependencies we need should be available from pypi as well, so virtualenv is fine with me. |
@grlee77 indeed, the invite went right to my spam folders :'(. I can now update labels. All lights are green, do you want me to rebase and cleanup the history? |
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.
@serge-sans-paille no need! The changes are actually compact enough that I think squashing is appropriate. Congrats! It's only taken three years to get this in!!! 😂 🎉 🚀
Now that I am less worried about all of our infrastructure, I am looking more slowly at the actual implementation details and damn it's awesome. 😃 Thank you @serge-sans-paille!
\o/ I won't submit more kernel until next skimage release, so that we have some feedbacks about how wheels are built etc. |
@serge-sans-paille If you have an idea of what might be the cause, please let us know! The failure was on a
<\details> |
Just a guess: |
Thanks, I will try this! In the meantime, I did confirm that AMD64 cases work okay and it is just x86 that is failing |
The changes in commit a0e0f7a fixed the x86 windows builds. I had to add For consistency I also force installed LLVM in the AMD64 case, but this was not strictly necessary (CI was already passing without the explicit LLVM install steps for AMD64). Let me know if this doesn't seem like the right choice. The force install doesn't take long, but does result in a >200MB download. |
Description
Use Pythran instead of cython to compile several kernels, while keeping
somewhat high level and pythonic code.
Checklist
[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]
./doc/examples
(new features only)./benchmarks
, if your changes aren't covered by anexisting benchmark
[For detailed information on these and other aspects see scikit-image contribution guidelines]
References
#2956
For reviewers
(Don't remove the checklist below.)
later.
__init__.py
.doc/release/release_dev.rst
.@meeseeksdev backport to v0.14.x