NotificationsYou must be signed in to change notification settings
Fork8.1k
Star22.2k

COMPAT: use tkagg backend on PyPy#9356

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

tacaswell merged 12 commits intomatplotlib:masterfrommattip:tkagg-cffi

Jan 12, 2018

Merged

COMPAT: use tkagg backend on PyPy#9356

tacaswell merged 12 commits intomatplotlib:masterfrommattip:tkagg-cffi

Jan 12, 2018

Conversation

Copy link

Contributor

mattip commentedOct 11, 2017•
edited
Loading

PR Summary

thetkagg::blitfunction uses theid built-in to obtain memory adresses. This is not compatible with PyPy. I have written a proof-of-concept alternativeblit 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

Copy link

Contributor

anntzer commentedOct 11, 2017•
edited
Loading

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 att.__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 added this to the2.2 (next feature release) milestone

Oct 11, 2017

tacaswell added the GUI: tk label

Oct 11, 2017

Copy link

ContributorAuthor

mattip commentedOct 16, 2017

@anntzer thanks for picking this up.

I tried to modify the id() calls as you said, but have not been able to getload_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 added4 commits

November 5, 2017 21:24

simple plot works

b4e2b8a

flesh out bbox handling, fix mpl_PyFile_Dup for PyPy which is like py…

68492fb

…thon3

change interface in PyAggImagePhoto to avoid id()

8e80a20

search for cffi c-extension module to find c-functions

512e4d8

Copy link

ContributorAuthor

mattip commentedNov 5, 2017•
edited by dopplershift
Loading

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 moduletk._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 inlib/matplotlib/widgets.py, maybe a threading issue or competing event loops?

anntzer reviewed

Nov 5, 2017

View reviewed changes

lib/matplotlib/backends/tkagg.py

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

Copy link

Contributor

anntzerNov 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

pep8

4c5dfaa

Copy link

ContributorAuthor

mattip commentedNov 7, 2017

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

Copy link

ContributorAuthor

mattip commentedNov 7, 2017

How do I fix the codecov/patch check failure?

Copy link

Contributor

dopplershift commentedNov 7, 2017

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

Copy link

Contributor

anntzer commentedNov 7, 2017

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

Copy link

ContributorAuthor

mattip commentedNov 23, 2017

ping

anntzer reviewed

Nov 23, 2017

View reviewed changes

src/_tkagg.cpp Outdated

		#defineSIZE_T_FORMAT"%zu"
		#defineIMG_FORMAT"%zu %d %d"
		#endif
		#defineBBOX_FORMAT"%f %f %f %f"

Copy link

Contributor

anntzerNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

extra space?

Copy link

ContributorAuthor

mattipNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed incc7b73e

anntzer reviewed

Nov 23, 2017

View reviewed changes

src/_tkagg.cpp Outdated

		deststride =4 * destwidth;

		destbuffer =newagg::int8u[deststride * destheight];
		destbuffer =newuint8_t[deststride * destheight];

Copy link

Contributor

anntzerNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

any reason for the change?

Copy link

ContributorAuthor

mattipNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

now it no longer uses#include py_converters.h which supplied theagg:int8u

anntzer reviewed

Nov 23, 2017

View reviewed changes

src/_tkagg.cpp Outdated

		return TCL_ERROR;
		}

		if (sscanf(argv[4], BBOX_FORMAT, &x1, &x2, &y1, &y2) ==4) {

Copy link

Contributor

anntzerNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should probably error out cleanly if an invalid string is passed?

Copy link

ContributorAuthor

mattipNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

yesish, these are basically "private" functions parsing a call from python, but I will add more checking code

Copy link

ContributorAuthor

mattipNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

updated incc7b73e

anntzer reviewed

Nov 23, 2017

View reviewed changes

src/_tkagg.cpp Outdated

		has_bbox =true;
		}
		else {
		elseif (bbox_parse ==1 && x1 ==0){

Copy link

Contributor

anntzerNov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

parens

fixes from review

c2a7143

Copy link

Contributor

anntzer commentedNov 24, 2017

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

silence warnings, fix build on old MSC compilers

09576fa

anntzer reviewed

Nov 24, 2017

View reviewed changes

src/_tkagg.cpp

		desty = hdata - y2;
		destwidth = x2 - x1;
		destheight = y2 - y1;
		destx =(int)x1;

Copy link

Contributor

anntzerNov 24, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't BBOX_FORMAT just be 4 integers and just use integers all along? (not sure if tk would have the bad idea to always cast things to float)

Edit: Actually I guess not, what should really happen is that we should explicitly cast on the caller's side but this can just go as it is for now...

Copy link

ContributorAuthor

mattipNov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The call sequence isbackend.blit(image, bbox) ->tkagg.py:blit(image, bbox) ->PyAggImagePhoto(as_string(image, bbox)) so we cannot change the rootbackend.blit call. AFAICT the choice is:

convert early intkagg.py:blit
convert later inPyAggImagePhoto

I usually prefer converting as late as possible to postpone rounding and allow future refactoring with floats to be as painless as possible, but will flow with whatever is matplotlib best practices

Copy link

Contributor

anntzerNov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think we can leave it as it is for now, we were using floats to start with and we can change that later.

anntzer approved these changes

Nov 24, 2017

View reviewed changes

Copy link

Contributor

anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

assuming appveyor passes

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

Copy link

ContributorAuthor

mattip commentedNov 24, 2017

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

Copy link

ContributorAuthor

mattip commentedNov 28, 2017

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

mattip mentioned this pull request

Dec 1, 2017

make SubplotTool into a modal dialog, keep ref to SubplotTool#9899

Merged

6 tasks

Copy link

ContributorAuthor

mattip commentedDec 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.

596345e

embedding_in_tk2_sgskip is essentially a slightly simplified duplicateof embedding_in_tk_sgskip.

tacaswell mentioned this pull request

Dec 5, 2017

Matplotlib 2 is extremely slow afterplt.show()#8129

Closed

mattip mentioned this pull request

Dec 8, 2017

Python crashes when closing figures using TkAgg on Mac OS#9856

Closed

Merge branch 'tkagg-anim' into tkagg-cffi

1712678

mattip mentioned this pull request

Dec 8, 2017

BUG: clear events before destroying windows in tkagg#9956

Merged

6 tasks

tacaswell reviewed

Dec 11, 2017

View reviewed changes

src/_tkagg.cpp Outdated

		deststride =4 * destwidth;

		destbuffer =newagg::int8u[deststride * destheight];
		destbuffer =newuint8_t[deststride * destheight];

Copy link

Member

tacaswellDec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why change away from the agg types here?

Copy link

ContributorAuthor

mattipDec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Previously there was a conversion from aPyArrayObject * (in other words ndarray) to a data pointer, width, height, etc via py_converters. Now I pass that information directly from python. I no longer need the "py_converters.cpp" and ""py_converters.h" since there is no use of ndarray in the c++ code. Should I leave the include for this one type definition?

Copy link

Member

tacaswellDec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I am concerned about the corner case whereunint8_t andagg:int8u are different underlying types. Not sure how likely that actually is, hence leaning towards being conservative about changing it.

Copy link

Contributor

anntzerDec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I wonder what different types you think these can be.
I would also bet that there must be parts of the code where we assume that this is interconvertible with np.uint8, or that this fits in a single byte... doesn't leave you a lot of options!

Copy link

ContributorAuthor

mattip commentedDec 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