Skip to content

Replace nb::any with -1 and add argument to nb::ndarray in preparation of next nanobind release #3105

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

Merged
merged 13 commits into from
May 24, 2024

Conversation

francesco-ballarin
Copy link
Member

@francesco-ballarin francesco-ballarin commented Mar 16, 2024

This is not meant to be merged yet, because it will need to wait for a new nanobind release that includes that commit.
Furthermore, it will need to be preceded by :

  1. a commit/PR that bumps the minimum required nanobind version in pyproject.toml because, as shown by the failure https://github.com/FEniCS/dolfinx/actions/runs/8308122378/job/22738016168 (nanobind current release, dolfinx from this PR) and announced by the wording in the upstream commit, the change is backward incompatible
  2. a commit/PR that bumps the nanobind version in docker images.
  3. dropping workflow changes.

Requires FEniCS/basix#799

@francesco-ballarin francesco-ballarin added the nanobind Issues related to nanobindings label Mar 16, 2024
@jhale
Copy link
Member

jhale commented Apr 12, 2024

Is this merged in nanobind release yet? If not I would propose doing a backport at a later date.

@francesco-ballarin
Copy link
Member Author

It's not in any nanobind release yet, we will have to backport it after 0.8.0.

@jorgensd jorgensd added this to the 0.9.0 milestone Apr 12, 2024
@francesco-ballarin francesco-ballarin changed the title Replace nb::any with -1 in preparation of next nanobind release Replace nb::any with -1 and add argument to nb::ndarray in preparation of next nanobind release Apr 13, 2024
@francesco-ballarin francesco-ballarin force-pushed the francesco/nb-any branch 2 times, most recently from 3c7003f to 6e3e564 Compare April 13, 2024 05:53
@francesco-ballarin francesco-ballarin force-pushed the francesco/nb-any branch 3 times, most recently from 1e172e6 to e63a6c5 Compare April 13, 2024 06:55
@francesco-ballarin
Copy link
Member Author

The fourth commit, to be dropped before merging this PR, upgrades nanobind from source. nanobind installation happens before basix is installed, to ensure that basix and dolfinx wrappers use a matching nanobind version.

Will need to figure out at a later stage what is going on with the Red Hat build: those are the same errors that I used to get when there was a mismatch between basix and dolfinx wrappers, for instance basix wrappers were compiled with nanobind release version and dolfinx wrappers were compiled with nanobind main.

@jhale
Copy link
Member

jhale commented Apr 13, 2024

For redhat Basix will be built with build isolation (pip will download nanobind), dolfinx without (system nanobind).

@jhale
Copy link
Member

jhale commented Apr 13, 2024

I think all of our dolfinx basix builds should be without isolation - upstream Basix should do isolated build testing.

@jorgensd jorgensd modified the milestones: 0.9.0, 0.10.0 May 7, 2024
@francesco-ballarin francesco-ballarin force-pushed the francesco/nb-any branch 2 times, most recently from 1bba58f to 360dcb9 Compare May 19, 2024 08:50
… have a default value. Assign the former defaul value everywhere in nb wrappers
@francesco-ballarin francesco-ballarin force-pushed the francesco/nb-any branch 4 times, most recently from 0c0d8b5 to 181bd8d Compare May 19, 2024 11:20
@garth-wells garth-wells marked this pull request as ready for review May 23, 2024 18:15
@francesco-ballarin
Copy link
Member Author

@garth-wells do revert aab5ba6 altogether now that nanobind 2.0 is out.

@garth-wells garth-wells modified the milestones: 0.10.0, 0.9.0 May 24, 2024
@garth-wells garth-wells enabled auto-merge May 24, 2024 06:18
@garth-wells garth-wells added this pull request to the merge queue May 24, 2024
Merged via the queue into main with commit 42c4348 May 24, 2024
@garth-wells garth-wells deleted the francesco/nb-any branch May 24, 2024 07:25
@jorgensd
Copy link
Member

I think all of our dolfinx basix builds should be without isolation - upstream Basix should do isolated build testing.

Can we backport this to the latest release? Currently I would have to pin nanobind based on the dolfinx tag I want to test with.
Context:
I have a Github action that installs DOLFINx from source for the nightly and current images. As we have not pinned nanobind on v0.8.x, I get into build issues with the current workflow:
https://github.com/jorgensd/actions/blob/dokken/remove-pybind/install-dolfinx/action.yaml
with the CI: https://github.com/jorgensd/actions/actions/runs/9264998235/job/25486173015?pr=8
and a part of the traceback:

  *** Building project with Ninja...
  [1/24] Building CXX object CMakeFiles/cpp.dir/dolfinx/wrappers/dolfinx.cpp.o
  [2/24] Building CXX object CMakeFiles/cpp.dir/dolfinx/wrappers/graph.cpp.o
  FAILED: CMakeFiles/cpp.dir/dolfinx/wrappers/graph.cpp.o
  /usr/bin/c++ -DADIOS2_USE_MPI -DBOOST_TIMER_DYN_LINK -DBOOST_TIMER_NO_LIB -DDOLFINX_VERSION=\"0.8.0\" -DHAS_ADIOS2 -DHAS_PETSC -DHAS_PTSCOTCH -DHAS_SLEPC -Dcpp_EXPORTS -I/usr/local/lib/python3.12/dist-packages/petsc4py/include -I/usr/local/lib/python3.12/dist-packages/mpi4py/include -I/usr/include/python3.12 -I/usr/local/lib/python3.12/dist-packages/nanobind/include -isystem /usr/local/lib/python3.12/dist-packages/ffcx/codegeneration -isystem /usr/local/petsc/linux-gnu-complex64-32/include -isystem /usr/local/petsc/include -isystem /usr/local/slepc/linux-gnu-complex64-32/include -isystem /usr/local/slepc/include -O3 -DNDEBUG -std=c++20 -fPIC -fvisibility=hidden -fno-stack-protector -ffunction-sections -fdata-sections -MD -MT CMakeFiles/cpp.dir/dolfinx/wrappers/graph.cpp.o -MF CMakeFiles/cpp.dir/dolfinx/wrappers/graph.cpp.o.d -o CMakeFiles/cpp.dir/dolfinx/wrappers/graph.cpp.o -c /__w/actions/actions/src/dolfinx/python/dolfinx/wrappers/graph.cpp
  /__w/actions/actions/src/dolfinx/python/dolfinx/wrappers/graph.cpp: In instantiation of ‘void dolfinx_wrappers::declare_adjacency_list(nanobind::module_&, std::string) [with T = int; std::string = std::__cxx11::basic_string<char>]’:
  /__w/actions/actions/src/dolfinx/python/dolfinx/wrappers/graph.cpp:115:39:   required from here
  /__w/actions/actions/src/dolfinx/python/dolfinx/wrappers/graph.cpp:85:24: error: no matching function for call to ‘nanobind::ndarray<const int, nanobind::numpy>::ndarray(std::span<const int>::pointer, <brace-enclosed initializer list>)’
     85 |             return nb::ndarray<const T, nb::numpy>(link.data(), {link.size()});
        |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@garth-wells
Copy link
Member

The simplest course of action would be to backport a pin of nanobind.

jorgensd added a commit to jorgensd/dolfinx_mpc that referenced this pull request May 28, 2024
- Also add some additional `create_connectivity(tdim, tdim)` calls prior to midpoint computations (consequence of FEniCS/dolfinx#3209)
- Switch to spdlog and move some logging to debug mode (FEniCS/dolfinx#3216)
- Support nanobind 2.0.0 (FEniCS/dolfinx#3105)
- Replace `dolfinx.Function.vector` with `dolfinx.Function.x.petsc_vec` (FEniCS/dolfinx#3092)
@efirvida
Copy link

Hi.

this change: https://github.com/FEniCS/dolfinx/pull/3105/files#diff-1a45bd938160f7761769e7b6fbc819d1cc7f84ce37f1a6a23ef83cc4e214abc8

Breaks the compatibility with the version v0.8.0, in which the python binding can't be installed from source with the nanobind v2.0.0.

@francesco-ballarin
Copy link
Member Author

@efirvida That's the other way around. This change guarantees that dolfinx main is installable with nanobind 2.0.

You'll have to downgrade your nanobind version if you are trying to install v0.8.0.

@efirvida
Copy link

@francesco-ballarin thanks for you answer, what I mean is that the released package of the v0.8.0 have this issue, so as it is a release I think this should be fixed on the package. without affecting the main branch.

In my case for example I'm using it on a HPC cluste with easybuild package manager. When I try to use the release version it was imposible until I found this thread, and I use an older nanobind. was a quick fix but it take a while until I discovered.

if it's helpful for the project I can share the easy configs.

@francesco-ballarin
Copy link
Member Author

Indeed, this could be backported, or the alternative way discussed in #3105 (comment) could be followed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nanobind Issues related to nanobindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants