-
Notifications
You must be signed in to change notification settings - Fork 569
Port all conda recipes to rattler-build
#6440
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
Conversation
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
/ok to test |
/ok to test |
/ok to test |
There isn't a GPU on the build worker, so we can't do an import test here
/ok to test |
/ok to test |
/ok to test |
Currently waiting on #6400 |
Here are the finalized run dependencies for the cuda 11 cpp builds (posting this because we currently don't have this tested on cuda 11.4):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some questions, but I strongly suspect most are just from my relative lack of familiarity with rattler-build
and will have answers like "yeah yeah this is expected it's just how rattler-build
works and we've already talked about this in previous migration PRs". So marking this "approve" from a packaging-codeowners
perspective, so that you don't need to wait on a re-review from me.
To review this, after looking through the diff I also looked through logs for stuff like this:
- dependency metadata looks roughly correct
- versions numbers look correct (correctly include the git tag distance)
- the packages built in
conda-{cpp,python}-build
jobs are actually getting installed inconda-*-test
jobs - no significant warnings / errors in any builds or tests
All look ok to me!
Just 1 question... do we care about these warnings and errors for cuml
? I'm not sure what they mean?
⚠ warning Overdepending against dask-cudf
⚠ warning Overdepending against joblib
⚠ warning Overdepending against cuda-cudart
Compiling 1 .py files to .pyc
⚠ warning Error compiling "/tmp/cumlWaVN1x/share/gdb/auto-load/tmp/conda-bld-output/bld/rattler-build_cuml/host_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_p/lib/libarrow.so.1900.1.0-gdb.py" to .pyc:
⚠ warning [Errno 2] No such file or directory: '/tmp/cumlWaVN1x/share/gdb/auto-load/tmp/conda-bld-output/bld/rattler-build_cuml/host_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_p/lib/libarrow.so.1900.1.0-gdb.py'
And these for cuml-cpu
?
⚠ warning Overdepending against numpy
⚠ warning Overdepending against pandas
⚠ warning Overdepending against scikit-learn
⚠ warning Overdepending against hdbscan
⚠ warning Overdepending against umap-learn
⚠ warning Overdepending against nvtx
# 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/libcuml \ | ||
--experimental \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, haven't reviewed one of these PRs before... why are we opting into experimental stuff from rattler-build
? What specifically do we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main usage of --experimental
is the pre-build or cache-build option, otherwise we'd have to compile everything for each output. It's been marked experimental for a while, but we're confident it's not going anywhere (I suspect the main reason it's still gated is that upstream may want to change some names around).
Wolf and I were discussing how the phrase multiple output cache can cause confusion when paired with sccache
and compilation caches -- so maybe pre-build
would be a better name, but that will require breaking changes to the yaml spec.
ci/build_cpp.sh
Outdated
|
||
sccache --show-adv-stats | ||
|
||
# remove build_cache directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding... is this necessary because rattler-build
puts some files into $RAPIDS_CONDA_BLD_OUTPUT_DIR
and rapids-upload-conda-to-s3
is doing something like trying to zip up the entire contents of that directory?
I think it'd be helpful to modify this comment to answer the question "why?" instead of "what?"... it's clear to me from the code that this is removing a directory called build_cache
, but not clear why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's an upstream issue tracking this that I can also link in there.
ci/build_python.sh
Outdated
|
||
sccache --show-adv-stats | ||
sccache --zero-stats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sccache --zero-stats |
This looks like it's unnecessary. It's wasted time on CUDA 11 builds (where no other compilation will happen after this line), and redundant with this existing call on CUDA 12 builds (where we compile cuml-cpu
):
Line 37 in 84946a9
sccache --zero-stats |
Thanks @jameslamb ! I'm going to push up fixes for the things you flagged. We are currently not worrying about overdepending, partially because it's hard to trace out, and also because it's non-critical -- it usually means a dependency is being overspecified, but so long as those are proper duplicates (and not in conflict) then it's not a big issue. We care a lot about overlinking errors, since those might point to missing libraries. The |
Welp, I can't find a similar error in the conda build logs, so let me rebuild locally and see if I can either fix that, or confirm that it's a non-issue. |
Ok, here's a diff of the contents of the 2,3d1
< info/files
< info/git
5d2
< info/has_prefix
7d3
< info/licenses/LICENSE
9d4
< info/recipe/build.sh
11,20c6,16
< info/recipe/meta.yaml
< info/recipe/meta.yaml.template
< info/test/run_test.py
< lib/python3.12/site-packages/cuml-25.4.0a132.dist-info/direct_url.json
< lib/python3.12/site-packages/cuml-25.4.0a132.dist-info/INSTALLER
< lib/python3.12/site-packages/cuml-25.4.0a132.dist-info/licenses/LICENSE
< lib/python3.12/site-packages/cuml-25.4.0a132.dist-info/METADATA
< lib/python3.12/site-packages/cuml-25.4.0a132.dist-info/RECORD
< lib/python3.12/site-packages/cuml-25.4.0a132.dist-info/REQUESTED
< lib/python3.12/site-packages/cuml-25.4.0a132.dist-info/WHEEL
---
> info/recipe/recipe.yaml
> info/recipe/rendered_recipe.yaml
> info/recipe/variant_config.yaml
> info/tests/tests.yaml
> lib/python3.12/site-packages/cuml-25.4.0a150.dist-info/direct_url.json
> lib/python3.12/site-packages/cuml-25.4.0a150.dist-info/INSTALLER
> lib/python3.12/site-packages/cuml-25.4.0a150.dist-info/licenses/LICENSE
> lib/python3.12/site-packages/cuml-25.4.0a150.dist-info/METADATA
> lib/python3.12/site-packages/cuml-25.4.0a150.dist-info/RECORD
> lib/python3.12/site-packages/cuml-25.4.0a150.dist-info/REQUESTED
> lib/python3.12/site-packages/cuml-25.4.0a150.dist-info/WHEEL
674a671
> share/gdb/auto-load/tmp/conda-bld-output/bld/rattler-build_cuml/host_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_p/lib/libarrow.so.1900.1.0-gdb.py All of the changes are expected because of different commit counts and recipe files -- the only other difference is this weird |
Ok, because that If we want to ship it, we can always add it back in, but I think it's just an artifact of the earlier compilation stage. |
/merge |
Description
Port all condabuild recipes over to use
rattler-build
instead.Contributes to rapidsai/build-planning#47
rattler-build
, this changes all the licenses in thepyproject.toml
files to the SPDX-compliantApache-2.0
instead ofApache 2.0