-
Notifications
You must be signed in to change notification settings - Fork 613
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
Build Addons for Windows (CPU) #771
Conversation
a04fed8
to
4cb1cba
Compare
364bfab
to
5c95b20
Compare
5cab8b1
to
00c7a42
Compare
@tensorflow/sig-addons-maintainers ready for review. Built with travis and pushed to pypi test: No py27 package since theres no binary for TF2.1.0rc1 but it should be there for the full release. Tested on windows VM:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the effort on build stuff!
@@ -1,5 +1,3 @@ | |||
workspace(name = "tensorflow_addons") | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we get rid of the workspace name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so I tried for a while to get this to work. Windows seemed unwilling to copy the files during build_pip_pkg
if the directory was specified using workspace. Possibly related to windows sym-links but I'm not really sure. It's worth figuring out, but I've had about as much of windows build as I can stand for a while :P
The workspace name is something I added during CUDA10.1 PR and isn't being done for custom-op:
https://github.com/tensorflow/custom-op/blob/master/WORKSPACE
@@ -38,7 +38,7 @@ struct Rrelu { | |||
typename TTypes<T>::Tensor alpha, | |||
typename random::SimplePhilox& random) { | |||
if (training) { | |||
T storage[alpha.size()]; | |||
T* storage = new T[alpha.size()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @fsx950223
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Let's get this merged and deal with other issues later :D
Fixes #173
TODO (Future Issues):