-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add caffe #256
Add caffe #256
Conversation
16eca4d
to
bc868ae
Compare
Hi! This is the friendly conda-forge-admin automated user. I just wanted to let you know that I linted all conda-recipes in your PR ( |
50f6a17
to
0d3d8bd
Compare
mv "${PREFIX}/python/caffe" "${SP_DIR}/" | ||
find "${PREFIX}/python/" -name "*.py" | xargs chmod +x | ||
find "${PREFIX}/python/" -name "*.py" | xargs -I {} cp {} "${PREFIX}/bin/" | ||
rm -rf "${PREFIX}/python/" |
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.
This is pretty hacky here. However, the install just dumps all of the Python stuff in ${PREFIX}/python
, which is not cool. So, we try to resort it based on where it should have gone. Should work with upstream to fix this.
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.
Point them to cookiecutter: https://github.com/audreyr/cookiecutter
Shouldn't need much work to use that for the drudge work, then just copy their whole folder structure in.
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.
Sure, will keep that in mind. Thanks.
This passes CI. However, would appreciate a review here. |
# Python installation is non-standard. So, we're fixing it. | ||
mv "${PREFIX}/python/caffe" "${SP_DIR}/" | ||
find "${PREFIX}/python/" -name "*.py" | xargs chmod +x | ||
find "${PREFIX}/python/" -name "*.py" | xargs -I {} cp {} "${PREFIX}/bin/" |
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.
Pretty gross that these still have .py
extensions, but are intended to be binaries. Maybe we could strip them?
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.
Doesn't sound unreasonable. It is a trick one though - we are kind of fundamentally tinkering with somebody else's software. What does debian do with caffe?
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.
I don't think it is packaged in Debian or any other Linux distro. This is breaking edge research software for deep learning. Caffe is still doing release candidates. So, I think we are out of luck if we try to look for any kind of canonical installation via Linux package managers. Not to mention, most installation instructions involve a mixture of conda
/apt-get
installs and some sort of build in-place (so no proper install). Many are unofficial guides (e.g. I managed to get it to work like this) that may involve custom build scripts. If anything, we are laying out the canonical install.
Caffe's install instructions actually suggest mucking with the PYTHONPATH
, which feels pretty dirty to me (maybe to you too?). Especially, since putting the Python packages in site-packages
and the binaries in bin
feels so much cleaner and works the same.
I would definitely want to interact with upstream on a better way to do this Python install. Either by using setup.py
or at a bare minimum changing the copy steps in their CMakeLists.txt.
Even though this is pretty rough and hacky, it doesn't seem nearly as bad as the alternatives. That all being said, by even attempting to package this in conda
we are doing a major service to the Machine Learning community that has varying levels of proficiency and interest in package management.
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.
Also, what do you mean by "intended to be binaries"? They are making a bunch of stuff directly executable, but that doesn't make it binary. It is unusual to have python files directly executable like that, but I can remember doing such things as a scientist learning to code.
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.
Also, what do you mean by "intended to be binaries"?
Yeah, this is fun too. They state at the top of these Python files that they are meant for use as CLIs. Take classify.py
, for instance, it has the logic to run under Python on Mac and Linux from the command line and has a note that this is the intent. The rest are the same (though feel free to check). All of the files lack executable permissions, have .py
extensions, lack any real Python callable API (except for main
), and are not placed in the usual bin
directory.
It is unusual to have python files directly executable like that, but I can remember doing such things as a scientist learning to code.
Sure, when I was still learning Python, I did a few things like this. To be honest, they should be lauded for making a Python interface to Caffe and, to be honest, the Python code that I have seen (though haven't seen all of it) looked pretty clean for the most part. It is just their package management side that is a bit weak. Then again this is a weakness of Python too and a bit rough to follow too. So maybe they just thought this would be the easiest way to get it out there.
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.
So, I changed the logic slightly here. Now it will strip the .py
extensions as well so they appear like normal binaries.
- gflags | ||
- glog | ||
- h5py | ||
- hdf5 |
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.
hdf5 is an implicit dependency, I think. the h5py dependency will get it. Only include it if they drectly use the HDF5 API and link against it.
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, I believe they do link against it.
117d192
to
c2d976e
Compare
Just to review, what are the actionables here? Is it just testing the moved python executables? |
That, and maybe pinging the authors to let them know that their software has been packaged and that we'd love to have them in the maintainers team if they are willing. Otherwise, 👍 from me |
Definitely, they already recommend people use Anaconda for the installation anyways. So, they would likely appreciate it the effort put forward here. Figured I'd wait though until we were close to done so as not to increase their noise level. |
2425b7e
to
879b6c4
Compare
Alright, made the final tweaks (testing Python binaries, dropping extensions from them, and making this track the |
@shelhamer @jeffdonahue @longjon, we are adding As I packaged this, I had some ideas about how the Python components could be structured to make their install a bit smoother. If you have some time to chat about this, I would be happy to point you guys in the right. |
Just as FYI, while this does not currently contain GPU support. We are actively exploring this topic in this issue ( conda-forge/conda-forge.github.io#63 ). If anyone has information they are willing to share on the subject of GPU support in general, please feel free to share in that issue. |
@jakirkham I can't promise the bandwidth to take ownership of a packaging effort now, but thanks for including Caffe in the conda forge.
If you have a chance please do post an issue on our github tracker—it's likely that someone will follow up on it. |
@shelhamer, I'm not asking that people from Caffe take ownership of this (not that it would be a problem if they did). I'm interested in keeping this package maintained for my own uses. That being said, I do welcome people from the Caffe project to sign up as maintainers. As maintainers, your level of engagement is completely up to you. Though it would be really helpful just to have the watchful eye of developers that know how this should be built in case we go astray.
Sure, I'll try to come up with some concrete suggestions and add them to an issue on your issue tracker. |
I guess adding support for Windows would be somewhat painful, since it's on a different branch than the rest of the code (https://github.com/BVLC/caffe/tree/windows) and I'm not sure if all the requirements are available |
Not sure. Could you add it as an issue on the feedstock, @ivoflipse? |
@ivoflipse @jakirkham I just added support for building the windows branch with CMake so it should work better with conda build. I even tried to use conda packages to gather all the dependencies and got that to work with my own recipes, but it was a bit of a pain since caffe requires at least VS 2013 and Python 2.7 uses VS 2008. Hopefully we may get this working with Python 3.5 and VS 2015. If any of you want to give this a try I will support you as best as I can. |
Builds caffe from source. Currently, this supports Linux only, but Mac should be feasible. However, this will likely require feature-based BLAS support. Getting Windows support will be challenging as many of the dependencies don't support Windows. Also, this does not enable GPU support. That will likely require modifications to CI packages installed and use of features. Though should be possible. While somewhat limited, this both demonstrates the build is possible and creates a base for building on going forward.