triton icon indicating copy to clipboard operation
triton copied to clipboard

[BUILD] Use build-system requires to install pybind11

Open tiran opened this issue 1 year ago • 4 comments

pybind11 is now a build-system requirement. The package is no longer downloaded in setup.py. Instead the build system automatically installs the build dependency pybind11 before it runs setup.py. This simplifies offline builds and makes the build requirement visible in standard packaging tools.

I kept support support for PYBIND11_SYSPATH. The code can be simplified even more if the feature is no longer needed:

cmake_args = [
    ...
    f"-DPYBIND11_INCLUDE_DIR={pybind11.get_include()}",
    ...
]

See: #4414

The core Triton is a small number of people, and we receive many PRs (thank you!). To help us review your code more quickly, if you are a new contributor (less than 3 PRs merged) we ask that you complete the following tasks and include the filled-out checklist in your PR description.

Complete the following tasks before sending your PR, and replace [ ] with [x] to indicate you have done them.

  • [x] I am not making a trivial change, such as fixing a typo in a comment.

  • [x] I have written a PR description following these rules.

  • [x] I have run pre-commit run --from-ref origin/main --to-ref HEAD.

  • Select one of the following.

    • [ ] I have added tests.
      • /test for lit tests
      • /unittest for C++ tests
      • /python/test for end-to-end tests
    • [x] This PR does not need a test because it modifies the build system in a backwards compatible way.
  • Select one of the following.

    • [x] I have not added any lit tests.
    • [ ] The lit tests I have added follow these best practices, including the "tests should be minimal" section. (Usually running Python code and using the instructions it generates is not minimal.)

tiran avatar Aug 02 '24 09:08 tiran

CC @SomeoneSerge Thank you for your TRITON_OFFLINE_BUILD feature! I was working on a similar improvement and downstream patches for Fedora. You may find this PR a useful addition on top of your PR #4414.

tiran avatar Aug 02 '24 09:08 tiran

I wonder why we didn't adopt this approach? Seems like pybind11 has been available on pypi for a long time @ThomasRaoux @ptillet

Jokeren avatar Aug 02 '24 14:08 Jokeren

I wondered that, too, @Jokeren . The old approach probably worked fine for you and you never ran into problems.

I'm working on downstream packaging of the Python deep learning and AI stack for Fedora ecosystem. Just like nixOS, we have to generate packages from offline artifacts and without any access to the internet. It's part of our supply chain security and reproducible build policy. Triton is a ... challenge. I had a bunch of downstream patches. #4414 got rid of several patches. This PR is another low-hanging fruit from my patch set.

tiran avatar Aug 02 '24 14:08 tiran

Thanks @tiran. Indeed we'd benefit from this patch too, our current PR works around the pybind11 situation in a very ad hoc way

SomeoneSerge avatar Aug 02 '24 23:08 SomeoneSerge

@Jokeren @ThomasRaoux @ptillet please take a look again and approve CI run.

tiran avatar Aug 28 '24 10:08 tiran

@tiran see the installation problem on mac

Jokeren avatar Aug 28 '24 13:08 Jokeren

@tiran see the installation problem on mac

I have fixed the --no-build-isolation test case by installing pybind11 requirement manually. No build isolation does not install build dependencies automatically.

tiran avatar Aug 28 '24 14:08 tiran