-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add wheel builds to cuxfilter #497
Conversation
ae19cc1
to
3f8ee02
Compare
6bf963c
to
99d4fde
Compare
5aaffec
to
fd70ca3
Compare
…README.md that does not exist
Co-authored-by: AJ Schmidt <ajschmidt8@users.noreply.github.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
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.
Suggestions attached -- nice work @AjayThorve, really glad to have these updates.
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
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.
LGTM!
with: | ||
build_type: pull-request | ||
package-name: cuxfilter | ||
test-unittest: "python -m pytest -n 8 ./python/cuxfilter/tests" |
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.
You'll need to borrow some logic from here for dask-cuda: https://github.com/rapidsai/cudf/blob/1899fb1990eae693bd74f92fa4a3f6a217889210/.github/workflows/pr.yaml#L132-L133
dask-cuda has to install from source, since the 23.8.*a*
packages on PyPI are pre-release / alphas. We can't really use pip install ... --pre
because it uses pre-release versions of all packages.
Also note that you'll need to change update-version.sh
to accomodate this. https://github.com/rapidsai/cudf/blob/1899fb1990eae693bd74f92fa4a3f6a217889210/ci/release/update-version.sh#L112
I'm not sure if you need just dask-cuda or also dask/distributed so I have suggestions for both.
All of dask/distributed/dask-cuda:
test-unittest: "python -m pytest -n 8 ./python/cuxfilter/tests" | |
# Test against latest dask/distributed/dask-cuda. | |
test-before: "python -m pip install git+https://github.com/dask/dask.git@main git+https://github.com/dask/distributed.git@main git+https://github.com/rapidsai/dask-cuda.git@branch-23.08" | |
test-unittest: "python -m pytest -n 8 ./python/cuxfilter/tests" |
Only dask-cuda:
test-unittest: "python -m pytest -n 8 ./python/cuxfilter/tests" | |
# Test against latest dask-cuda. | |
test-before: "python -m pip install git+https://github.com/rapidsai/dask-cuda.git@branch-23.08" | |
test-unittest: "python -m pytest -n 8 ./python/cuxfilter/tests" |
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.
ah okay got it. It would just be dask-cuda at this point. Thanks again!
@bdice I am assuming this is a temporary fix, until dask-cuda has a PyPI stable release (23.8.*?), right?
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.
Actually @bdice, we don't need dask_cuda
as a direct dependency, since it's only used in the dask_cudf based notebooks. So I removed the direct dependency, and added it to notebooks
dependency only
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.
Great that the dependency could be moved.
To answer your question about whether this would be a temporary fix, it isn't temporary. The dask-cuda wheels used in CI are always pre-release alphas. We are always testing RAPIDS packages on branch-XX.YY
against pre-releases of dask-cuda XX.YY
to ensure that when the releases are made, everything works together as intended.
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.
got it, good to know. Thanks again for the thorough review. Let me know if this looks good to merge.
/merge |
Thanks everyone for the reviews! |
Contributes to rapidsai/build-planning#31 Removes `.gitattributes` files. These were added in #66 for use with `versioneer`. Per the `git` docs ([link](https://git-scm.com/docs/gitattributes#_export_subst)), setting the attribute `export-subst` on a file via a `.gitattributes` tell `git` to replace placeholders in the file with some `git` information. This is no longer done in `_version.py` files in this project, and this project no longer uses `versioneer` (#497). `rapids-build-backend` handles storing git commit information in the published packages. ## Notes for Reviewers For more details, see rapidsai/build-planning#31 (comment) Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Mike Sarahan (https://github.com/msarahan) - Ajay Thorve (https://github.com/AjayThorve) URL: #603
This PR aims to add wheels builds to cuxfilter
Resolves #491
cc @bdice @exactlyallan