-
Notifications
You must be signed in to change notification settings - Fork 107
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
#809 Switch from Boost Python to PyBind11 #950
Conversation
(#809-pybind11)
@jmeyers314 I think I fixed the issues you were having. Could you try again with the current version? |
Thanks @rmjarvis . Unfortunately, that still didn't work; I got the same I went ahead and tried (and committed) making libc++ the default stdlib for clang, and that seems to work for my system (can build and all tests pass). Definitely feel free to revert or clean this up though if you see a more elegant solution or if I broke anything for other systems. |
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.
Looks good to me. There are a few things broken in the markdown files that I noted, and a couple other questions. I managed to build and test with setup.py, and also with scons, using all 4 variations of EIGEN/TMV + BOOST/PYBIND11 (with one minor change to the PyBind11Helper.h file), so that's nice.
I think it would be nice if we could add some of these variations to our Travis-CI build matrix. Is that possible?
Also, since this is the precursor to our next release, I also ran all the lsst_wcs tests that normally get skipped. It looks like these have bitrotted, at least with respect to the master branch of the LSST stack. (Actually, I think they may have been broken since Sep 2016). But I'll file that in a separate issue; no reason to hold this up.
INSTALL.md
Outdated
|
||
|
||
0. Overall summary |
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.
Something's broken with the internal links and headers here (See https://github.com/GalSim-developers/GalSim/blob/%23809-pybind11/INSTALL.md). (True on master branch too).
INSTALL.md
Outdated
root, of course: `sudo ./b2`... Also, you should be aware that if you are | ||
running `b2` a second time, you should use `b2 -a` to tell boost to | ||
recompile everything rather than use the existing libraries. | ||
- Eigen (3.2.8) (via eigency 1.77) |
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.
Eigen 3.2.5 works for me. Maybe reduce this version requirement to that?
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 tried to say that these weren't a minimum required version, but sure, I'll downgrade the requirements to the earlier versions that you tested with.
INSTALL.md
Outdated
running `b2` a second time, you should use `b2 -a` to tell boost to | ||
recompile everything rather than use the existing libraries. | ||
- Eigen (3.2.8) (via eigency 1.77) | ||
- NumPy (1.14.0) |
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.
1.13.1 works for me.
INSTALL.md
Outdated
to them during the build. We require version 3 (or greater presumably), which | ||
is often distributed as fftw3. See Section 5 for some suggestions about | ||
installing this on your platform. | ||
python setup.py install |
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.
Maybe add
git clone [email protected]:GalSim-developers/GalSim.git
cd GalSim
INSTALL.md
Outdated
- Eigen (3.2.8) (via eigency 1.77) | ||
- NumPy (1.14.0) | ||
- Future (0.16.0) | ||
- Astropy (2.0.3) |
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.
2.0.1
INSTALL.md
Outdated
|
||
* `FFTW_DIR`: Explicitly give the FFTW prefix | ||
Probably, you should put this into your .bash_profile file so it always gets |
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.
"shell login file" maybe?
INSTALL.md
Outdated
code information in the compiled library (-g3 for g++ compiler). This | ||
increases the size of the compiled library, but makes debugging with things | ||
like gdb easier. Probably end users will never need to use this. | ||
By default, the python tests will use the pytest plugins `pytest-xdist` (for |
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.
Should probably mention pip install -r test_requirements.txt
here.
INSTALL_SCONS.md
Outdated
|
||
|
||
1. Software required before building GalSim | ||
=========================================== |
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.
===== sectioning in markdown seems to be broken here too (https://github.com/GalSim-developers/GalSim/blob/%23809-pybind11/INSTALL_SCONS.md)
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 think the numbering is what messed it up. It looks like it tried to make a numbered list rather than treat it as a heading. Probably an ambiguity in Markdown's specs and GitHub's implementation changed at some point since we first wrote this.
INSTALL_SCONS.md
Outdated
* `TMV_DIR`: Explicitly give the TMV prefix | ||
* `EIGEN_DIR`: Explicitly give the Eigen prefix | ||
|
||
# `USE_BOOST`: Specify that you want to use Boost rather than PyBind11. |
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.
# -> * ?
pysrc/PyBind11Helper.h
Outdated
// macros allow us to write code that works for either boost python or pybind11. | ||
|
||
// First some things where the boost equivalent of some pybind11 function is different: | ||
#define PYBIND11_MODULE(x,x) BOOST_PYTHON_MODULE(x) |
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.
My compiler didn't like this macro (duplicate variable). Changing it to
PYBIND11_MODULE(x,y) BOOST_PYTHON_MODULE(x)
seemed to work. But this is well outside my expertise so I'll let you decide...
Thanks Josh. I fixed up the markdown. The links are all working now. Just had to remove the numbering from the section headings. Also made the other adjustments you recommended. |
I looked into this a bit, and yes, it's possible. But I think I'd rather not. I'd rather have us just run a few of the scons installations before each release to make sure things still work rather than have travis run these for every PR. One nice feature of the new installation is that it is faster, especially when the cache isn't populated. So if we keep the boost/tmv tests on travis, we'll keep having to do those annoying "disable tests to repopulate cache" "git revert HEAD" cycles, which I don't think will be required for the setup.py installations. |
Let's merge this Friday if there are no objections. We'll want to ask more people to try installing using pip, but we can wait until 1.6 is ready, so we can have 2.0 released at the same time. |
Here is the final PR for issue #809. The last few things needed to finally make GalSim pip installable are:
Now the only non-pip-installable dependency is fftw, and if you are using conda, then even that one is conda-installable, so for most users it's not too hard.
I'd like as many users as possible to try out the new installation procedure and let me know any problems you have with it. I fully expect there to be some issues as we try this on a range of systems. So far, it works for me on my laptop (OSX 10.10), my wife's laptop (OSX 10.12), nersc (cori and edison), the BNL astro cluster, Travis, and a unix cluster at UPenn. But there are lots of other systems out there that you all use, so I expect there to be issues still... :)
Full explanation of the new installation process is in the INSTALL.md file. But here is a summary.
For conda users, the installation process is (from the GalSim root directory):
If you don't use conda, then you are required to have a modern enough compiler that it can handle the c++11 standard (or newer). gcc 4.8 or later will work. You also need fftw installed somewhere appropriate (cf. instructions in INSTALL.md for details). Then
(You might need to either precede these with sudo or follow them with --user.)
Then to run the tests, do
A few further comments:
pip install galsim
work), but I will do that soon. Probably after whatever early rounds of trouble-shooting are required from people trying the above methods.scons install
now by default uses Eigen and PyBind11. That may be a more productive workflow than using setup.py during development, so I didn't get rid of it. We'll have to see.scons USE_BOOST=true
.scons USE_TMV=true
.import galsim; galsim.__file__
will tell you which file is being imported in python.