remove 'wget' conda dependency, re-organize dependencies.yaml #4805
Conversation
jameslamb
added
improvement
|
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 |
|
Tests here are failing like this: Documented that in #4823 ... there are some changes in |
|
/ok to test |
|
Oh great thanks for merging |
|
/ok to test |
jameslamb
changed the title
|
/ok to test |
jameslamb
changed the title
dependencies.yaml
Outdated
| @@ -565,6 +562,31 @@ dependencies: | |||
| - pylibwholegraph-cu11==25.2.*,>=0.0.0a0 | |||
| - {matrix: null, packages: [*pylibwholegraph_unsuffixed]} | |||
|
|
|||
| depends_on_librmm: | |||
There was a problem hiding this comment.
We probably want the same for other packages too? libcudf, libraft? It’s okay to defer on this in the name of gradual improvement.
There was a problem hiding this comment.
Sure, we can go further in this PR. I just pushed 64d0fad moving more of the RAPIDS dependencies into these depends_on_* lists.
Some notes on other changes in that commit:
- proposes removing explicit
libraft-headerslistinglibraftis explicitly listed, and directly pulls that in (recipe link)- normally I'd advise against relying on transitive relationships like this implicitly, but in this case I think it's ok... the
libraftconda package's purpose is to pull in the compiled bits of RAFT and the headers needed to link against them - if you disagree I'll happily add a
depends_on_libraft_headers, don't feel THAT strongly about it either way
- removes
pyprojectandrequirementsentries forlibcudf,libraft,andlibrmm- because
libcudfis only used for benchmarks here, which aren't built from wheels - because
libraftwheels don't actually exist yet - because
librmmwheels aren't being used here yet - I'll add
libraftandlibrmmentires in WIP: introduce libcugraph wheels #4804 - these
requirementsdependencies would all end up in the devcontainers environment: https://github.com/rapidsai/devcontainers/blob/branch-25.02/features/src/rapids-build-utils/opt/rapids-build-utils/bin/make-pip-dependencies.sh#L46
- because
Requesting a re-review, that's too much to merge on a stale approval.
There was a problem hiding this comment.
Yes, it is okay to use libraft to get its headers transitively.
The other bits are fine, too. I’ll re-review, just make sure update-version.sh and conda recipes reflect things correctly.
There was a problem hiding this comment.
Alright great, thanks.
update version.sh and conda recipes
Just did that in 34697a6.
- removed
libraft-headersfrom conda recipes andupdate-version.sh - put a floor on
thriftpy2independencies.yamlto match what conda recipes were using
dependencies.yaml
Outdated
| common: | ||
| - output_types: conda | ||
| packages: | ||
| - &cugraph_service_client_unsuffixed cugraph-service-client==25.2.*,>=0.0.0a0 |
There was a problem hiding this comment.
This package does not use CUDA or have CUDA dependencies, so it should never be suffixed. This can be simplified.
References:
There was a problem hiding this comment.
Ah good catch, thanks. Guess the suffixes that were in the file before were unnecessary. I pushed 7fc9473 removing the suffix-related stuff for that.
|
/merge |
In #4804, I've started working on adding
libcugraphwheels.This includes a few fixes for things I noticed while doing that:
wgetfromtest_notebookenvironmentdependencies.yamlre-organization:librmmdependency out into adepends_on_librmmto reduce duplication, and for consistency with other RAPIDS dependenciesdepends_on_pylibwholegraphgroup everywhere instead of repeatingpylibwholegraphin multiple placesNotes for Reviewers
I'd originally wanted to also add a
librmmwheel dependency for wheel builds here, but looks like doing that resulted in a lot moresccachemisses, I guess because of building with build isolation. That change will happen in #4804 .