[MRG+1] Some changes to let Scikit-learn work in Pyston#7162
[MRG+1] Some changes to let Scikit-learn work in Pyston#7162Daetalus wants to merge 2 commits intoscikit-learn:masterfrom
Conversation
…rror Unlike CPython, Pyston treat implicity declaration as error. So declare some structs and functions in related headfiles, and includes them in some source files. Avoid implicity declaration error.
The type of type.__call__ is instancemethod in Pyston. And the function of `signature` rely on it to avoid recursion and segfault(mentioned in the comment of `signature` function. Get the type of wrapper_descriptor by bytearray.__add__, and get method_wrapper by `u'str'.__add__`. These could both work for Pyston, CPython 2 and CPython 3.
|
no objection to merge. with this pyston + sklearn do work? |
|
We still need upstream some changes to Cython. cython/cython#528 Waiting for reply. With those Cython changes, the Scikit-learn test suite can fully passed in Pyston release mode. So after this PR, the sklearn side is ok. |
|
@Daetalus It seems the the cython PR is merged. |
|
yes, I think it is ready now. Thank you! |
| __all__ = ['BoundArguments', 'Parameter', 'Signature', 'signature'] | ||
|
|
||
|
|
||
| _WrapperDescriptor = type(type.__call__) |
There was a problem hiding this comment.
Is that a backport? I don't understand what's happening here (but I'm also not familiar with the code).
There was a problem hiding this comment.
Scikit-learn use type(type.__call__) to get the type wrapper_descriptor and _MethodWrapper = type(all.__call__) to get method_wrapper, according the comment in here: https://github.com/Daetalus/scikit-learn/blob/ab4d8467ccd1586701c7bd698cfeeff460987b34/sklearn/externals/funcsigs.py#L159 It could avoid infinite recursive or segfault.
In Pyston the type of type.__call__ is instancemethod. So it will cause segfault. I change it to +_WrapperDescriptor = type(bytearray.__add__), because in Pyston, this type is wrapper_descriptor, same as CPython. _MethodWrapper = type(u'str'.__add__) used to get method_wapper. Hope it can help.
There was a problem hiding this comment.
funcsigs is a backport of Python 3 function introspection tools for Python 2 compat. It is maintained here:
https://github.com/testing-cabal/funcsigs/
We just embed a vendored copy in sklearn.externals. @Daetalus could you please submit your change in a PR upstream? If it gets accepted there we can merge this PR here. I would rather not have our vendored funcsigs diverge from upstream otherwise it will become a pain to maintain.
There was a problem hiding this comment.
BTW I confirm that type(bytearray.__add__) returns wrapper_descriptor in CPython 2.7 (and in CPython 3.5 as well but we don't use this backport module under Python 3) so the proposed change seems harmless to me.
There was a problem hiding this comment.
Np, but which one is the final upstream? The link you gave me: https://github.com/testing-cabal/funcsigs/, or here: https://github.com/aliles/funcsigs ? Thanks! @ogrisel
There was a problem hiding this comment.
Looking at the commit history it looks like testing-cabal/funcsigs is a continuation of aliles/funcsigs that stopped in December 2013. testing-cabal/funcsigs is also the repo listed in PyPI: https://pypi.python.org/pypi/funcsigs.
|
I don't understand the first change, the rest seems harmless. |
|
@agramfort with this change, the scikit-learn test suite can fully pasted in Pyston latest release mode. Here is the scikit-learn test result in Pyston with this commit: But in debug mode, there still has an issue, but after some investigation, I think the problem is on Pyston side. So after this patch, the scikit-learn side is ok for Pyston. However, Pyston is still in rapidly evolving, so maybe there will have new commits from Pyston. |
|
+1 as well if upstream is ok with funcsigs change as explained in #7162 (comment) . |
@Daetalus out of curiosity how long does it take to run the same test suite on CPython on your machine? |
The original code use type(type.__call__) to get the type `wrapper_descriptor` and use `type(all.__call__)` to get type `method_wrapper`. according the comment in here: https://github.com/Daetalus/funcsigs/blob/master/funcsigs/__init__.py#L170. It could avoid infinite recursive or sefault. In Pyston, the type of `type.__call__` is `instancemethod`. So it will cause segfault. I change it to `type(bytearray.__add__)`, because in Pyston, its type is `wrapper_descriptor`, this is also work in CPython 2.x and CPython 3.x. More information, please see scikit-learn/scikit-learn#7162.
|
@ogrisel That test result is by Pyston release mode. I have a scikit-learn test result with CPython 2.7.7 debug build. The CPython debug build cost 500+s. In order to get the result that you asked for. I will test sklearn with CPython 2.7 release mode today. |
|
It looks like funcsigs is going nowhere on this: testing-cabal/funcsigs#30. Perhaps we should merge despite. |
|
Vendored BLAS was removed in #13203 and the vendored Though some other fixes might be necessary for Pyston compatibility with latest scikit-learn. A PR for that would be welcome. |
This are some changes to let Scikit-learn could work in Pyston.
Unlike CPython, Pyston treat implicit declaration as error. So I added some structs and function declarations in related head files. And also includes proper head files. Please correct me if I did something wrong.
The second change is: In Pyston, the
type(type.__call__)is different than CPython. This was found by @undingen Thank you! He usetype(unicode.__getitem__), but I change it totype(arraybyte.__add__)for compatibility reason. More details please see the second commit.