DIPlib / diplib

Quantitative Image Analysis in C++, MATLAB and Python
https://diplib.org
Apache License 2.0
228 stars 50 forks source link

Issues with running PyDIP on Windows #55

Closed Wicloz closed 4 years ago

Wicloz commented 4 years ago

Windows 10 Python 3.8.0 Visual Studio 16.5.4 Java 8u211 DIPlib @ca62d40 PyCharm 2020.1


After installing DIPlib and PyDIP in accordance with INSTALL_Windows.md, except for the optional dependencies, I got the following error:

Traceback (most recent call last):
  File "D:/Stack/Bioinformatica/2019-2020/Image Analysis with Applications in Microscopy/Assignment 2/main.py", line 1, in <module>
    import PyDIP as dip
  File "C:\Program Files\Python38\lib\site-packages\PyDIP\__init__.py", line 30, in <module>
    from PyDIP.PyDIP_bin import *
ImportError: DLL load failed while importing PyDIP_bin: The specified module could not be found.

After doing some research, I stumbled on this Python issue. As it turns out, at some point during the development of Python 3.8, the usage of the PATH variable for DLL loading has been disabled. See also this blog and this Microsoft documentation.

In addition to this, it appear that DIPjavaio.dll depends on jvm.dll which is bundled with Java. The default Java installation on Windows, however, does not add this DLL to the PATH variable. As such, PyDIP gives the same ImportError even on Python version where DLLs are still loaded using the PATH variable.

I was able to work around this issue by copying all of DIP's DLLs as well as jvm.dll to the PyDIP package folder. It should, however, be solvable from within __init__.py in accordance with the Python issue linked above.


After solving the DLL issue, two other issues still remain. First of all, there is no autocompletion in my IDE, even though this works on Linux. Secondly, upon the usage of the Kuwahara filter, I get the following unspecific error:

Traceback (most recent call last):
  File "D:/Stack/Bioinformatica/2019-2020/Image Analysis with Applications in Microscopy/Assignment 2/main.py", line 83, in <module>
    dip.Kuwahara(img1a),
RuntimeError: Caught an unknown exception!

Could this be because I'm lacking a certain library?

Wicloz commented 4 years ago

It would appear that installing Doxygen and completely restarting the installation adds autocompletion to my IDE. If this can be reproduced, it should be added to INSTALL_Windows.md. This autocompletion is far from perfect, however.

crisluengo commented 4 years ago

Thanks @Wicloz for reporting these issues.

  1. Windows 10 / Python 3.8 issue locating DLL files. I'm not sure how to solve this. I didn't spot an easy solution in the Python issue you linked, I probably skimmed the page too quickly and missed it. Moving the jvm.dll file to the DIPlib directory doesn't seem like a good solution. We need to tell the binary where to search for libraries. I tested this on Windows 7 with Python 3.7, I guess I need to upgrade my Windows VM to be able to test possible solutions. @ronligt do you have any ideas on how to solve this?

  2. I don't have an issue with the Kuwahara filter on macOS. The following works:

    import PyDIP as dip
    a=dip.ImageRead('/Users/cris/newdip/examples/trui.ics')
    dip.Kuwahara(a)

    This filter uses OpenMP (if you compiled with it enabled), but doesn't depend on any other external libraries. If other filters work, then so should this one. I'll run it under the memory checker to see if there's an out-of-bounds write that is harmless on macOS but problematic on Windows.

  3. I don't see how Doxygen would add autocompletion to your IDE. We use Doxygen to generate documentation for the C++ library, but not for Python. I think one issue is that we don't have any documentation at all for the Python interface (well, there's just a little bit of documentation there). It might have taken your IDE some time or some prodding to index the function names and parameter names. By the way, I'm still looking for a way to auto-generate documentation for Python from the C++ Doxygen documentation.

crisluengo commented 4 years ago

OK, in Python 3.8 we need to use os.add_dll_directory. No need to test for Python versions, though, it is better to test for the existence of that particular function.

import os
if os.name == 'nt':
    if 'add_dll_directory' in dir(os):
        os.add_dll_directory("@CMAKE_INSTALL_PREFIX@/@LIBRARY_DESTINATION@")
    else:
        os.environ["PATH"] += os.pathsep + "@CMAKE_INSTALL_PREFIX@/@LIBRARY_DESTINATION@"

from PyDIP.PyDIP_bin import *
from PyDIP.PyDIP_py import *

Though we should really be using something like

with os.add_dll_directory(...):
    from PyDIP.PyDIP_bin import *
    from PyDIP.PyDIP_py import *

Not sure what the best way is to combine those two.

crisluengo commented 4 years ago

Found a way to combine the two here.

import contextlib
import os

@contextlib.contextmanager
 def add_gdal_dll_directories():
     dll_dirs = []
     for dll_directory in dll_directories:
         dll_dirs.append(os.add_dll_directory(dll_directory))
     try:
         yield None
     finally:
         for dll_dir in dll_dirs:
             dll_dir.close()

with fiona._loading.add_gdal_dll_directories():
     from fiona.collection import BytesCollection, Collection
     ...
crisluengo commented 4 years ago

@Wicloz I have no way of testing this fix currently. Would you be willing to fetch the Python3.8_on_Windows_issue branch and see if this works for you?

Wicloz commented 4 years ago

With regard to the Kuwahara filter failing, I have tried out all the non-linear filters listed in the documentation. Most filters work, except for the following:

Additionally, when running the code multiple times the error above is occasionally replaced with the following error, which confirms your theory of memory issues.

Traceback (most recent call last):
  File "D:/Stack/Bioinformatica/2019-2020/Image Analysis with Applications in Microscopy/Assignment 2/main.py", line 115, in <module>
    dip.AdaptiveBanana(img2b, [img3a]).Show()
RuntimeError: bad allocation
in function: AdaptiveFilter (D:\Downloads\diplib-master\src\nonlinear\adaptivegauss.cpp at line number 822)

I will be able to try out the fixed branch later tonight.

crisluengo commented 4 years ago

The three bilateral filters you mention do not have Python bindings because the function BilateralFilter is the interface to call any of the three.

Kuwahara is implemented using SelectionFilter, so it's the same error you see. On macOS, using Clang and OpenMP, I don't have any issues that AddressSanitizer can detect, so it's not a write-out-of-bounds issue nor a writing to unallocated or freed memory. I think this is a Windows-specific issue. SelectionFilter does not yet use multi-threading, so it's definitely not an OpenMP implementation issue. Unfortunately, without access to Windows I cannot debug this problem.

AdaptiveBanana also shows no issues in AddressSanitizer. It is implemented using the much of the same code as AdaptiveGauss, but that one runs without problems? That limits the source of the problem to a very limited section of code...

Wicloz commented 4 years ago

Unfortunately, this new branch does not work, and gives the following error:

Traceback (most recent call last):
  File "C:\Program Files\Python38\lib\site-packages\PyDIP\__init__.py", line 42, in add_diplib_dll_directory
    yield None
  File "C:\Program Files\Python38\lib\site-packages\PyDIP\__init__.py", line 50, in <module>
    from PyDIP.PyDIP_bin import *
ImportError: DLL load failed while importing PyDIP_bin: The specified module could not be found.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:/Stack/Bioinformatica/2019-2020/Image Analysis with Applications in Microscopy/Assignment 2/main.py", line 1, in <module>
    import PyDIP as dip
  File "C:\Program Files\Python38\lib\site-packages\PyDIP\__init__.py", line 64, in <module>
    hasDIPjavaio = True
  File "C:\Program Files\Python38\lib\contextlib.py", line 131, in __exit__
    self.gen.throw(type, value, traceback)
  File "C:\Program Files\Python38\lib\site-packages\PyDIP\__init__.py", line 45, in add_diplib_dll_directory
    dll_dir.close()
AttributeError: 'str' object has no attribute 'close'
wcaarls commented 4 years ago

I fixed the SelectionFilter problem. As far as I can tell, AdaptiveBanana works if called with the example from the documentation, but fails with this error when called as

dip.AdaptiveBanana(a, [])

so I think the parameter checking should be improved.

wcaarls commented 4 years ago

As for the location of jvm.dll, that seems quite hairy. Perhaps the easiest option would be to use %JAVA_HOME% or %JRE_HOME% to search for it, but in many cases those will have to be set manually by the user. Another option is to remove PyDIPjavaio altogether and write some Python code that depends on python-bioformats (similar to DIPimage).

crisluengo commented 4 years ago

@wcaarls Thanks for that fix! Good catch!

The AdaptiveBanana and related functions apparently checked for the right number of images in the 2nd parameter using DIP_ASSERT, which does nothing when compiled for release. I have fixed this, just haven't pushed it yet.

python-bioformats sounds like a really good idea. But I don't think it fixes "DLL load failed while importing PyDIP_bin", since that one doesn't depend on Java any more.

crisluengo commented 4 years ago

@Wicloz Thanks so much for testing that! I guess I have more work to do there. I will have to find a Windows 10 distribution somewhere, otherwise it's going to take for ever... :)

Wicloz commented 4 years ago

With regard to what @wcaarls mentioned, I should clarify that there are 2 separate DLL issues, which both give the same error:

  1. On all operating systems, PyDIP will not work on Python >= 3.8 due to the PATH no longer being used.
  2. On only Windows DIPjavaio.dll does not work. This affects all versions of Python, and probably MATLAB as well. This is because it relies on jvm.dll, which is not added to the PATH of Windows.

One of my classmates installed DIPlib without compiling DIPjavaio on Python 3.7, which worked without any DLL issues, confirming the issues above.

wcaarls commented 4 years ago

I just pushed a change that uses os.add_dll_directory when available. This leaves finding jvm.dll, but that might be obviated if we remove PyDIPjavaio altogether.

crisluengo commented 4 years ago

@wcaarls : I have a first attempt at using the python-bioformats module: https://github.com/DIPlib/diplib/commits/python_bioformats

Let me know what you think.

There's an issue where the interpreter hangs upon exit (^D) after using Bio-Formats. No idea what that is about.

wcaarls commented 4 years ago

I'm having trouble compiling javabridge in Windows. It does not seem to recognize non-oracle JDK immediately. I'll check more later.

For reference, I wrote this code to setup Java under windows for PyDIPjavaio. It works as long as there is some kind of environment variable indicating where Java is to be found. This is the default when installing AdoptOpenJDK (on PATH); when unzipping OpenJDK manually, JAVA_HOME needs to be set.

import os, shutil

def setup_java_windows():
    paths = []
    if "JAVA_HOME" in os.environ:
        paths.append(os.path.join(os.environ["JAVA_HOME"], "bin", "server"))
        paths.append(os.path.join(os.environ["JAVA_HOME"], "jre", "bin", "server"))
    if "JRE_HOME" in os.environ:
        paths.append(os.path.join(os.environ["JRE_HOME"], "bin", "server"))

    exe = shutil.which("java")

    if exe:
        paths.append(os.path.join(os.path.dirname(exe), "server"))
        paths.append(os.path.join(os.path.dirname(exe), "..", "jre", "bin", "server"))

    for p in paths:
        dll = os.path.join(p, "jvm.dll")
        if os.path.exists(dll):
            try:
                os.add_dll_directory(p)
            except:
                os.environ["PATH"] += os.pathsep + p
            return

    print("jvm.dll not found")

When compiling from source, it might be easier to just find it from CMake, but that doesn't work for binaries.

crisluengo commented 4 years ago

@wcaarls,

I just learned that javabridge is an older package, the newer version is python-javabridge. Try to install that one, openjdk8 and openjdk11 are part of its Travis test suite. It installs the module with the same name, so nothing else changes.

According to the developer, the JVM cannot be started twice in the same process (so start, kill, start -> doesn't work). And there's no way to kill the JVM using atexit. So the only clean way of using javabridge is to have the user call a function to kill the VM before trying to exit Python. It sort of sucks, but it seems to be the only way.

The advantage of BioFormats in MATLAB is that it runs in MATLAB's JVM.

In your C++ interface, the JVM is started upon first use, and not explicitly killed, I presume it is automatically killed upon process termination.

In Python, we would be relying on this 3rd party package that doesn't let Python terminate until the JVM is killed, and cannot be killed by atexit because that only runs when Python terminates, which it cannot do.

I much prefer using DIPjavaio, if the path thing on Windows can be solved neatly.

wcaarls commented 4 years ago

It's not just a problem under Windows; in Linux (and OSX?) as well if we distribute binaries without RPATHs. I'm thinking about just getting the discovery code from python-javabridge and using that. It's BSD-licensed. What do you think?

crisluengo commented 4 years ago

I'm fine with that. Just add an entry in the list of included code in the README.md file.

wcaarls commented 4 years ago

Could you give d74ee7c a spin? It seems to work for Windows and Linux (at least my versions, with my Java versions, etc., etc.)

crisluengo commented 4 years ago

I need to change the import statement to:

import PyDIP.loadjvm as loadjvm

otherwise it doesn't find the loadjvm module.

With that change, it checks a whole lot of directories and finally finds the right one. It also produces these two lines:

Exception ignored in: <generator object suggest_javahome at 0x1191100d0>
RuntimeError: generator ignored GeneratorExit
wcaarls commented 4 years ago

I committed eb936dc (but forgot to reference this issue...), which hopefully works. With that, all three parts of this issue should be solved.

Other issues may arise when distributing binaries (such as rpaths in the Python modules), which might also be solved by preloading like this. But that is a problem for another time.

crisluengo commented 4 years ago

Works great!

With this commit you've completed the fix for the DLLs not being found. We've also fixed the issues with Kuwahara and AdaptiveBanana. So I'll close this issue.

Wicloz commented 4 years ago

I can confirm that installing the latest master works out of the box now, without any workarounds. I can also confirm that the three filters mentioned earlier (SelectionFilter, Kuwahara, and AdaptiveBanana) work properly on Windows. Thank you for the effort you put in to expand the range of platforms PyDIP functions on.

As a final note, I would recommend adding Doxygen to the list of dependencies in INSTALL_Windows.md and include this link, as this software is required for the c++ documentation, which in turn is processed by certain IDEs to generate autocompletion.

crisluengo commented 4 years ago

Thanks @Wicloz for testing this again!

I'll add a note to the Windows installation instructions about Doxygen. Thanks for the idea. I did not know that any IDEs would use the output of Doxygen, rather than the Doxygen comments in the header files, to help with autocompletion.