-
Notifications
You must be signed in to change notification settings - Fork 60
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
use libucx wheels #1041
Merged
Merged
use libucx wheels #1041
Changes from all commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
1b7399a
use libucx wheels
jameslamb cbb283a
keep libucp
jameslamb 875d582
get all dependencies via conda
jameslamb 14d0233
ignore libucx dependency for conda packages
jameslamb 2f030df
upload artifacts
jameslamb 5541ee1
replace suffix for libucx
jameslamb dc74edd
uncomment uploads again
jameslamb 2b4b6e3
maybe installation of the library itself can be skipped on readthedocs
jameslamb f7098fe
dlopen() earlier
jameslamb e424eb3
add build dpeendency too, remove system installations
jameslamb 0a397f0
install is necessary for docs builds
jameslamb c8d91ca
fix conda deps, try to fix readthedocs builds
jameslamb acecf8e
fix libucx replacement in pyproject.toml
jameslamb e6acd0c
fix lib paths (still not to update rpath/runpath)
jameslamb f8f25c9
remove debugging stuff, switch to ImportError
jameslamb 0c2234c
Merge branch 'branch-0.38' into ucx-wheels
jameslamb 9825f82
Update dependencies.yaml
jameslamb 9bd984c
Update conda/recipes/ucx-py/meta.yaml
jameslamb 0523ba1
make build-time search for libucx insensitive to its internal layout
jameslamb c60800f
grammar
jameslamb 91cbdf5
test with PIP_NO_DEPENDENCIES
jameslamb 95da3ad
try an empty commit... maybe RTD doesnt apply environment variable up…
jameslamb 327e93e
try with PIP_NO_BUILD_ISOLATION
jameslamb 63976f2
just put a real dependency in pyproject.toml
jameslamb 72596ae
build and run
jameslamb 1146231
please
jameslamb 5046584
another empty commit to test removing env variables from the RTD UI
jameslamb 5c90c8e
catch more issues
jameslamb f1f949d
restore CPU support
jameslamb 6723bae
remove try-catch for no-GPU settings
jameslamb ca60927
exclude libucs_signal too
jameslamb fb12aa3
remove ucx from docs conda env
jameslamb eba110f
use the headers from the libucx wheels too
jameslamb fb022ee
restore unsuffixed dependencies in pyproject.toml
jameslamb 37dbad6
run patching script earlier on docs builds
jameslamb e2cffe3
catch run dependencies too
jameslamb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,9 @@ dask-worker-space | |
__pytestcache__ | ||
__pycache__ | ||
*.egg-info/ | ||
final_dist/ | ||
dist/ | ||
.vscode | ||
|
||
*.sw[po] | ||
|
||
*.whl |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
#!/bin/bash | ||
# Copyright (c) 2024 NVIDIA CORPORATION. | ||
# | ||
# [description] | ||
# | ||
# ucx-py's docs builds require installing the library. | ||
# | ||
# It does that by running 'pip install .' from the root of the repo. This script | ||
# is used to modify readthedocs' local checkout of this project's source code prior | ||
# to that 'pip install' being run. | ||
# | ||
# For more, see https://docs.readthedocs.io/en/stable/build-customization.html | ||
# | ||
|
||
set -euo pipefail | ||
|
||
sed -r -i "s/\"libucx/\"libucx-cu12/g" ./pyproject.toml |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
There is no
pip
-installable librarylibucx
... it needs to be eitherlibucx-cu11
orlibucx-cu12
.Readthedocs builds are installing directly from the
pyproject.toml
checked into source control, whichdoesn't have those suffixes added.
ucx-py/.readthedocs.yml
Lines 8 to 11 in 03c864b
Resulting in this:
(example build link)
readthedocs
doesn't allow customizing thepip install
with arbitrary flags, e.g. by adding--no-deps
(docs).So I think this pre-install script to fix the suffixes is the best option to keep those builds working. For more details on how it works, see https://docs.readthedocs.io/en/stable/build-customization.html.
This comment was marked as resolved.
Sorry, something went wrong.
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 the suggestion, I hadn't considered trying to get this into the conda environment file that way!
But I don't think that will work.
conda
converts thepip:
section in anenv.yaml
into arequirements.txt
, and you can only supply arguments that'd be valid in a requirements file passed topip
.various forms of that that I tried (click me)
There are some issues tracking the request "forward custom flags to pip in conda environments" but none have resulted in changes to
conda
:That means it's not possible to include arguments like
--no-deps
.yields the following
And without that
--no-deps
,conda
tries to install all the dependencies of the package, resulting inThe only other thing I can think of is setting environment variable
PIP_NO_DEPS=true
in the build environment. I tried that usingconda
's support for environment variables (docs), but I don't think those affect the build... just the environment's activation.I see 2 options for this:
ucx-py
readthedocs project define environment variablePIP_NO_DEPS=true
in the project's settings (https://docs.readthedocs.io/en/stable/guides/environment-variables.html)I think that environment variable option would be preferable, actually. This project has a small number of top-level dependencies, keeping the necessary ones in the conda environment file isn't a big lift. @vyasr what do you think?
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 don't have a preference, but I can set
PIP_NO_DEPS=true
if you two agree that's the preferred way.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 agree, that seems a bit cleaner to me.
Did you try
PIP_NO_DEPENDENCIES
? I don't remember which is the right name (or maybe both work), and also the behavior of true/false for pip's*_NO_*
variables can be confusing so it's worth double-checking all the options with the setting that you proposed in the env.yaml before we set this on RTD. If all else fails, though, I'm fine with setting on RTD.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.
Ahhhhh I forgot that
PIP_NO_DEPENDENCIES
does not stoppip install
from trying to download build dependencies.(build link)
I just added
PIP_NO_BUILD_ISOLATION="True"
to the environment as well... still didn't work 😞(build link)
I have one other idea, will post in a second.
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.
To summarize.... these things did not work:
PIP_NO_DEPS="True"
in readthedocs buildsPIP_NO_BUILD_ISOLATION="True"
in readthedocs buildsvariables:
block inbuilddocs.yml
conda envpip: - --no-deps ../../
viabuilddocs.yml
conda envlibucx-cu11>=1.15.0
inpyproject.toml
+ add--extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple
inbuilddocs.yml
conda envThat last one failing revealed a more significant issue, not limited to just docs building...
calling
libucx.load_library()
unconditionally meansucx-py
will not be importable unless the process has access to a GPU 😱(build link)
The docs DO BUILD successfully on the current state of this branch 🎉 (build link), but only because I added behavior to work around not having a GPU available.
So @pentschev @vyasr that's a design decision for you.
When someone tries to
import ucp
without access to a GPU, what should happen?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.
Summarizing our offline conversations:
ucx-py
is expected to be usable without a GPUlibucx
wheels should stop raising exceptions whenlibucx.load_library()
is called in an environment without a GPUIn response, we published a new
libucx==1.15.0.post1
removing those exceptions: rapidsai/ucx-wheels#5Tested it with
ucxx
on systems with and without a GPU, and it seemed to work well: rapidsai/ucx-wheels#5 (comment)Those new wheels helped the docs builds here get further, but those builds exposed another issue... as of the changes in this PR,
ucx-py
was compiling with the system-installed headers (e.g./usr/include/ucm
) but the wheel-provided shared libraries (e.g.site-packages/libucx/lib/libucm.so.0
).That showed up in "symbol not found" issues like this:
(build link)
I just pushed a change that fixes that: eba110f
And looks like the docs builds are happy and docs are rendering correctly (RTD build link) 🎉
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.
Good catch on the headers! I forgot to check on those. Glad you found this issue.
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.
Update for those finding this thread from search or other links.... thanks to @bdice 's suggestion in #1044 (comment), we were able to get this working without messing with environment variables or the conda environment YAML file, like this in
.readthedocs.yaml
: