search

NSLS-II / docs

Standards Documentation for NSLS-II DAQ and Analysis
https://nsls-ii.github.io
BSD 2-Clause "Simplified" License
2 stars 13 forks source link

WIP: Restore tomopy example. #112

Open danielballan opened 5 years ago

danielballan commented 5 years ago

Our procedure for installing Tomopy on Travis has been broken by changes on Tomopy's side that we have yet to identify. From some local testing it looks like they are tightly bound to conda these days---they are not on PyPI and both the dev and user installation instructions involve conda. They exact cause of the issue seems to be a hard dependency on MKL, which is why I have started by trying to install MKL using apt.

Unclear how much work this will be to fix, but it does not seem trivial (for me).

mrakitin commented 5 years ago

Ehh, there is another compilation issue:

    CMake Error: CMake was unable to find a build program corresponding to "Ninja".  CMAKE_MAKE_PROGRAM is not set.  You probably need to select a different build tool.

Also, in my local tests I saw it required a version of cmake 3.x+. Maybe it should be installed as well.

mrakitin commented 5 years ago

The solutions I see:

Thoughts?

danielballan commented 5 years ago

I don't love the idea of adding Docker to this repo for the sake of just one cookbook example. Conda is an easier sell, but I'm still reluctant: ever since we switch our builds from conda to pip, things have been a lot more reliable.

I wonder if we can enlist some help from the tomopy developers to build this properly?

danielballan commented 5 years ago

@dgursoy Any thoughts on what changed in Tomopy that our procedure for installing on Travis no longer works? As you might clean from the conversation above, I'm interested in installing tomopy without adding Docker or conda to the mix, if possible.

dgursoy commented 5 years ago

We have always been relying on conda for distribution and we didn’t encounter any major issue.

So do you want to build it from source and manually install all dependencies by yourself?

Maybe we can relax the hard dependency of MKL to a soft one, if this is the source due to a conflict of versions. Any comments @carterbox?

dgursoy commented 5 years ago

Also looping in @jrmadsen, who has been active in improvements in packaging.

jrmadsen commented 5 years ago

MKL is a hard dependency due to gridrec. It used to be soft because of fftw but that was so uncommonly used, it was broken for quite a while before anybody noticed. I would be fairly easy to disable gridrec if MKL is not found though if there is interest in that...

Technically, conda is still not a requirement (ASFAIK), it is just recommended. I can't think of any reason why the new changes would break using pip once scikit-build is added.

CMake Error: CMake was unable to find a build program corresponding to "Ninja". CMAKE_MAKE_PROGRAM is not set. You probably need to select a different build tool.

@mrakitin This surprises me, my understanding is that scikit-build prefers ninja but if that is not found, it should fall back to Unix Makefiles. If you have make installed, this is probably a bug in scikit-build and you should file an issue on their github. Also, you can install ninja via apt with apt-get install ninja-build, or you could try running python setup.py install -- -DCMAKE_MAKE_PROGRAM=$(which make) or python setup.py install -- -G Unix\ Makefiles. If none of these solutions work, I could get look adding a patch (or fixing the bug) to scikit-build itself to get it to fall back to CMake generating Makefiles but I would kindly request you try the aforementioned options.

carterbox commented 5 years ago

The Issue where I proposed removing a bunch of dependencies (including FFTW) is here: https://github.com/tomopy/tomopy/issues/377.

@jrmadsen Their build is not failing due to Ninja missing. It does fall back to the Unix Makefiles (so there's no bug with scikit-build). It fails when CMake doesn't find OpenCV. I would like to discuss making both MKL and OpenCV optional build requirements and disable the relevant algorithms (with an error message) if TomoPy is not build with all options. This relates to thttps://github.com/tomopy/tomopy/issues/422. Hopefully we can come up with one system to consistently handle this type of thing for all these cases.

@danielballan To fix the current build, I think you should only need to install MKL and OpenCV.

mrakitin commented 5 years ago

@dgursoy, @jrmadsen, @carterbox, thank you for your feedback! We are pretty new to the cmake world, so it's a good opportunity for us to learn more about this powerful tool. Thank you again for your suggestions. We will try to install OpenCV (MKL is already being installed by @danielballan's updates). Is it a good set of instructions -- https://www.learnopencv.com/install-opencv-3-4-4-on-ubuntu-18-04/?

jrmadsen commented 5 years ago

@mrakitin Yes that looks fine, although it may be unnecessary. We don't really have a hard dependency on a minimum OpenCV version so you should just be able to apt-get install libopencv-dev

danielballan commented 5 years ago

Great, we’ll try that. Thanks for quick response, all!