Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python Windows fixes (ready) #1885

Merged
merged 34 commits into from Nov 3, 2019

Conversation

@rcurtin
Copy link
Member

rcurtin commented Apr 27, 2019

This is collecting fixes for #1872. @gmanlan let me know if these fixes solve the problems you're having on Windows. As we get to the bottom of more issues we can add to this PR and merge when we have it working.

@rcurtin rcurtin removed the s: unanswered label Jun 19, 2019
rcurtin added 7 commits Aug 30, 2019
It seems we need to avoid passing memory pointers back and forth---it
seems like on Windows, anything allocated in Python cannot be freed in
C++, and vice versa.
@rcurtin

This comment has been minimized.

Copy link
Member Author

rcurtin commented Sep 25, 2019

Okay, not quite sure what I did with the Windows git client to push like that, but I'll figure it out tomorrow... 21dc0f7 is the commit that should fix the runtime DLL issue.

(slightly hacky but I don't see any better way)
@rcurtin rcurtin force-pushed the rcurtin:python-windows-fix branch from 21dc0f7 to 2005a2f Sep 27, 2019
@rcurtin

This comment has been minimized.

Copy link
Member Author

rcurtin commented Sep 27, 2019

Ok, fixed the commit issue; hopefully this will pass the build tests now---we'll see. :)

@rcurtin

This comment has been minimized.

Copy link
Member Author

rcurtin commented Oct 10, 2019

Alright, I think that this is ready for review. There may still be a few small build issues that I have to work out, but both @gmanlan and I have tested this and had success. Once this is merged, I can then start making the mlpack/mlpack-wheels repository build on Windows too, so that we can have mlpack PyPI packages for Windows also.

Copy link
Contributor

gmanlan left a comment

Just a minor comment

Copy link
Contributor

gmanlan left a comment

Just a minor comment

@@ -21,7 +21,12 @@ if (WIN32)
option(BUILD_PYTHON_BINDINGS "Build Python bindings." OFF)
option(BUILD_SHARED_LIBS
"Compile shared libraries (if OFF, static libraries are compiled)." OFF)
message(WARNING "By default Python bindings are not compiled for Windows because they are not known to work. Set BUILD_PYTHON_BINDINGS to ON if you want them built.")
if (NOT BUILD_PYTHON_BINDINGS)
message(WARNING "By default Python bindings are not compiled for Windows because they are not known to work. Set BUILD_PYTHON_BINDINGS to ON if you want them built.")

This comment has been minimized.

Copy link
@gmanlan

gmanlan Oct 10, 2019

Contributor

I think this warning should be deprecated right?

This comment has been minimized.

Copy link
@rcurtin

rcurtin Oct 11, 2019

Author Member

Good point, thanks! Let me fix that.

@gmanlan

This comment has been minimized.

Copy link
Contributor

gmanlan commented Oct 31, 2019

Updates on this one..?

@rcurtin

This comment has been minimized.

Copy link
Member Author

rcurtin commented Nov 1, 2019

Just waiting on a review.

@zoq @ShikharJ @KimSangYeon-DGU @walragatver (or anyone else), any interest in reviewing this one? It should be good to go, just needs some eyes on it to see if anything can be improved in the code. :) (No huge hurry, but I would love to get it into mlpack 3.2.2.)

@zoq

This comment has been minimized.

Copy link
Member

zoq commented Nov 1, 2019

Sorry for the slow response, thought this is still WIP (see title).

@zoq
zoq approved these changes Nov 2, 2019
Copy link
Member

zoq left a comment

Reviewed the code from a plane seat; nice to see that the python bindings working on windows as well, I can just imagine how long it took to figure out all these small details. Anyway, no comments from my side, I also tested the code in a Windows VM and they worked for me as well.

@rcurtin rcurtin changed the title Python Windows fixes (WIP) Python Windows fixes (ready) Nov 2, 2019
@rcurtin

This comment has been minimized.

Copy link
Member Author

rcurtin commented Nov 2, 2019

Thanks---I didn't realize that I had the (WIP) in the name still (I updated it). It definitely took a long time to make it all work but I think it was worth it. :) I wish I could have reviewed code on the plane too but the wifi didn't work for me and I wasn't smart enough to preload some PRs in tabs to review 😠

@mlpack-bot
mlpack-bot bot approved these changes Nov 3, 2019
Copy link

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit d6d163b into mlpack:master Nov 3, 2019
5 of 6 checks passed
5 of 6 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@rcurtin rcurtin added s: fixed and removed s: needs review labels Nov 3, 2019
@rcurtin rcurtin added this to the mlpack 3.2.2 milestone Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.