Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
make pip source installs a bit easier #1048
make pip source installs a bit easier #1048
Changes from all commits
ee40cd7
724647f
7b675a4
72bc8b4
0387f3e
1c2d4de
e6cc176
a39811c
068f556
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How did this change here?
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.
@pentschev accidentally committed it (it was intentional, just not intended for this PR) and proposed just keeping it: #1048 (comment)
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.
Does
ucx-py
actually do anything different between CUDA versions? My understanding was noOnly
libucx
would potentially do different things for different CUDA versionsSo am curious why we want to suffix here. Is this just to provide a way to influence
libucx
indirectly? Or is there not really a need?Edit: Recognize we may just be agreeing, but want to double check that there isn't still something else going on with the suffix
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'll let @pentschev or @vyasr elaborate, but I can share a bit.
I can say confidently it's not ONLY to influence the
libucx
dependency, becauseucx-py
has been published with CUDA suffixes as far back as v0.32 (https://pypi.nvidia.com/ucx-py-cu12/), andlibucx
was only introduced in v0.38 (#1041).But now that we do have
libucx
... the suffixing does serve that purpose of influencing which version you get.e.g.
cugraph-cu12
depends onucx-py-cu12
which depends onlibucx-cu12
I don't think so.
This
disable-cuda=true
in this part of the diff here is removing that suffixing for source builds likepip install .
, based on @pentschev 's feedback that he wanted that workflow to be as simple as possible, especially since in some casesucx-py
is used without any CUDA at all.If this PR was accepted in its current state, we'd still be publishing
ucx-py-cu{version}
wheels, viapip wheel --config-settings rapidsai.disable-cuda=false
(see changes toci/build_wheel.sh
in this PR).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.
John is right, UCX-Py doesn't have a direct dependency on CUDA.
I have previously asked the same question myself, which James answered above.
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.
In that case, couldn't a user just specify the
libucx-cu*
install if they want that? What does it provide to haveucx-py
also including the same information?Would add on the Conda side we don't provide this distinction
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 think the issue is that
ucx-py
needslibucx
, if we don't encode the CUDA version inucx-py
then the user is responsible to install the appropriatelibucx-cu*
which can be error prone. I.e., installingucx-py
withoutlibucx-cu*
as a dependency will leaveucx-py
in an unusable state, whereasucx-py
with alibucx-cu*
dependency would make the correct CUDA version uninstallable (woulducx-py
depend onlibucx-cu11
orlibucx-cu12
?). Please correct me if I'm wrong @jameslamb .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.
Right, because there's
ucx
from conda-forge which builds variants for different CUDA versions.https://github.com/conda-forge/ucx-split-feedstock/blob/b60fb426784cbb6d5cbd384a76c86920737838f0/recipe/meta.yaml#L51-L53
And can constrain everything without needing to use names to disambiguate, by relying on the
cuda-version
metapackage.With wheels and
pip
those same mechanisms don't exist (part of what @msarahan and others have been talking about in https://discuss.python.org/t/implementation-variants-rehashing-and-refocusing/54884).If you want to use
ucx-py
with CUDA-enabled UCX, something somewhere has to get that CUDA-enabled UCX installed on the system... and one that was built against the correct version of CUDA. My understanding of rapidsai/build-planning#57 was that this idea of distributinglibucx
wheels was a response to difficulties users were facing when asked to install UCX themselves outside ofucx-py
.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 wouldn't even go as far as saying "users", even for ourselves it was quite challenging to bundle UCX with the libraries that need it, so much so that we had UCX broken for many months. One step further, this also bloated package sizes.
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.
Prior to the libucx wheels existing, ucx-py statically linked/bundled components of the CTK in order to support running with CUDA. In that world, yes it did have a CUDA dependency because the CUDA transport layer built into the wheel was specific to a given CUDA version. That CUDA dependency has now migrated to the libucx package, so the purpose of the suffix in ucx-py is now to select the appropriate libucx dependency.
This is generally what we've done throughout RAPIDS because if a pure Python package A depends on a CUDA-dependent package B there is no way to express the which CUDA version it should select for B without having separate versions of A. For instance, we do this in
dask-cudf-cuXY
, which has a hard dependency oncudf-cuXY
. ucx-py is a bit of a special case because it works without CUDA, so the CUDA versions are truly optional unlike with something likedask-cudf
(which requirescudf
and therefore must pull a specific CUDA versioned one). In the past we've discussed is having some more automated way of having a default choice of CUDA version, but that still requires a default behavior so you can't get around the different packages in any way that I can tell.For ucx-py, CUDA support is optional , and since rapidsai/ucx-wheels#5 we allow installing libucx on a system without a GPU. I don't see an option for the dependency specification that provides a good UX in all cases though. You either have to force users to manually install dependencies or you make the package not work by default out of the box AFAICT.
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.
Thanks for that! The used-to-statically-link-bits-of-the-CTK was a part I was missing.
I think influencing the
libucx
dependency via a CUDA suffix should continue to be the approach here.