Skip to content

setup.py: introduce TRITON_OFFLINE_BUILD#4414

Merged
ThomasRaoux merged 1 commit intotriton-lang:mainfrom
SomeoneSerge:fix/setup-unvendor
Aug 1, 2024
Merged

setup.py: introduce TRITON_OFFLINE_BUILD#4414
ThomasRaoux merged 1 commit intotriton-lang:mainfrom
SomeoneSerge:fix/setup-unvendor

Conversation

@SomeoneSerge
Copy link
Contributor

@SomeoneSerge SomeoneSerge commented Jul 29, 2024

In e.g. Nixpkgs (probably in other similar distributions as well) it's important to be able to inject dependencies rather than use vendored/locked dependencies. When we fail to provide complete dependencies for triton we want to get an immediate error, rather than an attempt to download a substitute from the internet (which fails after a timeout anyway, because we the actual builds offline). Sometimes we'd also try to override a recipe with a different version of a dependency than declared upstream, so e.g. we tend to treat strict semantic version checking as an optional feature.

This PR proposes an additional boolean environment variable meant to disable vendoring and fetching attempts in favour of either an immediate error ("input declared but not defined") or a later compile/link time error.

I'm happy to maintain a patch on the Nixpkgs side as well if you think this doesn't belong in your repo.

I'm waiting for the builds to finish.

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 FILL THIS IN.
  • 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.)

Copy link
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.

I'm a bit concerned about adding yet another build mode that would be untested in CI although I'm not sure I fully understand what it is trying to do. Is it just to get different version of NV tools and LLVM? There are already environment variable for that. Why does this not work for you?

@SomeoneSerge
Copy link
Contributor Author

There are already environment variable for that. Why does this not work for you?

That used to be sufficient when Package.syspath_var_name was first introduced. Now get_thirdparty_packages and get_thirdparty_packages have grown to implement more logic, in particular the version/url checks, so we're back to deciding on what is the proper way to skip over these branches:

(not os.path.exists(version_file_path) or Path(version_file_path).read_text() != p.url):

triton/python/setup.py

Lines 261 to 265 in ff7c691

download = not os.path.exists(src_path)
if os.path.exists(dst_path) and system == "Linux" and shutil.which(dst_path) is not None:
curr_version = subprocess.check_output([dst_path, "--version"]).decode("utf-8").strip()
curr_version = re.search(r"V([.|\d]+)", curr_version).group(1)
download = download or curr_version != version

A universal knob to make things "offline or fail" is the logical solution looking through our lense, but we're happy to adjust

@SomeoneSerge SomeoneSerge force-pushed the fix/setup-unvendor branch 3 times, most recently from 55771aa to 587d1f3 Compare July 31, 2024 21:18
To prevent any vendoring whatsoever
@ThomasRaoux ThomasRaoux merged commit 0503565 into triton-lang:main Aug 1, 2024
tiran added a commit to tiran/triton that referenced this pull request Aug 2, 2024
Instead of downloading `pybind11` in `setup.py`, it is now a
build-system requirement. The build system automatically installs
the build dependency `pybind11` before it runs `setup.py`. This
simplifies offline builds.

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 added a commit to tiran/triton that referenced this pull request 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:

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

See: triton-lang#4414
Signed-off-by: Christian Heimes <christian@python.org>
tiran added a commit to tiran/triton that referenced this pull request Aug 4, 2024
`is_offline_build()` now uses `check_env_flag()` like the rest of the
`setup.py` code. `TRITON_OFFLINE_BUILD=YES` and
`TRITON_OFFLINE_BUILD=NO` work as expected.

Offline builds now disable unit test builds. The default
`TRITON_BUILD_UT=YES` triggers a download of `googletests` code from
GitHub.

See: triton-lang#4414
Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran mentioned this pull request Aug 4, 2024
7 tasks
Jokeren pushed a commit that referenced this pull request Aug 5, 2024
`is_offline_build()` now uses `check_env_flag()` like the rest of the
`setup.py` code. `TRITON_OFFLINE_BUILD=YES` and
`TRITON_OFFLINE_BUILD=NO` work as expected.

Offline builds now disable unit test builds. The default
`TRITON_BUILD_UT=YES` triggers a download of `googletests` code from
GitHub.

See: #4414

- [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 improves the build system.

- 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>
tiran added a commit to tiran/triton that referenced this pull request Aug 5, 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
Signed-off-by: Christian Heimes <christian@python.org>
tiran added a commit to tiran/triton that referenced this pull request Aug 28, 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
Signed-off-by: Christian Heimes <christian@python.org>
tiran added a commit to tiran/triton that referenced this pull request Aug 28, 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
Signed-off-by: Christian Heimes <christian@python.org>
ThomasRaoux pushed a commit that referenced this pull request Aug 30, 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: #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>
bertmaher pushed a commit to bertmaher/triton that referenced this pull request Dec 10, 2024
In e.g. Nixpkgs (probably in other similar distributions as well) it's
important to be able to inject dependencies rather than use
vendored/locked dependencies. When we fail to provide complete
dependencies for triton we want to get an immediate error, rather than
an attempt to download a substitute from the internet (which fails after
a timeout anyway, because we the actual builds offline). Sometimes we'd
also try to override a recipe with a different version of a dependency
than declared upstream, so e.g. we tend to treat strict semantic version
checking as an optional feature.

This PR proposes an additional boolean environment variable meant to
disable vendoring and fetching attempts in favour of either an immediate
error ("input declared but not defined") or a later compile/link time
error.

I'm happy to maintain a patch on the Nixpkgs side as well if you think
this doesn't belong in your repo.

I'm waiting for the builds to finish.

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

- [ ] 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 `FILL THIS IN`.

- Select one of the following.
  - [ ] 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.)
bertmaher pushed a commit to bertmaher/triton that referenced this pull request Dec 10, 2024
`is_offline_build()` now uses `check_env_flag()` like the rest of the
`setup.py` code. `TRITON_OFFLINE_BUILD=YES` and
`TRITON_OFFLINE_BUILD=NO` work as expected.

Offline builds now disable unit test builds. The default
`TRITON_BUILD_UT=YES` triggers a download of `googletests` code from
GitHub.

See: triton-lang#4414

- [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 improves the build system.

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

2 participants