Skip to content

[BUILD] Use build-system requires to install pybind11#4450

Merged
ThomasRaoux merged 1 commit intotriton-lang:mainfrom
tiran:pybind11-build-requires
Aug 30, 2024
Merged

[BUILD] Use build-system requires to install pybind11#4450
ThomasRaoux merged 1 commit intotriton-lang:mainfrom
tiran:pybind11-build-requires

Conversation

@tiran
Copy link
Copy Markdown
Contributor

@tiran tiran commented Aug 2, 2024

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.

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

  • I have written a PR description following these
    rules.

  • 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
    • This PR does not need a test because it modifies the build system in a backwards compatible way.
  • Select one of the following.

    • 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 tiran requested a review from ptillet as a code owner August 2, 2024 09:17
@tiran
Copy link
Copy Markdown
Contributor Author

tiran commented Aug 2, 2024

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.

@Jokeren Jokeren changed the title Use build-system requires to install pybind11 [BUILD] Use build-system requires to install pybind11 Aug 2, 2024
@Jokeren
Copy link
Copy Markdown
Contributor

Jokeren commented Aug 2, 2024

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

@tiran
Copy link
Copy Markdown
Contributor Author

tiran commented Aug 2, 2024

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.

@SomeoneSerge
Copy link
Copy Markdown
Contributor

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

@tiran tiran force-pushed the pybind11-build-requires branch from 2f41196 to fae93cb Compare August 5, 2024 16:27
@tiran tiran force-pushed the pybind11-build-requires branch from fae93cb to 6e37f2f Compare August 28, 2024 10:38
@tiran
Copy link
Copy Markdown
Contributor Author

tiran commented Aug 28, 2024

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

@Jokeren
Copy link
Copy Markdown
Contributor

Jokeren commented Aug 28, 2024

@tiran see the installation problem on mac

`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:

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

See: triton-lang#4414
Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran force-pushed the pybind11-build-requires branch from 6e37f2f to d622426 Compare August 28, 2024 14:30
@tiran
Copy link
Copy Markdown
Contributor Author

tiran commented Aug 28, 2024

@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.

Copy link
Copy Markdown
Collaborator

@ThomasRaoux ThomasRaoux left a comment

Choose a reason for hiding this comment

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

LGTM

@ThomasRaoux ThomasRaoux merged commit aadf615 into triton-lang:main Aug 30, 2024
@tiran tiran deleted the pybind11-build-requires branch August 30, 2024 13:29
bertmaher pushed a commit to bertmaher/triton that referenced this pull request Dec 10, 2024
`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:

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

See: triton-lang#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](https://cbea.ms/git-commit/#why-not-how).

- [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](https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices),
including the "tests should be minimal" section. (Usually running Python
code
    and using the instructions it generates is not minimal.)

Signed-off-by: Christian Heimes <christian@python.org>
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.

4 participants