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

Add install instructions for editable Python installation #721

Closed
wants to merge 3 commits into from

Conversation

jorgensd
Copy link
Member

@jorgensd jorgensd commented Oct 27, 2023

Tested with:

FROM ubuntu:22.04

ENV DEBIAN_FRONTEND=noninteractive
ARG TARGET_PLATFORM
RUN apt-get update && \
    apt-get install -y software-properties-common
RUN apt-get install -y git \
    cmake \
    gcc \
    g++ \
    ninja-build \
    libopenblas-dev \ 
    liblapack-dev
ENV DEB_PYTHON_INSTALL_LAYOUT=deb_system
RUN apt-get install -y python3-pip
RUN python3 -m pip install -U setuptools pip
RUN python3 -m pip install git+https://github.com/FEniCS/ufl.git

ENV CMAKE_BUILD_PARALLEL_LEVEL 2
WORKDIR /src
RUN git clone --branch=dokken/basix-install-instructions --single-branch https://github.com/FEniCS/basix.git
WORKDIR /src/basix
RUN cmake -G Ninja -B build-dir -DCMAKE_BUILD_TYPE=Debug -S cpp/ && \
    cmake --build build-dir && \ 
    cmake --install build-dir

WORKDIR /src/basix/python
RUN python3 -m pip -v install -r build-requirements.txt
RUN python3 -m pip -v install --config-settings=build-dir="build" --config-settings=cmake.build-type="Debug" --no-build-isolation -e .

WORKDIR /tmp
RUN python3 -c "import basix"

and

FROM ubuntu:22.04

ENV DEBIAN_FRONTEND=noninteractive
ARG TARGET_PLATFORM
RUN apt-get update && \
    apt-get install -y software-properties-common
RUN apt-get install -y git \
    cmake \
    gcc \
    g++ \
    ninja-build \
    libopenblas-dev \ 
    liblapack-dev
ENV DEB_PYTHON_INSTALL_LAYOUT=deb_system
RUN apt-get install -y python3-pip
RUN python3 -m pip install -U setuptools pip
RUN python3 -m pip install git+https://github.com/FEniCS/ufl.git

ENV CMAKE_BUILD_PARALLEL_LEVEL 2
WORKDIR /src
RUN git clone --branch=dokken/basix-install-instructions --single-branch https://github.com/FEniCS/basix.git
WORKDIR /src/basix
RUN cmake -G Ninja -B build-dir -DCMAKE_BUILD_TYPE=Debug -S cpp/ && \
    cmake --build build-dir && \ 
    cmake --install build-dir

WORKDIR /src/basix
RUN python3 -m pip -v install -r build-requirements.txt
RUN python3 -m pip -v install --config-settings=build-dir="build" --config-settings=cmake.build-type="Debug" --no-build-isolation -e .

WORKDIR /tmp
RUN python3 -c "import basix"

Resolves #719

python/README.rst Outdated Show resolved Hide resolved
@jorgensd jorgensd mentioned this pull request Oct 27, 2023
python/README.rst Outdated Show resolved Hide resolved
Copy link
Member

@garth-wells garth-wells left a comment

Choose a reason for hiding this comment

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

See earlier PR - #720.

Co-authored-by: Jørgen Schartum Dokken <dokken@simula.no>
@jhale
Copy link
Member

jhale commented Oct 27, 2023

I think this PR is a little more complete than #720

@jorgensd
Copy link
Member Author

See earlier PR - #720.

720 is not complete as it doesn't specify the build requirements.

@francesco-ballarin
Copy link
Member

francesco-ballarin commented Oct 27, 2023

As user, I struggle to understand* why build isolation is required with editable installs, but not with standard ones. Maybe the reason for this is worth mentioning in the README?

* I do understand FEniCS/dolfinx#2707 instead, in the sense that with build isolation even the standard install would fail to locate mpi4py and petsc4py.

@garth-wells
Copy link
Member

I think this PR is a little more complete than #720

It has too much text. Too much to read and too much that can go out of date. The instructions are for developers, who can do a bit of work themselves.

@jorgensd
Copy link
Member Author

I think this PR is a little more complete than #720

It has too much text. Too much to read and too much that can go out of date. The instructions are for developers, who can do a bit of work themselves.

I dont this this is fair. It is far from trivial to get this to work. The other PR doesn’t even have valid instructions, as it is missing pins and build dependencies

@jorgensd
Copy link
Member Author

As user, I struggle to understand* why build isolation is required with editable installs, but not with standard ones. Maybe the reason for this is worth mentioning in the README?

* I do understand FEniCS/dolfinx#2707 instead, in the sense that with build isolation even the standard install would fail to locate mpi4py and petsc4py.

I'm not sure what the cause of the issue is, but if you don't use build-isolation, you get the following error when import basix:
``bash

[14/14] RUN python3 -c "import basix":
0.408 :241: RuntimeWarning: nanobind: type 'LatticeType' was already registered!
0.408
0.409 :241: RuntimeWarning: nanobind: type 'LatticeSimplexMethod' was already registered!
0.409
0.410 :241: RuntimeWarning: nanobind: type 'PolynomialType' was already registered!
0.410
0.410 :241: RuntimeWarning: nanobind: type 'MapType' was already registered!
0.410
0.410 :241: RuntimeWarning: nanobind: type 'SobolevSpace' was already registered!
0.410
0.410 :241: RuntimeWarning: nanobind: type 'QuadratureType' was already registered!
0.410
0.411 :241: RuntimeWarning: nanobind: type 'CellType' was already registered!
0.411
0.411 :241: RuntimeWarning: nanobind: type 'ElementFamily' was already registered!
0.411
0.411 :241: RuntimeWarning: nanobind: type 'FiniteElement' was already registered!
0.411
0.412 :241: RuntimeWarning: nanobind: type 'LagrangeVariant' was already registered!
0.412
0.412 :241: RuntimeWarning: nanobind: type 'DPCVariant' was already registered!
0.412
0.412 :241: RuntimeWarning: nanobind: type 'PolysetType' was already registered!
0.412
0.413 Traceback (most recent call last):
0.413 File "", line 1, in
0.414 File "/src/basix/python/basix/init.py", line 7, in
0.414 from . import cell, finite_element, lattice, polynomials, quadrature, sobolev_spaces, variants
0.414 File "/src/basix/python/basix/finite_element.py", line 3, in
0.415 from ._basixcpp import ElementFamily as _EF
0.415 ImportError: cannot import name 'ElementFamily' from 'basix.finite_element._basixcpp' (/usr/lib/python3/dist-packages/basix/_basixcpp.cpython-310-x86_64-linux-gnu.so)
0.419 free(): invalid pointer
0.507 Aborted (core dumped)


@francesco-ballarin
Copy link
Member

I'm not sure what the cause of the issue is, but if you don't use build-isolation, you get the following error when import basix:

I guess that my question is more: how come that pip install . (without -e) works correctly?

@jhale
Copy link
Member

jhale commented Oct 27, 2023

@francesco-ballarin It is a bug in scikit-build-core latest version for editable builds that will be fixed upstream at some point.

@jorgensd
Copy link
Member Author

@francesco-ballarin It is a bug in scikit-build-core latest version for editable builds that will be fixed upstream at some point.

Note that even pining scikit-build-core doesn’t help wrt removing build isolation. So there are two scikit bugs here:

  1. Need build isolation for editable install
  2. Import issues with relative paths and editable install scikit-build/scikit-build-core#532

@jorgensd
Copy link
Member Author

A compromise would be to use split install in one of the CIs, as it would then be easily maintainable and easily accessed by developers.

@mscroggs
Copy link
Member

split install

I think that this run on the CI is trying to do this, but perhaps needs updating? Or do you mean something else by split install?

@jorgensd
Copy link
Member Author

split install

I think that this run on the CI is trying to do this, but perhaps needs updating? Or do you mean something else by split install?

It does a split install, but not a split editable installation. Most developers would probably use the editable install option (which has the requirements described above).

@mscroggs
Copy link
Member

split install

I think that this run on the CI is trying to do this, but perhaps needs updating? Or do you mean something else by split install?

It does a split install, but not a split editable installation. Most developers would probably use the editable install option (which has the requirements described above).

We change it to editable. Or include both (is there any benefit to having both?)

@jorgensd
Copy link
Member Author

split install

I think that this run on the CI is trying to do this, but perhaps needs updating? Or do you mean something else by split install?

It does a split install, but not a split editable installation. Most developers would probably use the editable install option (which has the requirements described above).

We change it to editable. Or include both (is there any benefit to having both?)

Well, it would give better coverage. In any case, we would need the build_requirements.txt to be Added to Basix.

@@ -5,13 +5,20 @@
Basix can be installed using

```console
pip install .
python3 -m pip install .
Copy link
Member

Choose a reason for hiding this comment

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

The former was fine. Less is more.

For Python development, an editable install can be done with

```console
python3 -m pip -v install -r build-requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

-v not required


```console
python3 -m pip -v install -r build-requirements.txt
python3 -m pip -v install --config-settings=build-dir="build" --config-settings=cmake.build-type="Debug" --no-build-isolation -e .
Copy link
Member

Choose a reason for hiding this comment

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

-v not required

### Python interface

After installing the C++ library, install the Python interface by running in
the directory `python/`:

```console
pip install .
python3 -m pip install .
Copy link
Member

Choose a reason for hiding this comment

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

Use short version.

@@ -0,0 +1,2 @@
scikit-build-core[pyproject]==0.5.1 # Pinned due to https://github.com/scikit-build/scikit-build-core/issues/532
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't pin scikit-build-core; we support it, but it has a specific issue with editable install. spack, for example, could use a higher version without problems.

Add a note on editable builds and the scikit-build-core version.

Copy link
Member

Choose a reason for hiding this comment

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

Use markdown. We avoid rst where possible.

Basix Python interface
========================

This document explains how to install the Basix Python interface, given that the C++ interface has been built.
Copy link
Member

Choose a reason for hiding this comment

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

Shorten text.

Copy link
Member

Choose a reason for hiding this comment

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

See earlier version comments.


2. Ensure the build time requirements are installed::

python3 -m pip -v install -r build-requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

Use pip (shorter) and no need for -v.


3. Build Basix Python interface::

python3 -m pip -v install --no-build-isolation .
Copy link
Member

Choose a reason for hiding this comment

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

See above comments.

@garth-wells
Copy link
Member

Closing this as the editable issue was fixed by #727. Can be re-opened with any refinements of the docs.

@garth-wells garth-wells closed this Nov 7, 2023
@jorgensd jorgensd deleted the dokken/basix-install-instructions branch May 22, 2024 06:10
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.

Editable install of Python interface broken
5 participants