Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

port to rattler-build #1796

Open
wants to merge 54 commits into
base: branch-25.04
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
8ba7466
WIP: convert to rattler-build
gforsyth Jan 24, 2025
a11cbe2
wip: broken, but it at least tries to build
gforsyth Jan 27, 2025
93633b4
restore meta.yaml with changes required for `conda-recipe-manager`
gforsyth Jan 27, 2025
8fd3ef3
Update conda/recipes/librmm/meta.yaml
gforsyth Jan 27, 2025
b501ca1
Update conda/recipes/librmm/meta.yaml
gforsyth Jan 28, 2025
17f9f13
builds, but compiles for each output which is bad
gforsyth Jan 28, 2025
38c6feb
build once and cache, then define separate outputs
gforsyth Jan 28, 2025
9b82fea
port rmm conda recipe to rattler build
gforsyth Jan 28, 2025
be5dbef
add comment about testing environmetn
gforsyth Jan 28, 2025
4994e5c
set GIT_* from env, fix missing torch file
gforsyth Jan 28, 2025
dff85ca
Fix path to `_torch_allocator.cpp` in `.gitignore`
gforsyth Jan 29, 2025
2d251cf
Disable pip check in import test
gforsyth Jan 29, 2025
be8c03f
Swap in `rattler-build` for `mambabuild`
gforsyth Jan 29, 2025
9303d66
install rattler-build
gforsyth Jan 29, 2025
84b9c14
explicitly set CMAKE_MAKE_PROGRAM
gforsyth Jan 29, 2025
c9e4fca
Revert "explicitly set CMAKE_MAKE_PROGRAM"
gforsyth Jan 29, 2025
5ae6a16
add `make` to build
gforsyth Jan 29, 2025
00230cc
add cmake to individual outputs
gforsyth Jan 29, 2025
1fc7c42
explicitly set output directory
gforsyth Jan 29, 2025
e554922
pass `-GNinja` to `build.sh`
gforsyth Jan 29, 2025
5df221c
remove output/ from gitignore
gforsyth Jan 29, 2025
272c466
remove old meta.yaml
gforsyth Jan 29, 2025
5799ee2
use env-var for rattler output dir
gforsyth Jan 29, 2025
1dd5a9a
update copyright
gforsyth Jan 29, 2025
2b769b9
Switch to v1 selectors
gforsyth Jan 30, 2025
9d053ae
Merge branch 'branch-25.04' into rattler-test
gforsyth Jan 30, 2025
dd98e59
Fix indentation
gforsyth Jan 30, 2025
33e7351
explicit channel specification in python build
gforsyth Jan 30, 2025
a9067b2
move env vars to `env` and set defaults
gforsyth Jan 31, 2025
c70f762
see if ninja gets used automatically without flag
gforsyth Jan 31, 2025
f1bf18f
Merge remote-tracking branch 'upstream/branch-25.04' into rattler-test
gforsyth Jan 31, 2025
fe8c6f4
set `SCCACHE_S3_KEY_PREFIX` from env
gforsyth Jan 31, 2025
2b9755b
overwrite unused envvar
gforsyth Feb 3, 2025
cdb334d
Remove manual env-var setting and rattler install
gforsyth Feb 5, 2025
d44eade
Merge branch 'branch-25.04' into rattler-test
gforsyth Feb 5, 2025
6f3b103
Update conda/recipes/librmm/recipe.yaml
gforsyth Feb 5, 2025
7ee96f4
Add missing right-brace
gforsyth Feb 5, 2025
de85796
remove unnecessary branching for cuda compiler
gforsyth Feb 6, 2025
8b2e801
Add note re; build ids and remove build-cache before upload
gforsyth Feb 7, 2025
a3bdd12
Remove `GIT_DESCRIBE_*` envvars from build and pipeline
gforsyth Feb 10, 2025
5489596
Explicitly list channels for build
gforsyth Feb 10, 2025
39856c1
Use double-quotes everywhere
gforsyth Feb 10, 2025
f0b65fe
Use `load_from_file` for `about` metadata, standardize ordering
gforsyth Feb 10, 2025
a05975f
Set missing `py_version`
gforsyth Feb 10, 2025
d51e58a
Remove `python` bounds
gforsyth Feb 10, 2025
de87ce8
Remove extraneous `else` from `conda-recipe-manager`
gforsyth Feb 10, 2025
f5e4669
Use `git` plugin to grab short sha
gforsyth Feb 10, 2025
7d17397
Restore python upper-bound
gforsyth Feb 10, 2025
9c4bca8
Remove `fmt` and `spdlog` from `host`
gforsyth Feb 10, 2025
5d0a244
Add lsp schema for rattler yaml
gforsyth Feb 10, 2025
13420c3
simplify sccache s3 key prefix
gforsyth Feb 10, 2025
384d2d4
remove upper bound on python and remove numba
gforsyth Feb 10, 2025
ea232d5
Merge branch 'branch-25.04' into rattler-test
gforsyth Feb 10, 2025
47c2e40
swap in `rapids-logger`
gforsyth Feb 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ rmm.egg-info/
python/build
python/*/build
python/rmm/docs/_build
python/rmm/**/librmm/**/*.cpp
!python/rmm/librmm/_torch_allocator.cpp
python/rmm/rmm/librmm/*.cpp
!python/rmm/rmm/librmm/_torch_allocator.cpp
gforsyth marked this conversation as resolved.
Show resolved Hide resolved
python/rmm/**/librmm/**/*.h
python/rmm/**/librmm/.nfs*
python/rmm/**/pylibrmm/**/*.cpp
Expand Down
20 changes: 18 additions & 2 deletions ci/build_cpp.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,25 @@ rapids-logger "Begin cpp build"

sccache --zero-stats

# This calls mambabuild when boa is installed (as is the case in the CI images)
RAPIDS_PACKAGE_VERSION=$(rapids-generate-version) rapids-conda-retry mambabuild conda/recipes/librmm
RAPIDS_PACKAGE_VERSION=$(rapids-generate-version)
export RAPIDS_PACKAGE_VERSION

# --no-build-id allows for caching with `sccache`
# more info is available at
# https://rattler.build/latest/tips_and_tricks/#using-sccache-or-ccache-with-rattler-build
rattler-build build --recipe conda/recipes/librmm \
--experimental \
gforsyth marked this conversation as resolved.
Show resolved Hide resolved
--no-build-id \
gforsyth marked this conversation as resolved.
Show resolved Hide resolved
--channel-priority disabled \
--output-dir "$RAPIDS_CONDA_BLD_OUTPUT_DIR" \
-c rapidsai \
-c rapidsai-nightly \
Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we actually need both of these channels or if we should tighten things up using rapids-is-release-build to select one or the other.

-c conda-forge \
-c nvidia

sccache --show-adv-stats

# remove build_cache directory
rm -rf "$RAPIDS_CONDA_BLD_OUTPUT_DIR"/build_cache

rapids-upload-conda-to-s3 cpp
20 changes: 18 additions & 2 deletions ci/build_python.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,25 @@ CPP_CHANNEL=$(rapids-download-conda-from-s3 cpp)

sccache --zero-stats

# This calls mambabuild when boa is installed (as is the case in the CI images)
RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild -c "${CPP_CHANNEL}" conda/recipes/rmm
RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION)
export RAPIDS_PACKAGE_VERSION

# --no-build-id allows for caching with `sccache`
# more info is available at
# https://rattler.build/latest/tips_and_tricks/#using-sccache-or-ccache-with-rattler-build
rattler-build build --recipe conda/recipes/rmm \
--experimental \
--no-build-id \
--channel-priority disabled \
--output-dir "$RAPIDS_CONDA_BLD_OUTPUT_DIR" \
-c "${CPP_CHANNEL}" \
-c rapidsai \
-c rapidsai-nightly \
-c conda-forge \
-c nvidia
gforsyth marked this conversation as resolved.
Show resolved Hide resolved

sccache --show-adv-stats

rm -rf "$RAPIDS_CONDA_BLD_OUTPUT_DIR"/build_cache

rapids-upload-conda-to-s3 python
110 changes: 0 additions & 110 deletions conda/recipes/librmm/meta.yaml

This file was deleted.

105 changes: 105 additions & 0 deletions conda/recipes/librmm/recipe.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# yaml-language-server: $schema=https://raw.githubusercontent.com/prefix-dev/recipe-format/main/schema.json
# Copyright (c) 2018-2025, NVIDIA CORPORATION.
schema_version: 1

context:
version: ${{ env.get("RAPIDS_PACKAGE_VERSION") }}
cuda_version: ${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[:2] | join(".") }}
cuda_major: ${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[0] }}
date_string: ${{ env.get("RAPIDS_DATE_STRING") }}
head_rev: ${{ git.head_rev( "." )[:8] }}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need spaces around the parameter.

Suggested change
head_rev: ${{ git.head_rev( "." )[:8] }}
head_rev: ${{ git.head_rev(".")[:8] }}


recipe:
name: librmm-split

cache:
source:
path: ../../..

requirements:
build:
- cmake ${{ cmake_version }}
- ninja
- ${{ compiler("c") }}
- ${{ compiler("cxx") }}
- ${{ compiler("cuda") }}
- cuda-version =${{ cuda_version }}
- ${{ stdlib("c") }}

build:
script:
file: build.sh
gforsyth marked this conversation as resolved.
Show resolved Hide resolved
secrets:
- AWS_ACCESS_KEY_ID
- AWS_SECRET_ACCESS_KEY
- AWS_SESSION_TOKEN

env:
CMAKE_C_COMPILER_LAUNCHER: ${{ env.get("CMAKE_C_COMPILER_LAUNCHER", default="sccache") }}
CMAKE_CUDA_COMPILER_LAUNCHER: ${{ env.get("CMAKE_CUDA_COMPILER_LAUNCHER", default="sccache") }}
CMAKE_CXX_COMPILER_LAUNCHER: ${{ env.get("CMAKE_CXX_COMPILER_LAUNCHER", default="sccache") }}
CMAKE_GENERATOR: ${{ env.get("CMAKE_GENERATOR", default="Ninja") }}
PARALLEL_LEVEL: ${{ env.get("PARALLEL_LEVEL", default="4") }}
SCCACHE_BUCKET: ${{ env.get("SCCACHE_BUCKET", default="") }}
SCCACHE_IDLE_TIMEOUT: ${{ env.get("SCCACHE_IDLE_TIMEOUT", default="") }}
SCCACHE_REGION: ${{ env.get("SCCACHE_REGION", default="") }}
SCCACHE_S3_USE_SSL: ${{ env.get("SCCACHE_S3_USE_SSL", default="") }}
SCCACHE_S3_NO_CREDENTIALS: ${{ env.get("SCCACHE_S3_NO_CREDENTIALS", default="") }}
SCCACHE_S3_KEY_PREFIX: librmm-${{ env.get("RAPIDS_CONDA_ARCH", default="") }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want a default here. Otherwise we might get behavior that tries to read/write librmm- when the default "" is used. That would be a bug.

Can we make all of these env vars required? Certainly all the sccache-related ones should be mandatory imo, as they force our CI to be correct. For various reasons, such as our deep reliance on gha-tools, we don't expect recipes to be built outside of our CI images, so I am okay with having the recipes enforce correctness in CI.


outputs:
- package:
name: librmm
version: ${{ version }}
build:
string: cuda${{ cuda_major }}_${{ date_string }}_${{ head_rev }}
script: install_librmm.sh
requirements:
host:
- cmake ${{ cmake_version }}
- cuda-version =${{ cuda_version }}
run:
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="x") }}
- if: cuda_major == "11"
then: cudatoolkit
- rapids-logger =0.1
run_exports:
- ${{ pin_subpackage("librmm", upper_bound="x.x") }}
ignore_run_exports:
from_package:
- ${{ compiler("cuda") }}
tests:
- script:
- "test -d \"${PREFIX}/include/rmm\""
about:
gforsyth marked this conversation as resolved.
Show resolved Hide resolved
homepage: ${{ load_from_file("python/librmm/pyproject.toml").project.urls.Homepage }}
license: ${{ load_from_file("python/librmm/pyproject.toml").project.license.text | replace(" ", "-") }}
Copy link
Contributor

@bdice bdice Feb 10, 2025

Choose a reason for hiding this comment

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

We should probably change python/librmm/pyproject.toml to use Apache-2.0 as an SPDX identifier. https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license

Like this:

- license = { text = "Apache 2.0" }
+ license = "Apache-2.0"

@vyasr Do you know why we chose to write it this way in #1529?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably fix this across the board -- I would suggest raising this with ops (in case they know of restrictions that I do not know). If ops is supportive, let's open a build-planning issue and audit this for all repos. Non-OSS repos may need a different solution, but Apache/BSD-3 repos should be fixable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then this will look like:

Suggested change
license: ${{ load_from_file("python/librmm/pyproject.toml").project.license.text | replace(" ", "-") }}
license: ${{ load_from_file("python/librmm/pyproject.toml").project.license }}

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the current choices were made in order to guarantee compatibility with wheeltamer. @raydouglass may remember the exact list of "allowed" licenses. Given that we no longer run wheeltamer before releases that is a moot point, but I don't know if there are any subsequent scans that we have reinstated where this could still be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good -- that's exactly what I wondered. If we can adopt a normal SPDX license identifier in our pyproject.toml files, we absolutely should. I am okay with that being a follow-up to this PR, though.

summary: ${{ load_from_file("python/librmm/pyproject.toml").project.description }}
- package:
name: librmm-tests
version: ${{ version }}
build:
string: cuda${{ cuda_major }}_${{ date_string }}_${{ head_rev }}
script: install_librmm_tests.sh
requirements:
host:
- cmake ${{ cmake_version }}
- cuda-version =${{ cuda_version }}
- if: cuda_major == "11"
then: cudatoolkit
else: cuda-cudart-dev
run:
- if: cuda_major == "11"
then: cudatoolkit
else: cuda-cudart
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="x") }}
- ${{ pin_subpackage("librmm", exact=True) }}
ignore_run_exports:
from_package:
- ${{ compiler("cuda") }}
- if: cuda_major == "11"
then: cuda-cudart-dev
about:
homepage: ${{ load_from_file("python/librmm/pyproject.toml").project.urls.Homepage }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we load_from_file once into something in the context and then access that data? I don't know if rattler is smart enough to cache the file on its own.

license: ${{ load_from_file("python/librmm/pyproject.toml").project.license.text | replace(" ", "-") }}
summary: librmm test & benchmark executables
90 changes: 0 additions & 90 deletions conda/recipes/rmm/meta.yaml

This file was deleted.

Loading