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

Scikit buildsystem #38

Merged
merged 14 commits into from
Jan 18, 2019
Merged

Scikit buildsystem #38

merged 14 commits into from
Jan 18, 2019

Conversation

repagh
Copy link
Member

@repagh repagh commented Aug 21, 2018

After many iterations, a better set of changes for using scikit-build.
Works with conda build and with direct python/pip builds

This still contains a workaround for the lack of recent (>= 0.6.1) scikit-build on conda-forge. Directly installs & uninstall scikit-build using pip as part of a conda build. Can be simplified later.

Recent scikit build (0.8.1) available both on conda-forge and pip correctly build this.

@repagh repagh mentioned this pull request Aug 21, 2018
@moorepants
Copy link
Contributor

FYI, this will hopefully be merged today: conda-forge/scikit-build-feedstock#11

@moorepants
Copy link
Contributor

FYI, conda forge now has scikit-build 0.8.0.: conda-forge/scikit-build-feedstock#14

@repagh
Copy link
Member Author

repagh commented Aug 22, 2018

Yes, works. Am using the conda-forge scikit-build now, with a conda install for CONDA=0, and through the meta.yaml for CONDA=1

@repagh
Copy link
Member Author

repagh commented Sep 5, 2018

@murrayrm , @moorepants, can this be merged? It uses the scikit-build currently in conda-forge (no longer needs the hack), builds and runs successfully on Linux, OSX, and Windows.

@roryyorke
Copy link
Collaborator

I can't get this to build on Ubuntu 18.04 64-bit. Conda version 4.5.11, conda build 3.16.0. Build command conda build -c conda-forge --python=3.6 slycot/conda-recipe-openblas (from directory containg slycot). The error I get is lengthy, but I think the important bit is

  File "/home/rory/.miniconda3/conda-bld/slycot_1539023903021/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_plac/lib/python3.6/site-packages/skbuild/command/bdist_wheel.py", line 6, in <module>
    from wheel import archive as _wheel_archive
ImportError: cannot import name 'archive'

I've attached a complete build log. Any ideas what I'm doing wrong?
build-3.6.log

@moorepants
Copy link
Contributor

@murrayrm , @moorepants, can this be merged? It uses the scikit-build currently in conda-forge (no longer needs the hack), builds and runs successfully on Linux, OSX, and Windows.

LGTM

@moorepants
Copy link
Contributor

Actually I have one question. Since setup.py has changed and now requires scikit build, this needs to be made an explicit build dependency of the package. And will it work without using conda and conda-forge?

@murrayrm
Copy link
Member

murrayrm commented Oct 9, 2018

I'd also like to test this on MacOS and make sure that it still works. Has anyone tried it?

@roryyorke
Copy link
Collaborator

And today it builds. conda-build 3.16.1, and scikit-build 0.8.1 (previously 0.8.0). I've got the build log if anyone's interested.

@@ -5,7 +5,7 @@
Slycot wraps the SLICOT library which is used for control and systems analysis.

"""
from __future__ import division, print_function
from skbuild import setup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The installation documentation will need to let users know of this new requirement, as well as cmake (and any other build tools that now need to be installed). I personally think that skbuild should be an optional requirement because a user can build it without this if they so desire.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added information on scikit-build and cmake requirements to the readme file.

I don't think it is feasible to maintain two buildsystems. I got to use cmake/skbuild after repeatedly trying to use and tweak the numpy distutils, however I found no way to specify flang to the distutils buildsystem, while flang works wonderfully for Windows builds.

The buildsystem does not rely on conda; if skbuild and cmake are available, local builds work fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, seems reasonable.

@roryyorke
Copy link
Collaborator

this had a failure on Dec. 8 https://travis-ci.org/python-control/Slycot/jobs/465308309 ; I reran one case (Python 3.6, TEST_CONDA=1) today without changing anything, and it passed https://travis-ci.org/roryyorke/Slycot/builds/473027170. Will try a rebuild (feels a bit pointless, but what the heck...).

According to [1], use Miniconda2-latest-Linux-x86_64.sh.  This fixes a build bug on Travis.

[1] https://conda.io/docs/user-guide/tasks/use-conda-with-travis-ci.html, retrieved 2018-12-28.
@repagh
Copy link
Member Author

repagh commented Jan 7, 2019

There seems to be something wrong with the conda-build for python 2.7. Have not found a fix for that yet.

@roryyorke
Copy link
Collaborator

Doesn't repagh/Slycot#2 fix the Python 2 build? See https://travis-ci.org/roryyorke/Slycot/builds/473091198

@repagh
Copy link
Member Author

repagh commented Jan 10, 2019

Doesn't repagh#2 fix the Python 2 build? See https://travis-ci.org/roryyorke/Slycot/builds/473091198

Good catch. It now works!

@roryyorke
Copy link
Collaborator

If there are no objections to this in the next week, I propose merging to master.

In favour:

  • possible to build on Windows; hopefully we can add Windows packages to conda-forge (@moorepants, is this practical?)
  • better build output; we don't have that 1000 (or whatever) character link command appearing in the build anymore (this is fairly minor)

Two downsides:

  • scikit-built doesn't support building wheels
  • scikit-build also doesn't support "setup.py develop" mode

@moorepants
Copy link
Contributor

is this practical?

I am not quite sure. As far as I know things that rely on Fortran are not possible to compile on Windows with the conda forge system. But maybe things have changed in the last months. The last I could understand was that flang might be the only way forward there.

@moorepants
Copy link
Contributor

Looks like numpy is built for all OSs on conda forge, but scipy isn't build on windows for some reason. It maybe be that there is still some hurdle.

@moorepants
Copy link
Contributor

It looks like the numpy feedstock installs from the numpy wheel on pypi (which is build with ATLAS on windows) and doesn't actually build on appveyor. The best bet would be to build slycot wheels for windows manually and then have the conda forge recipe install them.

@ilayn
Copy link

ilayn commented Jan 11, 2019

Since v.1.0 we are able to build SciPy on Windows via OpenBLAS. It is still definitely not a walk in the park to build the BLAS of choice but if you just use a prebuilt BLAS flavor it is quite possible (I will modify the notes with a correction soon scipy/scipy#9669).

@moorepants
Copy link
Contributor

It'd be great to get that working on conda forge so that other packages that need to build on windows would have a working example.

@repagh
Copy link
Member Author

repagh commented Jan 14, 2019

I use a pretty bare Windows (in a virtualbox) to build the conda packages with scikit, with the following manually installed:

  • Installation of msvc 2015 for the c parts
  • miniconda3
  • git, but only for getting the software

For the build this uses the conda-forge flang (available win64 only), cmake, scikit-build, etc. I don't know what the conda-forge windows buildsystem looks like, but I would guess this would be compatible.

p.s. python setup.py bdist_wheel simply works. I tried that against the openblas from conda-forge, you would need to have a wheel-based openblas to use it outside the conda ecosystem.

@moorepants
Copy link
Contributor

As soon as this is merged we can give it a shot on conda-forge and see what happens.

@roryyorke
Copy link
Collaborator

Sorry, I thought I saw an issue about scikit-build and wheels, but the only one I can find now is closed.

I may be wrong about "setup.py develop" too; see, e.g., scikit-build/scikit-build#371

@roryyorke roryyorke merged commit b82299b into python-control:master Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants