Build scripts for Python bindings are not correct [Windows] #1872
Comments
|
Thanks for the clear bug report. I definitely want to fix this so I'll do my best to provide patches if you can test them (I don't have a Windows setup :)). For the first three issues, thanks for pointing them out; I opened #1885 which I think will fix those parts. Let me know if they work (I can't test them on Windows). Some questions:
Ah, hmm, I am not sure, but do you mean that Python bindings fail if we try to link mlpack statically? I can set
This is the tricky one that will require a little debugging... there are a couple of things to test:
I might need to learn more about how Python works on Windows. But if we can get this working you are a hero because it means this will probably be easy to package on PyPI for Windows, which is something I haven't known how to do. |
|
Oops, accidentally clicked the wrong button... |
|
I pulled your branch and tried the changes. When attempting to build the build_pyx_xx projects I got a linker error for all of them --> fatal error LNK1181: cannot open input file 'mlpack.lib' I checked the directory build\src\mlpack\bindings\python\mlpack and it contains: Mode LastWriteTime Length Name -a---- 4/29/2019 7:49 PM 97453 adaboost.cpp If you notice the dump, you will notice the .pyx are empty (0 bytes). |
|
Hmmm, ok. What happens if you run one of the |
|
The generate_pyx_* were successfully built and if I run it it prints out the python code. I believe we need to fix a path to the libraries somewhere in the build scripts (that's I believe what I was doing in point (4)). I figured out that the linker is trying to fetch the libs from /LIBPATH:C:/mlpack/mlpack-python-pr-1885/build but that's not the right location (it should be /build/Release or Debug). Also it's failing to find boost_serialization.lib, which is not a valid library name for Boost in Windows (at least for the official MSVC package). It should be something like boost_serialization-vc141-mt-1_65_1.lib (note: the -vc*- and -1__ chunks will change from version to version). If I rename this .lib to boost_serialization.lib, then build_pyx_* can successfully build and link. |
|
Going a step beyond, this is what happens when you go ahead and install the generated mlpack python package:
|
|
Thanks for the information, and sorry for the slow response---this fell down on the list a little bit. I think I know what is happening---the I also had to use CMake generator expressions for getting the library paths right for mlpack (and some modification was needed to get the Boost serialization library path correctly). It seems to work for Linux. Let me know what happens if you test it... |
|
That's right. Last week I said I was going to update here with my findings but you beat me to it. The ${CMAKE_BINARY_DIR}/bin/ issue was one and the other is a missing openblas reference somewhere, but let me test the new version and report back, thanks! |
|
So I tried the new changes but these errors are blocking the way: And then... I noticed that setup.py is not being generated/copied into any directory. |
|
Ah, sorry, forgot to check in a file. ed6e3f5 should fix it---sorry about that. |
|
Ok we are getting close, but still facing some issues. Now it seems the linker is unable to find the boost libs. Also the CUSTOMBUILD warnings (or errors?) are unexpected as this is a static build. |
|
Oh, my, in digging into this I kind of stumbled upon some ugliness that I might have to work around in setuptools. Do you have the compilation command that setuptools calls for |
|
Yes, this is the link command, which I believe may be missing some of the lib paths specified at the beginning with cmake:
|
|
Thanks for the output---it was helpful. I tried pushing another update to #1885; can you try that? I know how to make CMake link against the right stuff, but figuring out how to pass that to setuptools in a platform-independent way I'm less sure about... |
|
Ok, just tried - now I can see some of the libs properly specified, although there is an empty LIBPATH statement (the second one in the list) which makes the LINK fail:
|
|
Okay... I pushed another commit (4906722) which should strip the empty library directory. I think it will fix the issue... or at least get us to a new error. :) |
|
Ack, hang on, spoke too soon---one more thing to fix. Just a second... |
|
Ok, better now. I realized the Boost link directories were not being properly passed. This should be fixed in a44fdfe. So I think it should link right... with any luck. :) |
|
Uhm, it seems the library_dirs type is not a list of strings: Traceback (most recent call last):
I have tried manually removing the filter() and it seems to work (although my setup.py is being re-generated with every build so I can't be sure it will work for all the pyx). |
|
@gmanlan sorry for the slow response on this one---a bunch of traveling for me in the past two weeks. Anyway I think I pushed a fix; when the library dirs are empty, we have to pass |
|
@rcurtin, just tried again but it seems the error remains:
May it probably be the filter()? |
|
Color me confused---when I run then it seems that what is returned is a list of strings, just like setuptools needs. Anyway, let's try it again just without that filtering code, since on your system we're only passing in a couple directories and I can see there are no empty directories there. I removed the filter in |
|
Ok, now we have LINK errors, which I believe are caused because we are missing LAPACK somewhere? And also some encoding issues (I think there is an unexpected character in this file):
|
|
Great! Thanks for the update @rcurtin. I think it would depend on how you include Boost into the project, as there are different ways of doing so with VS. Looking forward to the next version! |
|
I have an update---I've been working on this in the background for a couple weeks now. Progress is a little slow because it seems Visual Studio builds singlethreadedly which is pretty slow. I've gotten the Python bindings to build successfully and import, so In any case, I do have something seeming to work, but the only problem is, it crashes Python. I haven't yet determined the cause of the crash because I haven't figured out how to get good debugging output, but I am continuing to dig. It looks like some memory is getting freed twice for a reason I don't yet understand. Anyway, I'll keep working at it, but I wanted to provide an update. :) |
|
Another update--- I've been continuing to dig into what's going on here and it seems like the crash in Python has to do with the way memory is being passed back and forth between C++ and Python. My theory (which I have no validation for and am not sure how to validate) is that on Linux and OS X, the memory allocators used in C++ and Python are the same, meaning I can allocate memory in Python and free it in C++ and vice versa, but this seems to not be the case on Windows. What I am going to try as time allows is to rewrite the memory handling such that C++ memory is never "owned" by Python and vice versa, but this is likely to impact the efficiency of the bindings. That said, "bindings working but somewhat slower because of copies" is still better than "bindings not working at all". :) |
|
Hey, thanks for the update. That sounds like a big one, I'm wondering how this was working before on Windows. Looking forward to the next iteration! |
|
Ok, pretty happy to provide this update. I seem to have the Python bindings successfully working from Windows, when I don't allow any passing of memory pointers back and forth from Python to C++. This means there are more copies than we might hope for, but "a bit slow" is better than "doesn't work". Next I need to do some serious cleanups and fix some tests that don't work the way I hoped on Windows, but I believe I have the hard part of this (mostly?) figured out, assuming that I don't soon find that something was far more complicated than I thought. Once I get everything cleaned up, I'll open a PR and we can see if it works for both of us. :) |
Can't we do something like, let binding behave as it is in Linux and os x , but it should behave differently when it is windows , since i would not like it to become slow :) |
|
Agreed, I was planning on different behavior for each system. |
|
That's awesome @rcurtin. I look forward to testing the PR! |
|
I can't believe I am posting this. I thought it would never happen.
It'll take me a little while to figure out all the changes I made and clean it up for a PR, but, this is a nice big step forward. (The next question is whether or not it will work on anyone else's system!) |
|
@gmanlan I pushed some changes to #1885---do you want to try checking them out and seeing if the Python bindings now build on your system? There may still be some little things I overlooked, but at the very least, this code works on my system, so it shouldn't be too hard to figure out how to make it run on yours if it doesn't already. :) |
|
@rcurtin I have pulled the last changes and attempted to use it. On the positive side, I can build perfectly fine without issues. Unfortunately after installing the python package on Windows, I'm back to the initial errors (from .cf import cf ImportError: cannot import name 'cf' from 'mlpack.cf'). These are the steps I performed:
Let me know if you did something different when installing/using the package. Thanks! |
|
Hmmm, ok. Can you tell me if the file |
|
Good point - cf.pyx is empty (0 bytes), as well as lmnn.pyx. All the others are ok. |
|
Hmm, interesting. I seem to remember encountering this in my own experimentation but I thought I had it fixed in the PR. In any case, can you tell me what happens if you run the program |
|
By the way, when I ran these programs locally in my build, I had to make sure all the necessary DLLs were in place, and I ran it from either a command prompt or PowerShell so that I could see the output (instead of the window just closing). |
|
I think both are working fine (I can see the python code being generated, without errors). You can see the output of generate_pyx_cf.exe in the attached file.
|
|
Well now that is strange. You might be able to get everything to build by "rebuild"ing the |
|
I've been debugging but I see nothing wrong... still, the pyx is empty. Do you know from where the .exe is being called? I can't find it in the VS logs. |
|
I managed to reproduce it and nail down what the issue is. It so happens that A workaround is to drop I'll be working on figuring out how to get a clean solution for this and let you know what I figure out. |
|
Ah, interesting. I'm wondering why the command |
|
That's strange, but I agree, Visual Studio can be pretty mysterious. Same with CMake. I spent a while digging into it and I concluded that the safest way I can fix the issue is simply to copy all the necessary DLLs to the right place. So, the latest commit that I pushed adds the CMake variables In the case of the Boost libraries and specifically OpenBLAS installed via nuget (where the runtime libraries are in the OpenBLAS directory's I'll push a commit tomorrow with updated documentation, but, the commit I just pushed to #1885 at least I think should make the Python bindings compile and run successfully for you also. Sorry again for the slow response---the compile/test cycles are quite slow because I'm running in an underpowered VM, so it's basically one or two compile attempts per night. :) |
|
Hey, good news. I have tried the latest #1885 and the windows python bindings are building and running. To check if it's properly working, I adapted the example from https://www.mlpack.org/doc/mlpack-3.1.1/doxygen/python_quickstart.html and it seems to work just fine. Well done! We may want to properly document how to build this, and even better in the future, auto-generate a windows package to be installed using pip.
|
|
Definitely, the https://github.com/mlpack/mlpack-wheels repository is ready to try and attempt some Windows builds when everything is merged. I'm not actually sure what happens with the Python bindings on Windows when I make the "INSTALL" target. I'm running now to find out... :) |
|
Ok, I fought with it for a while and realized I was setting |
|
This is awesome @rcurtin! I left a minor comment after reviewing. Very excited to get this integrated soon! |
|
With #1885 merged, I think that we can close this. :) Next I will move towards getting mlpack Python bindings packaged for PyPI on Windows. |
Issue description
Attempting to build python bindings on Windows using Visual Studio 2017 fails due to several issues:
package_dir={ '': 'C:/mlpack/build/src/mlpack/bindings/python/' }
This path ends in a slash which is not valid in a python package. What is more, I believe this path should be relative. If so, it should be replaced by: package_dir={ '': '.' },
ImportError: cannot import name 'test_python_binding' from 'mlpack.test_python_binding' (C:\mlpack\build\src\mlpack\bindings\python\mlpack\test_python_binding.cp37-win_amd64.pyd)
Your environment
Steps to reproduce
-DBUILD_PYTHON_BINDINGS=ON -DBUILD_SHARED_LIBS=ON
Expected behavior
To successfully build python bindings AND the egg package to work (be able to import mlpack in python)
Actual behavior
Build failures (when workarounds are applied, the resulting package doesn't work)
The text was updated successfully, but these errors were encountered: