Bug Report: mplfinance >= 0.12.5a0 crashes without an error when loading an plot image in tKinter

[theGOTOguy]

Describe the bug
mplfinance crashes without an error when loading images into a tKinter label.

To Reproduce
requirements.txt:

certifi==2020.4.5.2
chardet==3.0.4
cycler==0.10.0
idna==2.9
kiwisolver==1.2.0
matplotlib==3.2.1
mpl-finance==0.10.1
mplfinance==0.12.5a2
numpy==1.18.5
pandas==1.0.4
Pillow==7.1.2
psutil==5.7.0
pyparsing==2.4.7
python-dateutil==2.8.1
pytz==2020.1
requests==2.23.0
retrying==1.3.3
six==1.15.0
urllib3==1.25.9

repro.py

import datetime
import matplotlib.pyplot as plt
import mplfinance as mpf
import pandas as pd
import random
import io 
import tkinter as tk
from tkinter import ttk
from PIL import Image, ImageTk

class ReproWindow():
  def __init__(self):
    self.root = tk.Tk()
    self.root.geometry('800x600')

    self.image_label = tk.Label()
    self.image_reference = None

    self.image_label.pack()
    button = tk.Button(self.root, text="Next!", command=self.next)
    button.pack()

    self.get_image_and_display()

    self.root.mainloop()
   
  def get_image_and_display(self):
    security_data = []

    last_close = 50
    for i in range(100):
      result = {'t': datetime.datetime(2000, 1, 1) + datetime.timedelta(days=i)}
      ohlc = random.choices(range(1, 100), k=3)

      result['l'] = min(ohlc)
      ohlc.remove(min(ohlc))
      result['h'] = max(ohlc)
      ohlc.remove(max(ohlc))
      result['o'] = last_close
      result['c'] = ohlc[0]
      last_close = ohlc[0]
      result['v'] = random.choice(range(3, 100))
      security_data.append(result)

    data = [{
        'Date': sd['t'],
        'Open': sd['o'],
        'High': sd['h'],
        'Low': sd['l'],
        'Close': sd['c'],
        'Volume': sd['v']} for sd in security_data]

    security_dataframe = pd.DataFrame(data)
    security_dataframe.set_index('Date', inplace=True)

    img_buffer = io.BytesIO()
    mpf.plot(security_dataframe, type='candle', style='yahoo', volume=True, show_nontrading=True, savefig=dict(fname=img_buffer, dpi=192))
    plt.close('all')
    img_buffer.seek(0)

    pil_image = Image.open(img_buffer)
    pil_image = pil_image.resize((self.root.winfo_width(), self.root.winfo_height() - (self.root.winfo_reqheight() - self.image_label.winfo_height())))
    self.image_reference = ImageTk.PhotoImage(pil_image)
    self.image_label.configure(image=self.image_reference)
    self.image_label.image = self.image_reference

  def next(self):
    # Reset the window.
    self.get_image_and_display()

app = ReproWindow()

Steps to reproduce the behavior:

First, create a new directory and create a virtualenv for testing:
virtualenv venv -p python3
source venv/bin/activate

Install the requirements.
pip install -r requirements.txt

Run the repro.
venv/bin/python repro.py

Click the "Next!" button.

After a small number of loads (usually just the first one for me), python will crash with no error reported.

Expected behavior
You should be able to click "Next!" an unlimited number of times to continue loading new simulated candlestick graphs.

Desktop (please complete the following information):

• OS: Ubuntu 18.04

Additional context
If you roll back to mplfinance 0.12.4a0, this crash does not occur. e.g.,:

pip install mplfinance==0.12.4a0

For now, this workaround is fine for me.

(venv) ben@anteater:~/code/MarketBias$ venv/bin/python repro.py 
(venv) ben@anteater:~/code/MarketBias$ pip install mplfinance==0.12.5a2
Collecting mplfinance==0.12.5a2
  Using cached mplfinance-0.12.5a2-py3-none-any.whl (49 kB)
Requirement already satisfied: pandas in ./venv/lib/python3.6/site-packages (from mplfinance==0.12.5a2) (1.0.4)
Requirement already satisfied: matplotlib in ./venv/lib/python3.6/site-packages (from mplfinance==0.12.5a2) (3.2.1)
Requirement already satisfied: numpy>=1.13.3 in ./venv/lib/python3.6/site-packages (from pandas->mplfinance==0.12.5a2) (1.18.5)
Requirement already satisfied: pytz>=2017.2 in ./venv/lib/python3.6/site-packages (from pandas->mplfinance==0.12.5a2) (2020.1)
Requirement already satisfied: python-dateutil>=2.6.1 in ./venv/lib/python3.6/site-packages (from pandas->mplfinance==0.12.5a2) (2.8.1)
Requirement already satisfied: pyparsing!=2.0.4,!=2.1.2,!=2.1.6,>=2.0.1 in ./venv/lib/python3.6/site-packages (from matplotlib->mplfinance==0.12.5a2) (2.4.7)
Requirement already satisfied: kiwisolver>=1.0.1 in ./venv/lib/python3.6/site-packages (from matplotlib->mplfinance==0.12.5a2) (1.2.0)
Requirement already satisfied: cycler>=0.10 in ./venv/lib/python3.6/site-packages (from matplotlib->mplfinance==0.12.5a2) (0.10.0)
Requirement already satisfied: six>=1.5 in ./venv/lib/python3.6/site-packages (from python-dateutil>=2.6.1->pandas->mplfinance==0.12.5a2) (1.15.0)
Installing collected packages: mplfinance
  Attempting uninstall: mplfinance
    Found existing installation: mplfinance 0.12.4a0
    Uninstalling mplfinance-0.12.4a0:
      Successfully uninstalled mplfinance-0.12.4a0
Successfully installed mplfinance-0.12.5a2
(venv) ben@anteater:~/code/MarketBias$ venv/bin/python repro.py 
{submitter note:  it crashed with no error given here}
(venv) ben@anteater:~/code/MarketBias$ pip install mplfinance==0.12.4a0
Collecting mplfinance==0.12.4a0
  Using cached mplfinance-0.12.4a0-py3-none-any.whl (42 kB)
Requirement already satisfied: pandas in ./venv/lib/python3.6/site-packages (from mplfinance==0.12.4a0) (1.0.4)
Requirement already satisfied: matplotlib in ./venv/lib/python3.6/site-packages (from mplfinance==0.12.4a0) (3.2.1)
Requirement already satisfied: numpy>=1.13.3 in ./venv/lib/python3.6/site-packages (from pandas->mplfinance==0.12.4a0) (1.18.5)
Requirement already satisfied: pytz>=2017.2 in ./venv/lib/python3.6/site-packages (from pandas->mplfinance==0.12.4a0) (2020.1)
Requirement already satisfied: python-dateutil>=2.6.1 in ./venv/lib/python3.6/site-packages (from pandas->mplfinance==0.12.4a0) (2.8.1)
Requirement already satisfied: kiwisolver>=1.0.1 in ./venv/lib/python3.6/site-packages (from matplotlib->mplfinance==0.12.4a0) (1.2.0)
Requirement already satisfied: pyparsing!=2.0.4,!=2.1.2,!=2.1.6,>=2.0.1 in ./venv/lib/python3.6/site-packages (from matplotlib->mplfinance==0.12.4a0) (2.4.7)
Requirement already satisfied: cycler>=0.10 in ./venv/lib/python3.6/site-packages (from matplotlib->mplfinance==0.12.4a0) (0.10.0)
Requirement already satisfied: six>=1.5 in ./venv/lib/python3.6/site-packages (from python-dateutil>=2.6.1->pandas->mplfinance==0.12.4a0) (1.15.0)
Installing collected packages: mplfinance
  Attempting uninstall: mplfinance
    Found existing installation: mplfinance 0.12.5a2
    Uninstalling mplfinance-0.12.5a2:
      Successfully uninstalled mplfinance-0.12.5a2
Successfully installed mplfinance-0.12.4a0
(venv) ben@anteater:~/code/MarketBias$ venv/bin/python repro.py 
{submitter note:  now I am able to load new plots indefinitely.}

[DanielGoldfarb]

@theGOTOguy
Ben,

First, thank you for the awesome job providing a simple, easy means to reproduce the error.

Second, a work-around for mplfinance version 0.12.5 is to set closefig=False in the call to mpf.plot()

mpf.plot(data,...,savefig={...},closefig=False)

Third, perhaps you can give me your thoughts on the source of this issue, after I fill you in on what I have found. Here is some background and what I have found so far:

v0.12.5 implemented kwarg closefig as described here. The default value for closefig is "auto" meaning mplfinance will close the figure when mplfinance thinks it is safe to do so. This value can be overriden with True or False just in case mplfinance gets it wrong.

Here's the thing: The line of code to close the figure, for your use-case, is here: https://github.com/matplotlib/mplfinance/blob/master/src/mplfinance/plotting.py#L658

If I change that line of code from:

plt.close(fig)

to:

plt.close('all')

then the "crash" problem goes away! Your thoughts?

I am beginning to suspect the issue may be in matplotlib itself, since it appears that plt.close(fig) and plt.close('all') are behaving differently. It is not clear to me how either of them could possibly affect the figure that was, in theory, already saved to an io.BytesIO() buffer.

By the way, I have also verified (with print() statements in repro.py) that the "crash" upon clicking "Next!" does not occur until after self.get_image_and_display() has returned . So it is not crashing per se (certainly not inside mpf.plot()) but rather there appears to be an issue with the image that mplfinance writes to the io.BytesIO object.

All the best. --Daniel

[DanielGoldfarb]

I am able to reproduce this problem by modifying get_image_and_display() to be:

def get_image_and_display(self):

    x = [0,1,2,3,4,5,6]
    y = [2,3,1,4,2,5,3]

    fig, ax = plt.subplots()
    ax.plot(x,y)

    img_buffer = io.BytesIO()

    plt.savefig(fname=img_buffer, dpi=192)
    #plt.close('all')    # This works!
    plt.close(fig)       # This doesn't work

    img_buffer.seek(0)

    pil_image = Image.open(img_buffer)
    pil_image = pil_image.resize((self.root.winfo_width(), self.root.winfo_height() - (self.root.winfo_reqheight() - self.image_label.winfo_height())))

    self.image_reference = ImageTk.PhotoImage(pil_image)
    self.image_label.configure(image=self.image_reference)
    self.image_label.image = self.image_reference

[theGOTOguy]

Thanks for the quick response, and the research into the issue!

I can confirm that adding closefig=False fixes the issue in 0.12.5a2.

Your matplotlib-only repro is so interesting that I've looked into it further. It appears that closing a single figure and closing all figures use entirely different routines in matplotlib.

Using the time-honored method of printf debugging, then, I tried printing the list of known figures in destroy_all (venv/lib/python3.6/site-packages/matplotlib/_pylab_helpers.py), thinking that maybe some extra figures were being created and left around. There were not.

Next, I tried printing the figure number found in destroy_fig, thinking that somehow that lookup was failing. It is not.

And now here's what I don't fully understand, but it works. I did a print after each line in destroy, and it made it all the way through correctly and crashed at some later time. Therefore, I concluded that the only difference between destroy's approach and destroy_all's approach is in the order of operations.

matplotlib at HEAD has changed from my version, so I am including a code snippet from _pylab_helpers.py in matplotlib 3.2.1:

    @classmethod
    def destroy(cls, num):
        """
        Try to remove all traces of figure *num*.

        In the interactive backends, this is bound to the
        window "destroy" and "delete" events.
        """
        if not cls.has_fignum(num):
            return
        manager = cls.figs[num]
        manager.canvas.mpl_disconnect(manager._cidgcf)
        cls._activeQue.remove(manager)
        del cls.figs[num]
        manager.destroy()
        gc.collect(1)

    @classmethod
    def destroy_fig(cls, fig):
        "*fig* is a Figure instance"
        num = next((manager.num for manager in cls.figs.values()
                    if manager.canvas.figure == fig), None)
        if num is not None:
            cls.destroy(num)

    @classmethod
    def destroy_all(cls):
        # this is need to ensure that gc is available in corner cases
        # where modules are being torn down after install with easy_install
        import gc  # noqa
        for manager in list(cls.figs.values()):
            manager.canvas.mpl_disconnect(manager._cidgcf)
            manager.destroy()

        cls._activeQue = []
        cls.figs.clear()
        gc.collect(1)

Note that in destroy_all, manager.destroy() is called before cls.figs.clear(), and in destroy, the figure is deleted from cls.figs before manager.destroy() is called. It turns out that swapping these two operations in destroy fixes the issue!

At first I thought that somehow del cls.figs[num] was deleting the manager and that this bug may have been accidentally fixed at HEAD in matplotlib by this code cleanup, but substituting in manager = cls.figs.pop(num) and removing the del cls.figs[num] entirely does not fix the issue, so that's not what's going on.

At any rate, you're right. This is a matplotlib bug, not a mplfinance bug. I'm happy to file a bug and a PR with matplotlib (unless you want to, you did a bunch of research into the issue too!) I do have one question, though: now knowing what exactly the issue is, is it clear to you why this crash is happening in the first place, and how it's possible that nobody's noticed it before? I'd love to have that explanation be part of the issue/PR so that I could go forward a smarter person.

[DanielGoldfarb]

Hi Ben,

Sorry for taking so long to get back to you. Thanks very much for taking this investigation further and identifying that calling manager.destroy() before deleting the figure appears the fix the problem. (I too had noticed that close(fig) and close('all') led to two different functions, but it was getting late and I had to log off and didn't have time look at any details.)

At any rate, I would be honored if you would file a bug issue with matplotlib/matplotlib and post a link here so that I can follow. (Meanwhile I have a few mplfinance enhancements and fixes that I am working on this week).

I am reluctant to change mplfinance to call close('all') since it creates only one figure itself and really should not be closing any other figures the caller may have created. At least, for now, we have the workaround of setting closefig=False. Again, thank you for your work and contribution on identifying this bug. I think we made a great team tracking it down. If not for you excellent work on the repro I would have been lost. (I'm not really familiar with tkInter).

All the best. --Daniel

[DanielGoldfarb]

Btw, about

now knowing what exactly the issue is, is it clear to you why this crash is happening in the first place, and how it's possible that nobody's noticed it before?

I have no idea. I am still relatively new myself at this matplotlib stuff, especially when it comes to "backends" and Figure managers, and things at that level.

It's still a mystery to me why closing the Figure would have anything to do with your tkInter objects, since the way it looks to me from my naive standpoint, the only thing that gets passed to tkInter is the image data in the BytesIO object. I was thinking that once matplotlib writes to the BytesIO object, anything else that matplotlib does after that would be completely idependent of anything that is done with the ByteIO object later by other code. I too would love to have an explanation so that I can go forward knowing more about both matplotlib and tkInter.

[theGOTOguy]

I've created a bug in matplotlib. One caveat that they do mention is that matplotlib is explicitly not thread-safe, but as far as I can tell, tKinter's mainloop() is executed in the same thread that calls it, so I don't believe that that would be an issue. Hopefully someone there will be able to enlighten us!

Thanks for maintaining this awesome library, and for helping me hunt down this weird bug. Always a pleasure to work with another physicist!

[DanielGoldfarb]

@theGOTOguy
Ben,
I'm going to close this issue. It looks like we can follow up on the issue you created in matplotlib/matplotlib As I would expect, Tom Caswell had some good insights into the problem. He knows quite a bit about the inner workings of matplotlib. Another fellow physicist, he's the one who got me involved in matplotlib and mplfinance when I met him during sprints at "PyData NYC" last year.
All the best. --Daniel

P.S. It seems the bug is apparent only when using matplotlib backend "TkAgg" -- now I can understand my misconception that

I was thinking that once matplotlib writes to the BytesIO object, anything else that matplotlib does after that would be completely idependent of anything that is done ... later by other code.

Apparently when using matplotlib.pyplot with "TkAgg" then matplotlib is using TkInter which can then "clash" with external use of TkInter.

[theGOTOguy]

Yep, makes sense. I'm good to go now that I have a workaround, but still looking forward to learning why the order of closing the manager and deleting it from the list of figures matters!

[DanielGoldfarb]

Ben,
By the way, although I have no experience using TkInter, the is another approach that involves using mpf.plot(data,...,returnfig=True) to return the Figure and Axes, and then use that Figure in TkInter. An example of this approach can be found here: #125 (comment) You can also see another example here.
HTH. All the best. --Daniel