wjakob / nanobind

nanobind: tiny and efficient C++/Python bindings
BSD 3-Clause "New" or "Revised" License
2.14k stars 161 forks source link

CMake version not reported #596

Open stellaraccident opened 1 month ago

stellaraccident commented 1 month ago

While trying to upgrade to nanobind 2.0, we encountered source level compatibility issues. As an aid to our users, I tried adding a CMake warning if nanobind_VERSION was less than 2.0, but it does not seem that find_package(nanobind) sets the version.

As a workaround, I am using this execute_process, which is less than ideal and is not precise:

  # Note that nanobind does not set the CMake version properly so we can't
  # check with find_package. Since 2.0 had breaking changes, we try a little
  # bit harder to make sure that a pip sourced version is correct, and that is
  # the best we can do.
  execute_process(
    COMMAND "${Python_EXECUTABLE}" -m nanobind --version
    OUTPUT_STRIP_TRAILING_WHITESPACE
    OUTPUT_VARIABLE NB_VERSION
    RESULT_VARIABLE RC)
  if(NB_VERSION VERSION_LESS "2.0")
    message(WARNING "IREE's runtime python bindings require nanobind>=2.0. Please upgrade: ${Python_EXECUTABLE} -m pip install --upgrade 'nanobind>=2.0'")
  endif()
wjakob commented 1 month ago

What is needed to set this version number? Is a line

SET(nanobind_VERSION "2.0.0")

in cmake/nanobind-config.cmake enough? Could I ask you to check?

stellaraccident commented 1 month ago

Yeah, I can check. I'd need to read the manual for find_package as its been a minute since I did this.

hpkfft commented 1 month ago

I think the full solution is for nanobind to use CMakePackageConfigHelpers

There's example code at the bottom of the page.

I would suggest specifying the version in the top level project command:

project(nanobind VERSION 2.0.0 LANGUAGES NONE)

and then using VERSION "${PROJECT_VERSION}" instead of VERSION 1.2.3 in write_basic_package_version_file().

Then, users can write:

find_package(nanobind 2.0 CONFIG REQUIRED)

in their own CMakeLists.txt

Sorry, I'm not able to provide more help than this comment. Also, if anybody has a better/easier idea, I'd love to hear it. Maybe this mechanism is only needed if using nanobind packages (e.g. debian packages or RPMs) and not needed when cloning from git? I don't know. Maybe just adding VERSION to the project command is worth something....

henryiii commented 1 month ago

Yes, setting VERSION in the project command sets a bunch of variables like PROJECT_VERSION, nanobind_VERSION (based on project name), and *_VERSION_{MAJOR,MINOR,PATCH}.

write_basic_package_version_file will use PROJECT_VERSION if you don't give it a VERSION.

If you want to do it by hand, you need to make a nanobindConfigVersion.cmake off of one of the templates, like https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/BasicConfigVersion-AnyNewerVersion.cmake.in. Though I'd just use the generator function unless there's something special about the version - several options for compatibility are provided already.

Here's what pybind11 does: https://github.com/pybind/pybind11/blob/86a64290dc63609b12da796afa334b1f24851420/CMakeLists.txt#L275-L293 - you shouldn't need the CMake <3.14 branch.

wjakob commented 1 month ago

Would one of the commenters here (who all understand CMake much better than me ;)) be willing to make a PR to include such a version number? Thank you!

stellaraccident commented 1 month ago

Yes, I can help if no one else does. Just have a long queue of things to help on :)

henryiii commented 1 month ago

Let us know here if you start working on it, and I'll do the same if I do. Also can help if no one does, also have a long queue. :) (Free-threading support for various things like scikit-build-core is eating up my time ATM)

joaander commented 2 weeks ago

I would be happy to work on this, but I am unsure how you would like me to proceed. I modified CMakeLists.txt as described above (using the canonical write_basic_package_version_file), however the changes do not take effect because setup.py is using setuptools https://github.com/wjakob/nanobind/blob/c5ae2a36a704c1c62da99a81fad919261a3e9848/setup.py#L1 even though pyproject.toml configures the build system for scikit-build: https://github.com/wjakob/nanobind/blob/c5ae2a36a704c1c62da99a81fad919261a3e9848/pyproject.toml#L1-L2 CMakeLists.txt is not used during the nanobind build process.

Should I refactor the setup.py to actually use scikit-build? The install rules in CMakelists.txt appear complete.

I would prefer not to manually create nanobindConfigVersion.cmake The auto-generated files are long and editing them would be prone to error. Also, future versions of CMake may generate the files differently.

henryiii commented 2 weeks ago

I can try to help, in all-day hackathon for the next three days, but might be able to look at this at night.

joaander commented 2 weeks ago

I'm capable of making the needed changes. My question is should I? Put another way, is the current setuptools build infrastructure intended to be kept as a long term solution? Or would the nanobind maintainers appreciate a conversion to scikit-build (or better yet, scikit-build-core)?

henryiii commented 2 weeks ago

I've pinged Wenzel to make sure he sees it, he can answer that.

wjakob commented 2 weeks ago

My 2cts: My main interaction with setup.py is to create a wheel for uploading to PyPI. There is no build process so to speak, and the setup.py script only serves the purpose of collecting a bunch of files and putting them into the right place of the generated archive.

I find that simplicity valuable. On the other hand, scikit-build-core addresses a more complex situation: to bridge the Python and C++ build system worlds where CMake is used to compile executables, shared libraries, etc. Since CMake doesn't even need to run to create the nanobind wheel, it would seem overkill to use it here. On the other hand, it makes a ton of sense to have it in the nanobind_example repo.

stellaraccident commented 2 weeks ago

Yeah, I would stick with setup.py, since as you say, there is actually no native build. Just a bit of python and "data files".

henryiii commented 2 weeks ago

That's fine, but you can use scikit-build to build a pure Python wheel that uses CMake to generate some cmake files. It won't affect most users because they get the pure Python wheel, not the SDist.

I don't think nanobindConfigVersion.cmake would be that bad to embed, though.

wjakob commented 2 weeks ago

That's fine, but you can use scikit-build to build a pure Python wheel that uses CMake to generate some cmake files.

I would prefer not to, it seems unnecessarily complicated.

stellaraccident commented 2 weeks ago

That's fine, but you can use scikit-build to build a pure Python wheel that uses CMake to generate some cmake files.

I would prefer not to, it seems unnecessarily complicated.

Yeah, there's not really a slippery slope here to needing much, and a simple, canonical pure python setup anyone can easily grok/debug without needing to know much more.

henryiii commented 2 weeks ago

unnecessarily complicated.

This sounds like a challenge. I might have to make a scikit-build-core attempt just because of this. :)

stellaraccident commented 2 weeks ago

I mean, you do you :) maybe less of a challenge if you take it how I think it was meant: "unnecessarily complicated for the use case [because it was built to solve for real but disjoint issues from what is needed here]". I expect the author wants to be able to maintain their own project once this bug is fixed without having to learn to use another set of tools.

You'd be competing literally with the simplest possible use of setuptools. There isn't even anything to build in this project. Just packaging some files.

Now on the other hand, if the key to getting you to fix some much "loved" but working old setuptools/cmake builds is to tell you you can't do it: well I have a few projects that I bet you you couldn't improve with scikit-build-core ;) (just joking of course -- kind of)

henryiii commented 2 weeks ago

The problem with is there is no "simple" usage of setuptools. Very few people using it actually knows how it works, they just go from examples of examples of examples that have been passed down for years. ChatGPT is probably pretty good at setuptools, that's the only immediate benefit I can think of. ;) If something goes wrong, a lot of the 40,000 lines of organically grown code shows up in the inheritance & plugin internals. The current build is broken, in fact:

$ pipx run build
* Creating isolated environment: venv+pip...
* Installing packages in isolated environment:
  - cmake>=3.17
  - ninja
  - scikit-build
  - setuptools>=42
  - typing_extensions
  - wheel
* Getting build dependencies for sdist...
running egg_info
creating nanobind.egg-info
writing nanobind.egg-info/PKG-INFO
writing dependency_links to nanobind.egg-info/dependency_links.txt
writing top-level names to nanobind.egg-info/top_level.txt
writing manifest file 'nanobind.egg-info/SOURCES.txt'
reading manifest file 'nanobind.egg-info/SOURCES.txt'
adding license file 'LICENSE'
writing manifest file 'nanobind.egg-info/SOURCES.txt'
* Building sdist...
running sdist
running egg_info
writing nanobind.egg-info/PKG-INFO
writing dependency_links to nanobind.egg-info/dependency_links.txt
writing top-level names to nanobind.egg-info/top_level.txt
reading manifest file 'nanobind.egg-info/SOURCES.txt'
adding license file 'LICENSE'
writing manifest file 'nanobind.egg-info/SOURCES.txt'
running check
creating nanobind-2.0.1
creating nanobind-2.0.1/nanobind.egg-info
creating nanobind-2.0.1/tests
copying files to nanobind-2.0.1...
copying LICENSE -> nanobind-2.0.1
copying README.md -> nanobind-2.0.1
copying pyproject.toml -> nanobind-2.0.1
copying setup.py -> nanobind-2.0.1
copying nanobind.egg-info/PKG-INFO -> nanobind-2.0.1/nanobind.egg-info
copying nanobind.egg-info/SOURCES.txt -> nanobind-2.0.1/nanobind.egg-info
copying nanobind.egg-info/dependency_links.txt -> nanobind-2.0.1/nanobind.egg-info
copying nanobind.egg-info/not-zip-safe -> nanobind-2.0.1/nanobind.egg-info
copying nanobind.egg-info/top_level.txt -> nanobind-2.0.1/nanobind.egg-info
copying tests/test_chrono.py -> nanobind-2.0.1/tests
copying tests/test_classes.py -> nanobind-2.0.1/tests
copying tests/test_eigen.py -> nanobind-2.0.1/tests
copying tests/test_enum.py -> nanobind-2.0.1/tests
copying tests/test_eval.py -> nanobind-2.0.1/tests
copying tests/test_exception.py -> nanobind-2.0.1/tests
copying tests/test_functions.py -> nanobind-2.0.1/tests
copying tests/test_holders.py -> nanobind-2.0.1/tests
copying tests/test_inter_module.py -> nanobind-2.0.1/tests
copying tests/test_intrusive.py -> nanobind-2.0.1/tests
copying tests/test_issue.py -> nanobind-2.0.1/tests
copying tests/test_make_iterator.py -> nanobind-2.0.1/tests
copying tests/test_ndarray.py -> nanobind-2.0.1/tests
copying tests/test_stl.py -> nanobind-2.0.1/tests
copying tests/test_stl_bind_map.py -> nanobind-2.0.1/tests
copying tests/test_stl_bind_vector.py -> nanobind-2.0.1/tests
copying tests/test_stubs.py -> nanobind-2.0.1/tests
copying tests/test_typing.py -> nanobind-2.0.1/tests
copying nanobind.egg-info/SOURCES.txt -> nanobind-2.0.1/nanobind.egg-info
Writing nanobind-2.0.1/setup.cfg
Creating tar archive
removing 'nanobind-2.0.1' (and everything under it)
* Building wheel from sdist
* Creating isolated environment: venv+pip...
* Installing packages in isolated environment:
  - cmake>=3.17
  - ninja
  - scikit-build
  - setuptools>=42
  - typing_extensions
  - wheel
* Getting build dependencies for wheel...
Traceback (most recent call last):
  File "/Users/henryschreiner/.local/pipx/.cache/b34db7242f3e439/lib/python3.12/site-packages/pyproject_hooks/_in_process/_in_process.py", line 373, in <module>
    main()
  File "/Users/henryschreiner/.local/pipx/.cache/b34db7242f3e439/lib/python3.12/site-packages/pyproject_hooks/_in_process/_in_process.py", line 357, in main
    json_out["return_val"] = hook(**hook_input["kwargs"])
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/henryschreiner/.local/pipx/.cache/b34db7242f3e439/lib/python3.12/site-packages/pyproject_hooks/_in_process/_in_process.py", line 134, in get_requires_for_build_wheel
    return hook(config_settings)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-env-o8lhy6kf/lib/python3.12/site-packages/setuptools/build_meta.py", line 325, in get_requires_for_build_wheel
    return self._get_build_requires(config_settings, requirements=['wheel'])
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-env-o8lhy6kf/lib/python3.12/site-packages/setuptools/build_meta.py", line 295, in _get_build_requires
    self.run_setup()
  File "/private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-env-o8lhy6kf/lib/python3.12/site-packages/setuptools/build_meta.py", line 311, in run_setup
    exec(code, locals())
  File "<string>", line 12, in <module>
FileNotFoundError: [Errno 2] No such file or directory: '/private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-via-sdist-iwdnro16/nanobind-2.0.1/include/nanobind/nanobind.h'

It is invalid to have a setup.py with no MANIFEST.in unless you only have simple .py files in your project. Nanobind is currently not shipping SDists, which would be broken (but this is how all third party distributions want Python packages) The .whl would be just as simple, and the SDist would work, and I'm 70% sure I could even avoid requiring CMake when making a wheel from an SDist, it would only be needed from source... I think. That's one reason this is interesting. This would be the first example I'm aware of this. But I could also use it in pybind11, at least. I'm 100% I could make a nice wheel, though, and a better SDist situation than currently, even if CMake was always required (automatically) when making the wheel. (Which it is now, actually, due to the over specified requirements)

I could also just fix the setuptools-based packaging, remove the unneeded dependencies, and add the MANIFEST.in - maybe ideally I could make two PRs and the same time, and then it would be a "pick your favorite" situation. :)

I'm sadly short on time, though, hackathon is all this week.

Now on the other hand, if the key to getting you to fix some much "loved" but working old setuptools/cmake builds is to tell you you can't do it

That's not hard to do if you ask nicely. That's basically what I did last SciPy during the sprints. People brought things like a 800 LoC setup.py and I replaced them with 20 line CMakeLists + scikit-build-core, and fixed long standing bugs too in the process. ;)

joaander commented 2 weeks ago

As an outsider looking at nanobind for the first time (a long time user of pybind11, though), I see that you already have the unnecessary complication of the CMake install rules:

https://github.com/wjakob/nanobind/blob/c5ae2a36a704c1c62da99a81fad919261a3e9848/CMakeLists.txt#L50-L97

and the unused dependency on scikit-build I mentioned previously.

These are duplicated by the python code: https://github.com/wjakob/nanobind/blob/c5ae2a36a704c1c62da99a81fad919261a3e9848/setup.py#L42-L53

In other words, the majority of the work in implementing a scikit-build-core solution is already done. It is only a few lines of extra code to enable and allows you to delete the duplication in setup.py. If you prefer your setup.py solution, then you will also need to maintain the nanobindConfigVersion.cmake file by hand which I am not familiar with.

stellaraccident commented 2 weeks ago

That's not hard to do if you ask nicely.

Maybe I will introduce myself and ask properly someday :) I did apply some advice you gave years ago upon seeing a travesty we were using, but sadly, much of what I try not to look at too closely falls into that "1000 line setup.py category", most of it come by bit by bit over many years and hands.

The trick with adding more nesting/layering/declarative stuff to the build tends not to be that someone who knows it well can say it in a crisp and robust way -- but that few others have the context to add "the one little thing" they need for their weird case. Like it or not, most folks can always hack the open coded way and not so much the better way. I hate even defending that, but have seen it often enough. I do miss build setups that are well engineered in this area...

joaander commented 2 weeks ago

Here is a draft PR that shows what I mean about the simplicity of switching to scikit-build-core: #618

As @henryiii states, there is a little more work to be done to fix sdist builds - but this PR is already most of the way there.