8000 MAINT Use Cython memoryviews · Issue #10624 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MAINT Use Cython memoryviews #10624

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.

Sign up for GitHub

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

Jump to bottom
Closed
rth opened this issue Feb 12, 2018 · 39 comments
Closed

MAINT Use Cython memoryviews #10624

rth opened this issue Feb 12, 2018 · 39 comments
Labels
cython

Comments

@rth
Copy link
Member
rth commented Feb 12, 2018 •
edited
Loading

Currently a fair amount of Cython code uses the numpy buffer interface.

When possible, it could be worth migrating some of it to the typed memoryviews Cython interface that has lower overhead and could result in some performance improvements [1], [2]. This would also allow to release GIL more frequently which would be positive for parallel computations with the threading backend.

I'm not saying that it makes sense to use memoryviews everywhere, and there are subtle differences between the two (possibly related #7059 (comment)). Just opening this issue to track progress.

For instance, sklearn/utils/sparsefuncs_fast.pyx could be a place to start.
@rth rth added Moderate Anything that requires some knowledge of conventions and best practices help wanted labels Feb 12, 2018
@glemaitre
Copy link
Member

Probably the main limitation is that cython memoryviews cannot be used in read-only.
This is something which used by joblib. But there is a PR: cython/cython#1869

@rth
Copy link
Member Author
rth commented Feb 12, 2018 •
edited
Loading

Probably the main limitation is that cython memoryviews cannot be used in read-only. This is something which used by joblib.

Right, good to know @glemaitre ! That is unfortunate.

Also related #7981 .Looks like not much can be done about it at the moment #7981 (comment) . Keeping this issue open in case this gets resolved upstream in Cython.

@rth rth removed Moderate Anything that requires some knowledge of conventions and best practices help wanted labels Feb 12, 2018
@jnothman
Copy link
Member
jnothman commented Feb 12, 2018 via email

@jnothman
Copy link
Member
jnothman commented Feb 12, 2018 via email

@rth
Copy link
Member Author
rth commented Feb 12, 2018 •
edited
Loading

maybe we need to persuade the numpy people that they should spend their
funding on fixing this...

Well, "[spend a] small part of it" might work better :)

Also, as far as I understand from cython/cython#1869 (comment), PR has an initial implementation, and providing some more usage tests for it would help to move it forward. I'm not familiar enough with the way memoryviews are treated in joblib to do that, unfortunately..

@rth rth mentioned this issue Mar 2, 2018
Closed
@rth
Copy link
Member Author
rth commented Mar 7, 2018

From scipy roadmap,

Regarding Cython code:
[...]
Cython’s old syntax for using NumPy arrays should be removed and replaced with Cython memoryviews.

so I don't understand why the fact that scipy uses memoryviews is not an issue but migrating internal cython code is? I imagine scipy won't use const memoryviews by default . cc @lesteve @glemaitre

Also Cython 0.28.b1 is out, what should be the procedure to test it? Currently travis uses mostly 0.26.1.
Should we eventually migrate that to 0.28 (cython/cython#1869 (comment)) or keep a combination of both?

@lesteve
Copy link
Member
lesteve commented Mar 7, 2018 •
edited
Loading

so I don't understand why the fact that scipy uses memoryviews is not an issue but migrating internal cython code is? I imagine scipy won't use const memoryviews by default

It is an issue as soon as you are using a cython function taking memoryview as arguments in a joblib.Parallel calll and that one of your argument is a numpy array whose size is above max_nbytes (or that you are passing a read-only memmap explicitly).

Also Cython 0.28.b1 is out, what should be the procedure to test it? Currently travis uses mostly 0.26.1.

Great to hear that a 0.28 beta is out! For the record, the scipy-dev-wheels build is run daily and uses the cython-dev wheel. If you are talking more about how to test the const memoryview cython fix, a first good step would be to pick one of the existing issues (#7981 seems like a good candidate for example), adding const where appropriate to the function signature and make sure that the bug is gone when compiling with cython 0.28b1. Another thing we could do is find places where a memoryview was used and a bug-fix got rid of it and revert the code to the original code using memoryviews with const added (#4775 seems like a good candidate).

I have a plan to add something to test_common.py which monkeypatches the joblib.Parallel max_nbytes default and set it to 0 as an attempt to detect more failures. I haven't got around to it yet.

@rth
Copy link
Member Author
rth commented Mar 8, 2018

It is an issue as soon as you are using a cython function taking memoryview as arguments in a joblib.Parallel

I know, I meant that generally speaking, I guess it might become an issue more frequently if scipy proceeds with the migration.

For the record, the scipy-dev-wheels build is run daily and uses the cython-dev wheel

Good to know. 8000

a first good step would be to pick one of the existing issues (#7981 seems like a good candidate for example), adding const where appropriate to the function signature and make sure that the bug is gone when compiling with cython 0.28b1.

Yes, but my point is that as long as CI uses cython < 0.28, we won't be able to add a PR testing that on different OS and Python versions because tests would fail. So I'm wondering what should/can be done about it.

I have a plan to add something to test_common.py which monkeypatches the joblib.Parallel max_nbytes default and set it to 0 as an attempt to detect more failures.

That would be very useful.

@lesteve
Copy link
Member
lesteve commented Mar 8, 2018 •
edited
Loading

Yes, but my point is that as long as CI uses cython < 0.28, we won't be able to add a PR testing that on different OS and Python versions because tests would fail. So I'm wondering what should/can be done about it.

What about creating a scikit-learn branch and experimenting with using const memoryview (both with cython <= 0.27 and cython > 0.27) ? I have started an experiment on my fork here. From this little experiment this seems feasible (although not completely trivial, mutiple trial and errors ...) to have a common cython codebase for cython <= 0.27 and cython > 0.27.

@jnothman
Copy link
Member
jnothman commented Mar 8, 2018 via email

@lesteve
Copy link
Member
lesteve commented Mar 8, 2018

Sure we can require cython>=0.28 when it is out. Given that cython 0.28b1 has been announced, this was more meant as an experiment to provide feed-back to cython in case we notice any problems.

@lesteve
Copy link
Member
lesteve commented Mar 9, 2018 •
edited
Loading

FYI, I tried to revert #4775 and use memoryviews in sklearn/linear_model/cd_fast.pyx. This does not seem straightforward (a Travis log of the error with cython master is here) at all at the moment with cython. The main blockers seems to be:

@rth
Copy link
Member Author
rth commented Mar 10, 2018

Thanks for looking into it @lesteve ! I guess hoping this would work straight away was a bit too optimistic ...

@lesteve
Copy link
Member
lesteve commented Mar 12, 2018

FYI I have emailed cython-users with the main issues I found: https://groups.google.com/d/msg/cython-users/7B_RNQXefC8/ryDyFUV7AAAJ. Let's see what they say ...

@rth
Copy link
Member Author
rth commented Mar 13, 2018 •
edited
Loading

Thanks for sending that email @lesteve !

After discussing this topic with @massich @jorisvandenbossche and @glemaitre , if we can't make const (ro) memoryviews work with fused types, one way around it could be to use the Cython.Tempita templating system. I can't find any documentation about it but there are a few references in #9032 (comment) and pandas-dev/pandas#13399

@lesteve
Copy link
Member
lesteve commented Mar 13, 2018

I tried to ask nicely but it looks like 0.28 will ship without the fix, but there is some hope that the bug will be looked at and fixed in the next minor release. Oh well at least I tried ...

I guess tempita could work as another work-around. My feeling is that it is a bit painful of adding layers of work-arounds that may have to be undone if the bug gets fixed eventually though.

@jorisvandenbossche jorisvandenbossche mentioned this issue Jul 2, 2018
Merged
3 tasks
@rth rth mentioned this issue Aug 1, 2018
Merged
@rth rth mentioned this issue Sep 1, 2018
Closed
@jakirkham
Copy link
Contributor

Regarding the const memoryview case, there is a simple workaround, which could be tried. Still a bit hacky when compared to having things just work as intended in Cython, but less so.

@rth
Copy link
Member Author
rth commented Sep 10, 2018

@jakirkham I saw that, is it a pretty impressive (but as you say, somewhat hacky) workaround. To put something like this in scikit-learn it would be better if cython/cython#1772 was just fixed altogether and I would really like to for it to happen.

With @lesteve we spend half a day trying to investigate this in Cython, with limited success, I'll post status updates on that issue.

@jakirkham
Copy link
Contributor

Thanks for sharing the update.

That's fair. Though I think we will need a Cython expert to direct us and/or possibly help us do some of the work.

The advantage of the workaround is we are able to get something running today with a minimal amount of "hackiness" and effort. Plus we can mirror the behavior Cython will have for end users.

@rth rth mentioned this issue Dec 6, 2018
Merged
@NicolasHug
Copy link
Member

Hi all,

Does this all mean that even new contributions shouldn't use memory views? I am working on #12807 and I am extensively using memory views.

Just want to make sure I won't have to change back to pointers later on.

Thanks!

@jeremiedbb
Copy link
Member
jeremiedbb commented Dec 21, 2018 •
edited
Loading
8000

You can't use memviews if both following conditions are met:

  • original array is read only
  • memview type is fused

otherwise it's fine. For the first it means you can't use for data which can be provided by the user (typically X and y in fit(X, y)), but when you allocate temporary arrays in read-write mode you can use memviews.

@thomasjpfan
Copy link
Member

In @scoder blog post about Cython 0.29:

Long-time users of Cython and NumPy will be happy to hear that Cython's memory views are now API-1.7 clean, which means that they can get rid of the annoying "Using deprecated NumPy API" warnings in the C compiler output. Simply append the C macro definition ("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION") to the macro setup of your distutils extensions in setup.py to make them disappear. Note that this does not apply to the old low-level ndarray[...] syntax, which exposes several deprecated internals of the NumPy C-API that are not easy to replace. Memory views are a fast high-level abstraction that does not rely specifically on NumPy and therefore does not suffer from these API constraints.

@thomasjpfan thomasjpfan mentioned this issue Jul 19, 2019
Merged
@lesteve
Copy link
Member
lesteve commented Sep 10, 2019

A PR has been recently been merged in cython to allow const with fused type memoryviews: cython/cython#3118. It would be great if someone could give it a go and give feed-back.

@rth rth mentioned this issue Sep 27, 2019
Open
@rth rth mentioned this issue May 9, 2020
Merged
@jjerphan
Copy link
Member
jjerphan commented Apr 13, 2021 •
edited
Loading

A PR has been recently been merged in cython to allow const with fused type memoryviews: cython/cython#3118. It would be great if someone could give it a go and give feed-back.

const with fused types memoryviews might help with issues like #19875

@jjerphan jjerphan mentioned this issue Apr 13, 2021
Merged
@jjerphan jjerphan mentioned this issue Apr 22, 2021
Closed
@lorentzenchr lorentzenchr mentioned this issue Aug 31, 2021
Merged
@lorentzenchr
Copy link
Member

Until Cython 3 arrives, a possible temporary fix for const with fused typed memoryviews is #20903.

@ginward ginward mentioned this issue Oct 3, 2021
Closed
@cmarmo cmarmo added the cython label Jan 15, 2022
@Micky774 Micky774 mentioned this issue Apr 17, 2022
Merged
@glemaitre glemaitre mentioned this issue Nov 10, 2022
Closed
22 tasks
@jjerphan jjerphan mentioned this issue Nov 14, 2022
Merged
@jjerphan
Copy link
Member
jjerphan commented Jan 5, 2023 •
edited
Loading

FYI, the next bug-fix release of Cython (ETA January 2023) will provide the fix for const fused-typed memoryviews. 🎉

cython/cython@78bb640#diff-ff3c479edefad986d2fe6fe7ead575a46b086e3bbcf0ccc86d85efc4a4c63c79R20-R21

@jakirkham
Copy link
Contributor

Looks like that landed in Cython 0.29.33, which was recently tagged 🚀

@jjerphan
Copy link
Member
jjerphan commented Jan 7, 2023

Yes! Cython 0.29.33 also was released yesterday on PyPI. 🥳

@lorentzenchr lorentzenchr mentioned this issue Jan 7, 2023
Closed
@lorentzenchr
Copy link
Member

That's great news. I opened #25322 to discuss increasing the minimum Cython version.

@jjerphan
Copy link
Member
jjerphan commented Jan 19, 2023 •
edited
Loading

It is now possible to use const-fused typed memoryviews in scikit-learn!

I have a draft of an issue to give indications, yet I think opening it might create a clash with this one?

Edit: removed the draft and have opened #25484

@lesteve lesteve mentioned this issue Jan 19, 2023
Merged
@lorentzenchr
Copy link
Member

FYI, scipy/scipy#14262 might be a blocker for const memoryviews when we call scipy C API for BLAS functions.

@jjerphan
Copy link
Member
jjerphan commented Jan 19, 2023 •
edited
Loading

FYI, scipy/scipy#14262 might be a blocker for const memoryviews when we call scipy C API for BLAS functions.

We use wrappers around SciPy interfaces to BLAS'; those are defined in sklearn/utils/_cython_blas.pyx. I think that we can discard the const-qualification in their implementations if needed.

@jakirkham
Copy link
Contributor

Are the wrappers already ignoring const since they don't have that in their signatures?

@jjerphan
Copy link
Member

They might. Some compilers are stricter than others and raise a compilation errors (e.g. when using C++ on some compilation units) whilst others are just raising warning when discarding const implicitly.

There's one place where we explicitly discard const-qualification:

scikit-learn/sklearn/metrics/_pairwise_distances_reduction/_middle_term_computer.pyx.tp

Lines 389 to 393 in 3f82f84

# Casting for A and B to remove the const is needed because APIs exposed via
# scipy.linalg.cython_blas aren't reflecting the arguments' const qualifier.
# See: https://github.com/scipy/scipy/issues/14262
DTYPE_t * A = <DTYPE_t *> &self.X[X_start, 0]
DTYPE_t * B = <DTYPE_t *> &self.Y[Y_start, 0]

I think that we can work around this limitation even if scipy/scipy#14262 remains unsolved.

@jjerphan
Copy link
Member

FYI, I have opened #25484 to list actionable improvements in this regards.

@rgommers rgommers mentioned this issue Feb 14, 2024
Merged
3 tasks
@lesteve
Copy link
Member
lesteve commented Jul 1, 2024

I am going to close this one since I guess #25484 has been merged a while ago.

Feel free to reopen if I missed something.

@lesteve lesteve closed this as completed Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cython
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0