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

More robust build-catalogue #1784

Merged

Conversation

thorstenhater
Copy link
Contributor

@thorstenhater thorstenhater commented Nov 30, 2021

Introduction

Make build-catalogue (a-b-c) a better tool, it is now used in production quite seriously, so
we need to match up with expectations.

  • enable GPU-backends
  • isolate from being installed in a non-default dir
  • rename to arbor-build-catalogue
  • enable PIC on arbor always to allow linking to shared objects
  • ensure a-b-c is installed by pip
  • throw errors if tools (cmake scripts, arbor package, modcc) missing
  • while we are at it, also allow hand-written C++ mechanisms to be linked in the same catalogue.
  • allow raw mechanisms to be mixed in with NMODL.
    • 'raw' here means 'written directly against the ABI'

Changes

  • transition setup.py setuptools -> skbuild.
  • use relative paths when configuring the script. This will work even when copying the install tree around as long as the relative locations do not change.

Open Issues

Linked Issues

Fixes #1782
Fixes #1776
Fixes #1775
Fixes #1779
Fixes #1792

@thorstenhater thorstenhater marked this pull request as draft November 30, 2021 13:36
pyproject.toml Outdated Show resolved Hide resolved
@brenthuisman
Copy link
Contributor

So it wouldn't fix #1779?

@thorstenhater
Copy link
Contributor Author

thorstenhater commented Dec 1, 2021

No. It's unrelated.

Now it does.

@brenthuisman
Copy link
Contributor

Intermediate result regarding the ciwheel action:

  1. a CMAKE error involving libxml2 not being found presents itself
  2. succesfully worked around by disabling nml. Functional manywheels will be output.
  3. Verified that old setup.py (it hadn't ran since v0.5.2 release) also still works, with nml support.
    So, it is still unclear why changing from setuptools to scikit-build causes CMAKE to stop finding libxml2 when ran in the PyPA image. TODO: run in podman locally, easier to debug.

Copy link
Collaborator

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

Could you explain how scikit-build picks up the parts?

setup.py Outdated Show resolved Hide resolved
@llandsmeer
Copy link
Contributor

This looks great!

I tried out the patch w.r.t. #1775. However, I ran into some problems. Did the syntax for command line options change?

$ python3 setup.py install --user --makejobs=8
/home/llandsmeer/.local/lib/python3.8/site-packages/setuptools/dist.py:691:
UserWarning: Usage of dash-separated 'description-file' will not be supported
in future versions. Please use the underscore name 'description_file' instead
  warnings.warn(
usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

error: option --makejobs not recognized

Asking for --help did not show any solutions. Running without --makejobs seemed to work and installed /home/llandsmeer/.local/lib/python3.8/site-packages/arbor-0.5.3.dev0-py3.8-linux-x86_64.egg/bin/arbor-build-catalogue which means that it solves #1775 for me.
And indeed I had to use pip3 install scikit-build

@thorstenhater
Copy link
Contributor Author

From the skbuild docs, I infer that --makejob is -j here, similar to raw make.

@thorstenhater
Copy link
Contributor Author

About the skbuild dependency, it should be picked up by pip, eg when running pip install . via pyproject.toml. For everybody else, I'll add it to the docs.

@llandsmeer
Copy link
Contributor

llandsmeer commented Dec 6, 2021

Great that fixed the makejobs problem. And the other options? I also cant get --gpu=cuda or --mpi to work

@thorstenhater
Copy link
Contributor Author

Ok, I'll take a look.

@brenthuisman
Copy link
Contributor

ATM, passing flags to setup.py does not work because the setuptools commands that used to process them have not yet been translated into however scikit-build lets you set them. The defaults in class CL_opt now determine the build flags.

Before I dive into that (and before @thorstenhater does ;)), I think it's best to figure out how to fix the out-of-tree builds thing. Apparently solving this is a very recent development, as the struggles by a PyPA maintainer show: pypa/pip#10573 (comment) .

I'm trying to get to a proper scikit-build-ified setup.py and see if that helps, if so, we shall add the options back in.

@brenthuisman
Copy link
Contributor

brenthuisman commented Dec 6, 2021

Note: for out of tree builds:

  • pip install ./path/to/this/branch/of/arbor/ builds, using ninja, through an intermediate build dir in /tmp.
    • Works because it changes dir to $tmpdir, thus CMakeLists.txt is found.
  • python3 ./path/to/this/branch/of/arbor/setup.py install does not (skipping skbuild (no CMakeLists.txt found)).
    • It's a problem because people will unfortunately use setup.py this way, but also because it's the only way I know of to build wheels, setup.py bdist_wheel.

Asked for some help: scikit-build/scikit-build#614

@Helveg
Copy link
Collaborator

Helveg commented Dec 6, 2021

@brenthuisman pip now has a PEP517/518 compliant wheel builder! You can use pip wheel! https://pip.pypa.io/en/stable/cli/pip_wheel/

@thorstenhater
Copy link
Contributor Author

The build options can be worked around using this

pip install . -- -DARB_USE_BUNDLED_LIBS=ON -DARB_WITH_MPI=ON -DARB_GPU=cuda

it's not the most convenient way, but it work.

doc/install/python.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@noraabiakar noraabiakar left a comment

Choose a reason for hiding this comment

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

Just a few more comments and requests, but otherwise it looks good, thanks!

doc/install/python.rst Outdated Show resolved Hide resolved
doc/install/python.rst Outdated Show resolved Hide resolved
doc/install/python.rst Outdated Show resolved Hide resolved
doc/install/python.rst Outdated Show resolved Hide resolved
doc/install/python.rst Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
scripts/build-catalogue.in Outdated Show resolved Hide resolved
scripts/patchwheel.py Outdated Show resolved Hide resolved
@thorstenhater
Copy link
Contributor Author

Hold off on that merge ... there might something amiss in the docs. Taking a look.

@llandsmeer
Copy link
Contributor

I tried using the pip install method in google colab, which did not work:

To reproduce, create a cell with the following contents and execute it:

!pip3 install ninja numpy cmake setuptools
!test -d arbor || git clone --depth 1 --recursive https://github.com/arbor-sim/arbor
!test -f 1784.diff || wget 'https://github.com/arbor-sim/arbor/pull/1784.diff'
!git -C arbor apply --reverse --check ../1784.diff 2> /dev/null ||\
     git -C arbor apply ../1784.diff
!pip3 install ./arbor --install-option="-DARB_GPU=cuda"

which will error after a few minutes. Running plain cmake && make results in

/content/arbor/arbor/mechcat.cpp: In member function ‘arb::hopefully<arb::derivation> arb::catalogue_state::derive(const string&, const string&, const std::vector<std::pair<std::__cxx11::basic_string<char>, double> >&, const std::vector<std::pair<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> > >&) const’:
/content/arbor/arbor/mechcat.cpp:333:16: error: could not convert ‘deriv’ from ‘arb::derivation’ to ‘arb::hopefully<arb::derivation> {aka arb::util::expected<arb::derivation, std::__exception_ptr::exception_ptr>}’
         return deriv;

As this is the first time I'm using google colab to build arbor this might be more a problem with google colab than this PR

@thorstenhater
Copy link
Contributor Author

I tried using the pip install method in google colab, which did not work:

To reproduce, create a cell with the following contents and execute it:

!pip3 install ninja numpy cmake setuptools
!test -d arbor || git clone --depth 1 --recursive https://github.com/arbor-sim/arbor
!test -f 1784.diff || wget 'https://github.com/arbor-sim/arbor/pull/1784.diff'
!git -C arbor apply --reverse --check ../1784.diff 2> /dev/null ||\
     git -C arbor apply ../1784.diff
!pip3 install ./arbor --install-option="-DARB_GPU=cuda"

which will error after a few minutes. Running plain cmake && make results in

/content/arbor/arbor/mechcat.cpp: In member function ‘arb::hopefully<arb::derivation> arb::catalogue_state::derive(const string&, const string&, const std::vector<std::pair<std::__cxx11::basic_string<char>, double> >&, const std::vector<std::pair<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> > >&) const’:
/content/arbor/arbor/mechcat.cpp:333:16: error: could not convert ‘deriv’ from ‘arb::derivation’ to ‘arb::hopefully<arb::derivation> {aka arb::util::expected<arb::derivation, std::__exception_ptr::exception_ptr>}’
         return deriv;

As this is the first time I'm using google colab to build arbor this might be more a problem with google colab than this PR

From private conversation:

  • colab seems to use CXX=GCC 7.4, which is too old
  • pip fails to resolve dependencies, could also be caused by dated versions, here's an excerpt from the error

    pip._internal.exceptions.InstallationError: Could not build wheels for cmake, ninja which use PEP 517 and cannot be installed directly
    ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts. datascience 0.10.6 requires folium==0.2.1, but you have folium 0.8.3 which is incompatible.

This seems to be a collusion of dated tools and pre-installed dependencies. While it would be nice to have Arbor+GPU in the cloud this way, we need to compile on site for that.

NB @brenthuisman @Helveg @thorstenhater @llandsmeer if there is sufficient interest and motivation, let us set up a tutorial/image/script to make Arbor+GPU happen on colab

@brenthuisman
Copy link
Contributor

Removing pyproject.toml appears essential for correct builds with --install-options. A side-effect is that users now must manually install a few packages (e.g. skbuild). Should we add these to setup(install_requires=['numpy'])?

@brenthuisman
Copy link
Contributor

brenthuisman commented Jan 24, 2022

TODO:

  • Fixup conflicts [TH]
  • Run bors [TH]
  • Later: add pip build GA workflow. [BH]

@thorstenhater
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jan 24, 2022
@bors
Copy link

bors bot commented Jan 24, 2022

try

Build failed:

@thorstenhater
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jan 24, 2022
@bors
Copy link

bors bot commented Jan 24, 2022

try

Build succeeded:

@thorstenhater thorstenhater merged commit d0dc6bd into arbor-sim:master Jan 24, 2022
max9901 pushed a commit to max9901/arbor that referenced this pull request Feb 3, 2022
- build-catalogue
  - now installed by pip
  - enable GPU-backends
  - isolate from being installed in a non-default dir
  - rename to arbor-build-catalogue
  - allow hand-written C++ mechanisms to be linked in the same catalogue.
- CMake
  - enable PIC on arbor always to allow linking to shared objects
  - use relative paths when configuring a-b-c, to make relocation less of a problem
- Python
    - ensure a-b-c is installed by pip, along with headers and libarbor.a
    - throw errors if tools (cmake scripts, arbor package, modcc) missing
    - transition setup.py setuptools -> skbuild.
- Wheels
    - Add NML2 support to wheels
    - scripts/build-wheels.sh builds wheels, in principle valid for submission to PyPI, on your own hardware. This should be kept in sync with .github/workflows/ciwheel.yml
    - scripts/patchwheel.py corrects the rpath in the libraries in the wheels, working around a bad interplay between auditwheel and skbuild, see pypa/auditwheel#363
    - Python Wheels are tested as part of the Github Action 
    - Add nml and bundled status to config().
@thorstenhater thorstenhater deleted the bug/arbor-build-catalogue branch August 1, 2022 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants