-
Notifications
You must be signed in to change notification settings - Fork 538
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
nest cuml one level deeper in python #5944
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
23b7d28
to
ae1f540
Compare
460c7b6
to
141881d
Compare
closing/reopening to re-run CI |
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 did this:
git grep 'python/'
And see a few other places that should be changed
Line 10 in 751ba82
python/setup.py @rapidsai/cuml-cmake-codeowners |
cuml/ci/release/update-version.sh
Lines 40 to 44 in 751ba82
# pyproject.toml versions | |
sed_runner "s/rmm==.*\",/rmm==${NEXT_SHORT_TAG_PEP440}.*,>=0.0.0a0\",/g" python/pyproject.toml | |
sed_runner "s/cudf==.*\",/cudf==${NEXT_SHORT_TAG_PEP440}.*,>=0.0.0a0\",/g" python/pyproject.toml | |
sed_runner "s/pylibraft==.*\",/pylibraft==${NEXT_SHORT_TAG_PEP440}.*,>=0.0.0a0\",/g" python/pyproject.toml | |
sed_runner "s/raft-dask==.*\",/raft-dask==${NEXT_SHORT_TAG_PEP440}.*,>=0.0.0a0\",/g" python/pyproject.toml |
cuml/ci/release/update-version.sh
Line 69 in 751ba82
sed_runner "s|/branch-[^/]*/|/branch-${NEXT_SHORT_TAG}/|g" python/README.md |
cuml/wiki/python/ESTIMATOR_GUIDE.md
Line 90 in 751ba82
There are some convenience [Mixins](../../python/common/mixins.py), that the estimator can inherit, can be used for indicating the preferred order (column or row major) as well as for sparse input capability. |
cuml/wiki/python/ESTIMATOR_GUIDE.md
Line 92 in 751ba82
If other tags are needed, they are static (i.e. don't change depending on the instantiated estimator), and more than one estimator will use them, then implement a new [Mixin](../../python/common/mixins.py), if the tag will be used by a single class then implement the `_more_static_tags` method: |
(not sure about this one):
Line 204 in 751ba82
'cuml/blob/{revision}/python/' |
And then technically this one, although it looks like it's a leftover from an older state of these builds and actually isn't used for anything:
Line 65 in 751ba82
PYTHON_DEPS_CLONE=${REPODIR}/python/external_repositories |
Fixed the remaining usages of |
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, left 2 more small suggestions but otherwise this looks good to me. I don't need to re-review.
We might also need to update these lines to use the containers with the new UCX version: - "BASE": "rapidsai/devcontainers:24.08-cpp-cuda11.8-ucx1.15.0-openmpi-ubuntu22.04"
+ "BASE": "rapidsai/devcontainers:24.08-cpp-cuda11.8-ucx1.17.0-openmpi-ubuntu22.04" - "BASE": "rapidsai/devcontainers:24.08-cpp-cuda12.2-ucx1.15.0-openmpi-ubuntu22.04"
+ "BASE": "rapidsai/devcontainers:24.08-cpp-cuda12.2-ucx1.17.0-openmpi-ubuntu22.04" |
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.
python and cmake changes lgtm
/merge |
This is a step towards adding support for dynamic linking with wheels (splitting the shared libraries out into their own wheels). That's being tracked in rapidsai/build-planning#33
This PR performs a necessary step of moving the cuml folder one level deeper, such that the python folder becomes a parent of multiple full-fledged projects, instead of having the python folder be the top level of one python project. This is split into this PR because this change affects so many files. It's easier to review the actual changes of supporting the split wheel when you don't have to also consider these moves.
This change also affects devcontainers, and there will need to be a change similar to rapidsai/devcontainers#283 for cuml.