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.

Sign up for GitHub

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

Jump to bottom

make pip source installs a bit easier #1048

Merged
merged 9 commits into from
Jul 3, 2024
Merged

make pip source installs a bit easier #1048

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ee40cd7
make pip source installs a bit easier
jameslamb Jun 13, 2024
724647f
ensure post versions can be found at build time
jameslamb Jun 13, 2024
7b675a4
do not depend on libucx in interactive builds
jameslamb Jun 27, 2024
72bc8b4
Merge branch 'branch-0.39' into docs/pip-install
jameslamb Jul 1, 2024
0387f3e
docs builds could now avoid specifying a CUDA dependency and just lin…
jameslamb Jul 1, 2024
1c2d4de
add ucx back to conda env
jameslamb Jul 1, 2024
e6cc176
revert to passing rapidsai.matrix-entry in docs builds, to continue g…
jameslamb Jul 1, 2024
a39811c
Document PyPI installation
pentschev Jul 2, 2024
068f556
Merge branch 'branch-0.39' into docs/pip-install
jameslamb Jul 2, 2024
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
2 changes: 0 additions & 2 deletions .readthedocs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ build:
post_create_environment:
- |
pip install \
--extra-index-url "https://pypi.anaconda.org/rapidsai-wheels-nightly/simple/" \
-C rapidsai.disable-cuda=true \
-C rapidsai.matrix-entry="cuda=12.2" \
.

Expand Down
2 changes: 1 addition & 1 deletion ci/build_wheel.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ rapids-generate-version > ./VERSION

RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen ${RAPIDS_CUDA_VERSION})"

python -m pip wheel . -w dist --no-deps --disable-pip-version-check
python -m pip wheel . -w dist --no-deps --disable-pip-version-check --config-settings rapidsai.disable-cuda=false

mkdir -p final_dist
python -m auditwheel repair \
Expand Down
8 changes: 5 additions & 3 deletions dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,17 +142,19 @@ dependencies:
- --extra-index-url=https://pypi.nvidia.com
- --extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple
specific:
# very tight >=x.x.x,<x.x.(x+1) here allows for installation of
# post release like 1.15.0.post1
- output_types: [requirements, pyproject]
matrices:
- matrix: {cuda: "12.*"}
packages:
- libucx-cu12==1.15.0
- libucx-cu12>=1.15.0,<1.15.1
vyasr marked this conversation as resolved.
Show resolved Hide resolved
- matrix: {cuda: "11.*"}
packages:
- libucx-cu11==1.15.0
- libucx-cu11>=1.15.0,<1.15.1
- matrix: null
packages:
- libucx==1.15.0
- libucx==1.15.0,<1.15.1
vyasr marked this conversation as resolved.
Show resolved Hide resolved
depends_on_ucx_run:
common:
- output_types: conda
Expand Down
4 changes: 2 additions & 2 deletions docs/source/install.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,6 @@ Building and installing UCX-Py can be done via `pip install`. For example:
conda activate ucx
git clone https://github.com/rapidsai/ucx-py.git
cd ucx-py
pip install -v .
pip install -v -C rapidsai.matrix-entry="cuda=12.2" .
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than always including this, should the default development instructions not enable CUDA, and therefore not specify a matrix entry? This assumes we make the other change I suggest above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a question for @pentschev ?

These docs are under a section that says it's assumed you're building on a system with CUDA.

ucx-py/docs/source/install.rst

Lines 50 to 53 in 7b70211

Source
------
The following instructions assume you'll be using UCX-Py on a CUDA enabled system and is in a `Conda environment <https://docs.conda.io/projects/conda/en/latest/>`_.

I'm happy to change this PR either way, but I don't feel qualified to make the call about whether or not the CUDA-enabled build should be the default.

I DO think that we should continue specifying this flag (and therefore depending on the libucx package) in docs builds here. Since we've seen that those docs builds end up also carrying some test coverage to catch works-without-CUDA types of issues (rapidsai/ucxx#231).

So @pentschev , the specific question for you is, which of these would you prefer?

  • pip install . works with 0 other flags, but requires a system installation of UCX
  • pip install . requires passing -C rapidsai.matrix-entry="cuda12.2" or similar, but in exchange the locally-built package depends on libucx wheels (so no system version required)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed 7b675a4 implementing this, by the way, so you can look at the diff and see what it'd look like.

Copy link
Member

Choose a reason for hiding this comment

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

The docs assume a CUDA system because of the --with-cuda=$CUDA_HOME build flag, which is our primary use case, if that flag is omitted then it should work on non-CUDA systems as well, but we don't document that at the moment.

the specific question for you is, which of these would you prefer?

  • pip install . works with 0 other flags, but requires a system installation of UCX
  • pip install . requires passing -C rapidsai.matrix-entry="cuda12.2" or similar, but in exchange the locally-built package depends on libucx wheels (so no system version required)

Can I have both? 🙂

Ideally, I would like both to work, but given your phrasing it seems this is not possible. So let's assume you have UCX installed on the system and libucx installed, what would happen then both with and without -C rapidsai.matrix-entry="cuda12.2"?

Copy link
Member Author

Choose a reason for hiding this comment

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

assume you have libucx installed

This question is about whether or not pip install . pulls that dependency in for you, not about what happens if it happens to already be installed.

If you've already done something like the following:

apt-get install openucx
pip install libucx-cu12

Then import ucp will call libucx.load_library(), which should find that system installation of libucs, libucm, etc., but will fall back to the libucx-bundled ones if finding them fails for some reason.

Ok so what's the point of -C rapidsai.matrix-entry? libucx isn't distributed as the name libucx.... you have to pick libucx-cu11 (CUDA 11) or libucx-cu12 (CUDA 12). That selection of major CUDA version can happen automatically in rapids-build-backend, but that detection is being turned off here via the use of disable-cuda = true, to support source installation on non-CUDA systems by default.

Another option we could pursue, if you want, is to choose a specific CUDA verrsion as the default one for source installations by e.g. recording libucx-cu12 as a dependency in pyproject.toml. Then pip install . would "just work" with no additional flags on a system with or without CUDA available and with or without a system installation of UCX. But anyone using CUDA 11 would have to pass -C rapidsai.matrix-entry="cuda11.8" or similar, or they'd get a libucx-cu12 (built against CUDA 12), which might lead to runtime issues.

Copy link
Member

Choose a reason for hiding this comment

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

Done in a39811c. Would you mind having a look if everything seems correct to you @jameslamb ?

Also, I've accidentally committed this local change that I was going to push to another PR. It's harmless and corrects the environment now, so if you don't mind let's leave it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's harmless and corrects the environment now, so if you don't mind let's leave it.

Sure, no problem, it's ok to leave it.

Would you mind having a look

Looking right now! Thanks for doing that.

Copy link
Member Author

@jameslamb jameslamb Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd written up a long comment about how the docs should indicate that CPU-only is totally supported by pip install ucx-py-cu{11,12} .... then re-read what you wrote and saw that you already said exactly that 🤦🏻

Anyway, in case it's interesting, here's how I tested that to convince myself:

how I tested that (click me)

Ran the following on my macOS laptop (so no possibility of accidentally finding CUDA):

docker run \
  --rm \
  -it python:3.11 \
  bash

# system-install UCX so we don't link to the the wheel one
apt-get update
apt-get install -y --no-install-recommends \
  libucx-dev

# install ucx-py-cu12 (which also pulls in libucx-cu12)
pip install --extra-index-url https://pypi.nvidia.com 'ucx-py-cu12>=0.38'

# import ucx-py and check where the loader found libraries
python -c "import ucp"
ldconfig -p | grep libuc
#	libuct.so.0 (libc6,AArch64) => /lib/aarch64-linux-gnu/libuct.so.0
#	libuct.so (libc6,AArch64) => /lib/aarch64-linux-gnu/libuct.so
#	libucs_signal.so.0 (libc6,AArch64) => /lib/aarch64-linux-gnu/libucs_signal.so.0
#	libucs_signal.so (libc6,AArch64) => /lib/aarch64-linux-gnu/libucs_signal.so
# ... etc., etc. ... it found them all

# write the demo code from https://github.com/rapidsai/ucx-py/blob/branch-0.39/docs/source/quickstart.rst
cat > server.py <<EOF
import asyncio
import time
import ucp
import numpy as np

n_bytes = 2**30
host = ucp.get_address(ifname='eth0')  # ethernet device name
port = 13337

async def send(ep):
    # recv buffer
    arr = np.empty(n_bytes, dtype='u1')
    await ep.recv(arr)
    assert np.count_nonzero(arr) == np.array(0, dtype=np.int64)
    print("Received NumPy array")

    # increment array and send back
    arr += 1
    print("Sending incremented NumPy array")
    await ep.send(arr)

    await ep.close()
    lf.close()

async def main():
    global lf
    lf = ucp.create_listener(send, port)

    while not lf.closed():
        await asyncio.sleep(0.1)

if __name__ == '__main__':
    asyncio.run(main())
EOF

cat > ./client.py <<EOF
import asyncio
import ucp
import numpy as np

port = 13337
n_bytes = 2**30

async def main():
    host = ucp.get_address(ifname='eth0')  # ethernet device name
    ep = await ucp.create_endpoint(host, port)
    msg = np.zeros(n_bytes, dtype='u1') # create some data to send

    # send message
    print("Send Original NumPy array")
    await ep.send(msg)  # send the real message

    # recv response
    print("Receive Incremented NumPy arrays")
    resp = np.empty_like(msg)
    await ep.recv(resp)  # receive the echo
    await ep.close()
    np.testing.assert_array_equal(msg + 1, resp)
    print("successfully received")

if __name__ == '__main__':
    asyncio.run(main())
EOF

# start the client and server
python ./server.py &
python ./client.py &

Saw output like this:

Send Original NumPy array
Receive Incremented NumPy arrays
Received NumPy array
Sending incremented NumPy array
successfully received

Maybe that'd be a helpful smoke test to add for rapidsai/ucxx#231.

Anyway... everything you wrote looks correct and clear to me! I think this is good to merge. Thanks as always for your patience working through it with me 😊

Copy link
Member

Choose a reason for hiding this comment

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

Smoke tests are always useful, my concern is how would that work in CI? Not sure if it's worth the trouble if we have to add another job at this time, from existing jobs it's probably relatively complicated to do so in a "safe" manner (i.e., ensuring linkage isn't resolved via the proper PyPI/conda UCX package).

I'm also +1 on merging this, I've approved the PR. Thanks so much James for the work here! 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if it's worth the trouble

For sure, this wasn't a proposal to do add a new job or anything right now.

Thanks! I'll merge this once CI passes.

# or for develop build
pip install -v -e .
pip install -v -e -C rapidsai.matrix-entry="cuda=12.2" .
4 changes: 3 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,12 @@ build-backend = "setuptools.build_meta"
commit-files = [
"ucp/COMMIT_FILE"
]
# by default, do not rename the package 'ucx-py-cu${ver}'
disable-cuda=true
dependencies-file = "dependencies.yaml"
requires = [
"cython>=3.0.0",
"libucx==1.15.0",
"libucx==1.15.0,<1.15.1",
] # This list was generated by `rapids-dependency-file-generator`. To make changes, edit dependencies.yaml and run `rapids-dependency-file-generator`.

[tool.setuptools]
Expand Down