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

COMPAT: use tkagg backend on PyPy #9356

Merged
merged 12 commits into from Jan 12, 2018

Conversation

Projects
None yet
6 participants
@mattip
Contributor

mattip commented Oct 11, 2017

PR Summary

the tkagg::blitfunction uses the id built-in to obtain memory adresses. This is not compatible with PyPy. I have written a proof-of-concept alternative blit function that uses a pure-python _tkagg backend via CFFI. Since CPython prefers a c-extension (_tkagg.so) over a python file (_tkagg.py) and PyPy reverses this, the two implementations can live side by side.

It is working but not yet complete:

  • on the matplotlib side I should probably only conditionally build the _tkagg c-extension on CPython
  • on the PyPy side the serialization/deserialization of a numpy ndarray should not make a copy of the data
  • updating only a bbox of data not supported yet
  • documentation

but I wanted to see if this approach is acceptable before continuing. Any thoughts?

Edit: now point three (bbox handling) is complete
Edit: (5.Nov.17) now points one and two are done, and documentation does not change

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Oct 11, 2017

Contributor

Looks like the only problematic use of id() is to get the underlying address of the ndarray object (the PyObject, not the memory buffer), right? I would suggest instead just passing a memoryview object, which contains all the relevant information even in Py2 (https://docs.python.org/2/library/stdtypes.html#memoryview), and adapting the extension code to accept such an input (it's all private anyways).

Actually now that I look at it it's not clear whether tk.call can pass memoryviews, but you can also just pass in the address of the underlying buffer and the size and strides of the array (and fix the extension code accordingly). The address of the underlying buffer is accessible at t.__array_interface__["data"] (https://docs.scipy.org/doc/numpy-1.13.0/reference/arrays.interface.html#__array_interface__).

If possible, I'd prefer avoid too much pypy specific code, because I fear it's quite likely to bitrot...

Contributor

anntzer commented Oct 11, 2017

Looks like the only problematic use of id() is to get the underlying address of the ndarray object (the PyObject, not the memory buffer), right? I would suggest instead just passing a memoryview object, which contains all the relevant information even in Py2 (https://docs.python.org/2/library/stdtypes.html#memoryview), and adapting the extension code to accept such an input (it's all private anyways).

Actually now that I look at it it's not clear whether tk.call can pass memoryviews, but you can also just pass in the address of the underlying buffer and the size and strides of the array (and fix the extension code accordingly). The address of the underlying buffer is accessible at t.__array_interface__["data"] (https://docs.scipy.org/doc/numpy-1.13.0/reference/arrays.interface.html#__array_interface__).

If possible, I'd prefer avoid too much pypy specific code, because I fear it's quite likely to bitrot...

@tacaswell tacaswell added this to the 2.2 (next feature release) milestone Oct 11, 2017

@tacaswell tacaswell added the GUI/tk label Oct 11, 2017

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Oct 16, 2017

Contributor

@anntzer thanks for picking this up.

I tried to modify the id() calls as you said, but have not been able to get load_tkinter_funcs fron the _tkagg.cpp c-extension working on PyPy, so in the mean time I fleshed out this approach to see if it is functional.

Contributor

mattip commented Oct 16, 2017

@anntzer thanks for picking this up.

I tried to modify the id() calls as you said, but have not been able to get load_tkinter_funcs fron the _tkagg.cpp c-extension working on PyPy, so in the mean time I fleshed out this approach to see if it is functional.

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Nov 5, 2017

Contributor

I actually needed to change very little to get this working, in the end I:

  • modified the private calls to pass "ptr height width" instead of id(array)
  • added another case to the code on non-win32 that searches for the shared object that works with PyPy's cffi module tk._tkinter.tklib_cffi.__file__

On PyPy unfortunately there seems to be an issue with opening the "configure subplot" subfigure on a basic plot, the mouse events are not reliably reaching the event handlers in lib/matplotlib/widgets.py, maybe a threading issue or competing event loops?

Contributor

mattip commented Nov 5, 2017

I actually needed to change very little to get this working, in the end I:

  • modified the private calls to pass "ptr height width" instead of id(array)
  • added another case to the code on non-win32 that searches for the shared object that works with PyPy's cffi module tk._tkinter.tklib_cffi.__file__

On PyPy unfortunately there seems to be an issue with opening the "configure subplot" subfigure on a basic plot, the mouse events are not reliably reaching the event handlers in lib/matplotlib/widgets.py, maybe a threading issue or competing event loops?

_tkagg.tkinit(tk.interpaddr(), 1)
else:
# very old python?
_tkagg.tkinit(tk, 0)

This comment has been minimized.

@anntzer

anntzer Nov 5, 2017

Contributor

interpaddr was added in 1998(! python/cpython@9d1b7ae) so I cannot actually imagine a case where this is triggered today (but that can go to another PR of course)

@anntzer

anntzer Nov 5, 2017

Contributor

interpaddr was added in 1998(! python/cpython@9d1b7ae) so I cannot actually imagine a case where this is triggered today (but that can go to another PR of course)

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Nov 7, 2017

Contributor

@tacaswell I got a mail reviewing the changes to setupext.py but cannot see it here, I removed the reference to py_converters.cpp since it is no longer needed. It was used to convert the python-level id(ndarray) to a C-level ndarray. Now I changed the code to pass in ptr height width instead, so the conversion does not require any knowlege about ndarrays

Contributor

mattip commented Nov 7, 2017

@tacaswell I got a mail reviewing the changes to setupext.py but cannot see it here, I removed the reference to py_converters.cpp since it is no longer needed. It was used to convert the python-level id(ndarray) to a C-level ndarray. Now I changed the code to pass in ptr height width instead, so the conversion does not require any knowlege about ndarrays

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Nov 7, 2017

Contributor

How do I fix the codecov/patch check failure?

Contributor

mattip commented Nov 7, 2017

How do I fix the codecov/patch check failure?

@dopplershift

This comment has been minimized.

Show comment
Hide comment
@dopplershift

dopplershift Nov 7, 2017

Contributor

Don't worry about the codecov check there. The Tk GUI code, unfortunately, has no automated tests for CI to run.

Contributor

dopplershift commented Nov 7, 2017

Don't worry about the codecov check there. The Tk GUI code, unfortunately, has no automated tests for CI to run.

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Nov 7, 2017

Contributor

We tend to ignore that check to be honest...

Contributor

anntzer commented Nov 7, 2017

We tend to ignore that check to be honest...

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Nov 23, 2017

Contributor

ping

Contributor

mattip commented Nov 23, 2017

ping

Show outdated Hide outdated src/_tkagg.cpp
Show outdated Hide outdated src/_tkagg.cpp
Show outdated Hide outdated src/_tkagg.cpp
Show outdated Hide outdated src/_tkagg.cpp
@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Nov 24, 2017

Contributor

Can you rebase on master (to try to make appveyor pass... perhaps)?

Contributor

anntzer commented Nov 24, 2017

Can you rebase on master (to try to make appveyor pass... perhaps)?

Show outdated Hide outdated src/_tkagg.cpp
@anntzer

assuming appveyor passes

Tested locally on cpython, not actually tested myself on pypy...

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Nov 24, 2017

Contributor

thanks, I tested on pypy HEAD and tkagg now works. However the way matplotlib uses weakref.ref in callbacks exposes a bug in the JIT, am working on distilling it so I can report and get it fixes

Contributor

mattip commented Nov 24, 2017

thanks, I tested on pypy HEAD and tkagg now works. However the way matplotlib uses weakref.ref in callbacks exposes a bug in the JIT, am working on distilling it so I can report and get it fixes

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Nov 28, 2017

Contributor

just to be clear, the weakref issue is not a matplotlib issue, rather a PyPy issue that matplotlib exposes

Contributor

mattip commented Nov 28, 2017

just to be clear, the weakref issue is not a matplotlib issue, rather a PyPy issue that matplotlib exposes

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Dec 2, 2017

Contributor

after #9899 was merged, this is all that is needed for the next release of PyPy to work with the TkAgg backend

Contributor

mattip commented Dec 2, 2017

after #9899 was merged, this is all that is needed for the next release of PyPy to work with the TkAgg backend

Cleanup embedding_in_tk, remove embedding_in_tk2.
embedding_in_tk2_sgskip is essentially a slightly simplified duplicate
of embedding_in_tk_sgskip.

@mattip mattip referenced this pull request Dec 8, 2017

Merged

BUG: clear events before destroying windows in tkagg #9956

1 of 6 tasks complete
Show outdated Hide outdated src/_tkagg.cpp
@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Dec 11, 2017

Contributor

I never mentioned that what I thought was a PyPy weakref or JIT issue was actually solved by PR #9899 so this PR is all that is needed for TkAgg and PyPy

Contributor

mattip commented Dec 11, 2017

I never mentioned that what I thought was a PyPy weakref or JIT issue was actually solved by PR #9899 so this PR is all that is needed for TkAgg and PyPy

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Dec 13, 2017

Contributor

what's up with docs build? It seems to have timed out with no connection to my pull request

@tacaswell converted back to agg::int8u

Contributor

mattip commented Dec 13, 2017

what's up with docs build? It seems to have timed out with no connection to my pull request

@tacaswell converted back to agg::int8u

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Dec 19, 2017

Contributor

what's up with docs build? It seems to have timed out with no connection to my pull request

@tacaswell converted back to agg::int8u

edit: docs build seems to have passed

Contributor

mattip commented Dec 19, 2017

what's up with docs build? It seems to have timed out with no connection to my pull request

@tacaswell converted back to agg::int8u

edit: docs build seems to have passed

@jklymak

This comment has been minimized.

Show comment
Hide comment
@jklymak

jklymak Dec 19, 2017

Contributor

Sometimes the builds need to be restarted manually because the host has a problem...

Contributor

jklymak commented Dec 19, 2017

Sometimes the builds need to be restarted manually because the host has a problem...

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Jan 11, 2018

Contributor

can I get this marked 2.2-critical?

Contributor

mattip commented Jan 11, 2018

can I get this marked 2.2-critical?

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Jan 11, 2018

Contributor

"Making PyPy work" sounds like a good thing to have...

Contributor

anntzer commented Jan 11, 2018

"Making PyPy work" sounds like a good thing to have...

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Jan 12, 2018

Contributor

By the way would be nice to add a whatsnew entry (including info about what versions of PyPy are supposed to work).

Contributor

anntzer commented Jan 12, 2018

By the way would be nice to add a whatsnew entry (including info about what versions of PyPy are supposed to work).

@mattip

This comment has been minimized.

Show comment
Hide comment
@mattip

mattip Jan 12, 2018

Contributor

Not sure 270f05d is the proper way to do it, let me know if I should have done it differently

Contributor

mattip commented Jan 12, 2018

Not sure 270f05d is the proper way to do it, let me know if I should have done it differently

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Jan 12, 2018

Member

Perfect @mattip

Member

tacaswell commented Jan 12, 2018

Perfect @mattip

@tacaswell tacaswell merged commit 3bb328f into matplotlib:master Jan 12, 2018

7 of 8 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/project/library 63.68% (target 50%)
Details
codecov/project/tests 98.85% remains the same compared to ad27247
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Jan 12, 2018

Member

Thanks @mattip !

Member

tacaswell commented Jan 12, 2018

Thanks @mattip !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment