fix: simplifiy pybind11 usage

[henryiii]

This is a suggestion for using pybind11 via submodule. pyproject.toml is more elegant, but does require Pip 10+. I can switch to that if requested. Follow up on #714. I don't know all the details of building from scratch (It wants bison 3), but this should be close, I think. CC @rwgk .

I did not include ParallelCompile, but that might also be useful, I think.

[Solumin]

I didn't see this in time for #714, sorry! Can you clarify how pyproject.toml would work with using pybind11's CMake rules? We're using pybind11_add_module in pytype/typegraph/CMakeLists.txt.

Additionally, this should merge on the google_sync branch, since that's adding the file that depends on pybind11.

[henryiii]

For using CMake, it's probably better to use the submodules (especially since you already are using them for other things) - if you ever allow a CMake run without running it from setup.py, you can't force the items in pyproject.toml; users would have to install pybind11 manually (but if they already do with requirements.txt, for example, maybe that's not an issue?)

If you use FindPython, it's easy:

find_package(Python COMPONENTS Interpreter Development)

execute_process(
    COMMAND "${Python_EXECUTABLE}" "-c"
            "import pybind11; print(pybind11.get_cmake_dir()')"
    OUTPUT_VARIABLE pybind11_ROOT
    OUTPUT_STRIP_TRAILING_WHITESPACE)

find_package(pybind11 CONFIG REQUIRED)

Feel free to check for errors on the execute_process command to make it more user friendly if something goes wrong.

If you use the classic FindPythonLibs, it's harder, since pybind11 modifies the Python search so heavily. If you found PythonInterp and PythonLibs already, or got them from the setup.py run, etc, then just run PYTHON_EXECUTABLE instead of Python_EXECUTABLE, but otherwise the same as above.

[henryiii]

You could also do a hybrid, where it is used from the environment if available, or from the submodule if present, so you don't have to put it into the SDist. But you'd have to keep them in sync manually.

[Solumin]

You could also do a hybrid, where it is used from the environment if available, or from the submodule if present, so you don't have to put it into the SDist. But you'd have to keep them in sync manually.

This is more or less what I did in #715 in setup.py, where pybind11 (2.6.0rc2) is in requirements.txt, then setup.py tries to import it. But using the code from this PR should work better.

Unfortunately, I'm not very familiar with our build system, so I lack the knowledge to really comment on your previous comment. Besides agreeing that sticking with a submodule is fine, at least.

[henryiii]

I can adjust it to try one then the other. Should the submodule override the site-packages version, or the other way round?

[Solumin]

If the submodule overrides, the site-packages version will never be used and it doesn't need to be included in requirements.txt, right?

If site-packages overrides, I think that will give better stability for users, because they'll get pybind11 bug fixes faster. That's worth it to me.
On the other hand, we'll have more control over the submodule update schedule, which makes it easier for us to make sure pytype continues to build, I think. We can more easily keep the pybind version used on GitHub in sync with what's being used in Google.

I think the site-packages version should take precedence then.

[henryiii]

Okay, I can try that.

the site-packages version will never be used

Does this/ will this ever support building from SDist? If that's the case, then the submodule doesn't have to be grafted into your SDist, and therefore compiling from SDist would not have the submodule. Also someone could forget to clone the submodules. (not sure how this, especially SDist, would handle CPython, etc).

When you are building, the items in requirements.txt or the user's environment aren't used - pip makes a new environment with just the pyproject.toml requirements and builds there (unless you pass --no-build-isolation, which is rare, mostly only used when making conda packages). So for practical purposes, it will still be pinned by pyproject.toml.

By the way, that's a good reason to prioritize on site packages - conda-forge may ship a modified pybind11 and you could take advantage of that.

[Solumin]

We do in fact use sdist when pushing a release to pypi: https://github.com/google/pytype/blob/master/build_scripts/release.py#L92
In that case, site packages is the way to go, if I'm following what you're saying.

[henryiii]

That's odd, I'm triggering

setup.py:24:0: C0413: Import "from pybind11.setup_helpers import Pybind11Extension" should be placed at the top of the module (wrong-import-position)

But I have:

from pybind11.setup_helpers import Pybind11Extension  # pylint: disable=g-import-not-at-top

[Solumin]

Can you add a comment explaining why this is necessary?

[henryiii]

It's just tidy. You could leave $PWD/pybind11 in the path if you prefer.

[henryiii]

I'll remove it, since you don't clean up the build_scripts addition below either.

[Solumin]

That's odd, I'm triggering

setup.py:24:0: C0413: Import "from pybind11.setup_helpers import Pybind11Extension" should be placed at the top of the module (wrong-import-position)

But I have:

from pybind11.setup_helpers import Pybind11Extension  # pylint: disable=g-import-not-at-top

Moving the import up will help, or using # pylint: disable=g-import-not-at-top,wrong-import-position might fix it, I think.

[Solumin]

This looks great, thank you so much for doing this!

[martindemello]

why == rather than >= for the requirement here?

[henryiii]

pyproject.toml's usually pin requirements fairly tightly; it can be used to create a completely reproducible build. Since the items here are always downloaded and installed into the temporary build environment, there's no need to make the requirements too wide. Though it could be loosened up if you want. I think 2.6.0 will be out Monday-ish; then you can change this. I would probably not do more than "pybind11~=2.6", though. (The tilde means the final digit, the 6 in this case, can be that or higher)

[henryiii]

My recommendation would be to keep it pinned to match the submodule, though.

I realize that I forgot to drop this from the MANIFEST, will do that.

[henryiii]

Here's an example of when it is very important to pin exactly, by the way: https://scikit-hep.org/developer/packaging#special-additions-numpy

[martindemello]

thanks, makes sense!