[Bug]: tk backend confused by presence of file named "move" in current working directory

[drmcnelson]

Bug summary

With a file named "move" in the current working directory, the tk backend reports that it cannot find /usr/share/matplotlib/mpl-data/images/move

Rename the local file to anything else, and everything works okay.

Code for reproduction

N/A  too large.  See the above error messages.

Actual outcome

Traceback (most recent call last):
File "/usr/lib64/python3.11/multiprocessing/process.py", line 314, in _bootstrap
self.run()
File "/usr/lib64/python3.11/multiprocessing/process.py", line 108, in run
self._target(*self._args, **self._kwargs)
File "/home/nelson/Projects/TeensyDataAcquistion/Python_Programs/GraphicsWindow.py", line 80, in animation
self.fig = plt.figure(self.name)
^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/_api/deprecation.py", line 454, in wrapper
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/pyplot.py", line 783, in figure
manager = new_figure_manager(
^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/pyplot.py", line 359, in new_figure_manager
return _get_backend_mod().new_figure_manager(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/backend_bases.py", line 3513, in new_figure_manager
return cls.new_figure_manager_given_figure(num, fig)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/backend_bases.py", line 3518, in new_figure_manager_given_figure
return cls.FigureCanvas.new_manager(figure, num)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/backend_bases.py", line 1703, in new_manager
return cls.manager_class.create_with_canvas(cls, figure, num)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/backends/_backend_tk.py", line 481, in create_with_canvas
manager = cls(canvas, num, window)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/backends/_backend_tk.py", line 431, in init
super().init(canvas, num)
File "/usr/lib64/python3.11/site-packages/matplotlib/backend_bases.py", line 2814, in init
tools.add_tools_to_container(self.toolbar)
File "/usr/lib64/python3.11/site-packages/matplotlib/backend_tools.py", line 1027, in add_tools_to_container
container.add_tool(tool, group, position)
File "/usr/lib64/python3.11/site-packages/matplotlib/backend_bases.py", line 3377, in add_tool
self.add_toolitem(tool.name, group, position,
File "/usr/lib64/python3.11/site-packages/matplotlib/backends/_backend_tk.py", line 957, in add_toolitem
button = NavigationToolbar2Tk._Button(frame, name, image_file, toggle,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/matplotlib/backends/_backend_tk.py", line 792, in _Button
NavigationToolbar2Tk._set_image_for_button(self, b)
File "/usr/lib64/python3.11/site-packages/matplotlib/backends/_backend_tk.py", line 726, in _set_image_for_button
with Image.open(path_large if (size > 24 and path_large.exists())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/site-packages/PIL/Image.py", line 3092, in open
fp = builtins.open(filename, "rb")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/usr/share/matplotlib/mpl-data/images/move'
<TextWindow.TextWindow object at 0x7f3fae9e0e10>

Expected outcome

No error messages

Additional information

The general context is a live a data acquisition system with data displayed in an animation. While there is a file named "move" in the current directory, the program produces the above error at launch.

Operating system

Fedora 37, Cinnamon desktop

Matplotlib Version

3.6.3

Matplotlib Backend

tk_agg

Python version

3.11.4

Jupyter version

n/a

Installation

Linux package manager

[ksunden]

I am unable to reproduce this problem, can you reproduce with up to date mpl, for instance?

[drmcnelson]

Thank you for trying. Unfortunately it is a large multi-process program and it will take some work to reduce it to a minimum example. But, the graphics library itself (attached) is not so long. There are a few buttons added to the standard toolbar, perhaps that is what "opens the can of worms". The error occurs at launch.

I removed and reinstalled and updated the mpl, no change.

One more note, this is has been going on intermittently for more than a year. I just today realized it was the presence of another file in the working directory with the same name as one of the standard buttons.

GraphicsWindow.py.zip

[drmcnelson]

Note it runs from the multiprocessing api rather than the threading library. One of these runs for each piece of equipment in the data acquisition system, generally one or two, sometimes three. The error appears independent of that.

[anntzer]

@ksunden I haven't actually tried but almost certainly this is because the OP uses MEP22 toolmanager (see their traceback), which has questionable semantics of first trying to load an image with the requested filename from the current directory (see ToolContainerBase._get_image_filename). In my view this behavior is basically broken due to cases like the OP's. I'd guess it should be deprecated (this can be done by checking whether the file would be loaded from a deprecated path) and replaced by something like ToolBase.image being the path to the image relative to the directory containing the source file where the tool was defined (so that one can still write third-party tools and distribute the icon next to the python source (or in a subfolder, via image = "data/..." (for the case of builtin tools, we'd have something like image = "mpl-data/images/move", so we can also remove the special-casing of basedir as well).

[QuLogic]

I've found the same, and I don't see how that ever could have been expected to work correctly. I guess we could deprecate it, but it seems unusable in general. Since we use normal Path joining, if they don't want the image from the mpl-data directory, third parties can set image to an absolute path.

[drmcnelson]

Yes, clearly. There seems to be multiple issues.

A) The internal default should include a default path, or something that refers to one.

B) The file named "move" in the local directory was not even typed as a graphics file (.png,.svg,.etc) The error message nothing about that.

C) The error message, unable to find "/usr/share/matplotlib/mpl-data/images/move" misleads as to the actual source of the error.

D) If there is going to be an override it would make more sense that it be the directory the program runs from, or better still, require that it be supplied by the programmer/user.

E) As I recall, developing the library that uses this, there were some difficulties having it use a supplied image for the button. In that instance I actually did want to supply a file, in a sub directory from where the program resides. Eventually I gave up and chose one from the mpl-data/images directory. Extending the button set for a program that uses the mpl libraries should not require that programmers/user add files to the directory tree where mpl is installed.

[drmcnelson]

So all in all it is a confused mess of (a) requiring that a file resides in mpl-data/images, (b) nonetheless getting tripped up when one with a similar name resides in the current working directory, and then (c) emitting an error message referring back to mpl-data/images with no reference to what actually happened.

When someone recently asked me to describe something "frustrating", in that moment I couldn't think of anything. But, here it is.

[drmcnelson]

@QuLogic

" they don't want the image from the mpl-data directory, third parties can set image to an absolute path."

Yes, but actually needs both. We would not want to have to recreate the entire images directory in our local tree just to add one button.

So something like "local_button_image_path". Check there first, then mpl-data/images. And do type checking too.

[anntzer]

Doesn't "path relative to the directory containing the python source defining the tool" (as I proposed above) work?

[ksunden]

Okay, yes, I can indeed reproduce once I set rcParams["toolbar"] = "toolmanager".

The culprit appears to be:

matplotlib/lib/matplotlib/backend_bases.py

Lines 3298 to 3311 in 88e4a43

	def _get_image_filename(self, image):
	"""Find the image based on its name."""
	if not image:
	return None
	basedir = cbook._get_data_path("images")
	for fname in [
	image,
	image + self._icon_extension,
	str(basedir / image),
	str(basedir / (image + self._icon_extension)),
	]:
	if os.path.isfile(fname):
	return fname

In combination with:

matplotlib/lib/matplotlib/backends/_backend_tk.py

Line 735 in 88e4a43

path_regular = cbook._get_data_path('images', button._image_file)

The first method sets the image property of the tool to the first found file , first looking in the cwd (without adding an extension), then adding an extension, then looking in the mpl-data/images folder (without and with extension added). If it finds the file in the cwd, it does not make it absolute.

The latter further modifies it by always looking in the mpl-data/images folder (though if an absolute path will ignore that).

The default move tool does not specify the extension so that it can pick up varied extensions using the dynamic searching of the first method linked there, and thus get e.g. svg where available and png when needed.

[tacaswell]

I think the issue here is that one part of the code treats relative paths against CWD (which in the case of writing an un-packaged application maybe makes sense) and another part of the code assumes that relative path are against our data directory (which makes some degree of sense).

diff --git a/lib/matplotlib/backend_bases.py b/lib/matplotlib/backend_bases.py
index 85ca72e3b5..ed7b99bce1 100644
--- a/lib/matplotlib/backend_bases.py
+++ b/lib/matplotlib/backend_bases.py
@@ -2971,7 +2971,7 @@ class NavigationToolbar2:

         if len(axes) == 0:
             return []
-
+k
         if self._nav_stack() is None:
             self.push_current()   # Set the home button to this view.

@@ -3296,17 +3296,29 @@ class ToolContainerBase:
         """Find the image based on its name."""
         if not image:
             return None
-
-        basedir = cbook._get_data_path("images")
-        for fname in [
-            image,
-            image + self._icon_extension,
-            str(basedir / image),
-            str(basedir / (image + self._icon_extension)),
-        ]:
-            if os.path.isfile(fname):
+        imgp = Path(image)
+        # if relative, first check against our data
+        if rel_path := (not img.is_absolute()):
+            imgp = cbook._get_data_path("images", image)
+
+        # check both as-is and with the tools image extension
+        for fname in [imgp, imgp.with_suffix(self._icon_extension)]:
+            if fname.exists():
                 return fname

+        # if we are here, we did not find the image try CWD for back-compat
+        if rel_path:
+            imgp = Path(image)
+
+            for fname in [imgp, imgp.with_suffix(self._icon_extension)]:
+                if fname.exists():
+                    # make absolute to be safe
+                    return fname.absolute()
+
+        # found no file
+        return None
+
+
     def trigger_tool(self, name):
         """
         Trigger the tool.

I think something like the above patch is the right way to fix this. We have a small behavior change (we prefer our images over the same named images in CWD).

It might make sense to swap the search order of the "fixed" suffix and not as well.

[drmcnelson]

Yes, probably. It is an interesting problem.

I think the right answer is that mpl-data is where the default images are stored, and anything else should be based on an explicit call with an explicit file path or image loaded into program memory.

In this case, nowhere in the code did I ask it to load an image named "move". It had no business whatsoever looking for that button image anywhere but mpl-data, and certainly not in my working directory.

[drmcnelson]

@tacaswell I don't think that is the right answer.

It should not be looking anywhere but relative to some explicit configured install path, except when the programmer explicitly tells it to use a different image file.

Second, if there would be a second tree to search it should not be the current working directory.

(For example, I generate data sets in different directorie. I cd into each directory and run that same graphics program. Why in the world, would I want the button to change every time I cd into a different directory? Really!!!)

Third, you get into trouble with this because you are searching more than one tree with one being dynamic, i.e. cwd. It simply has to produce chaos and spooky bugs like this one that wasted days of my time and probably others as well.

[tacaswell]

This is an argument against cwd in general (which I am quite sympathetic to 🤣). However, there is a very strong convention that a relative path are relative to the global CWD (e.g. what open does).

The core of the problem is that we have one part of the code doing the "normal" thing, one part doing a "weird" thing and using a different root to resolve the relative path, and we pass a normal path between them.

In the case of having an installed application I agree that grabbing files from CWD is surprising, however if you are developing a little script that looks like

my_app/
├── my_icon.png
├── my_lib.py
└── my_main.py

then you can both import your local library (because CWD (from Python start time) is in sys.path) and get your image. Not your use case, but also not an unreasonable one. I can also see the argument for searching CWD first (it is closer to the user and how the search paths on a lot of tools work).

We probably should also gracefully warn rather than fail if an icon image fails to load.