-
Notifications
You must be signed in to change notification settings - Fork 927
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
remove streamz git dependency, standardize build dependency names, consolidate some dependency lists #16611
remove streamz git dependency, standardize build dependency names, consolidate some dependency lists #16611
Conversation
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.
Approving in the current state but I have a proposal for further improvement.
dependencies.yaml
Outdated
- output_types: requirements | ||
packages: | ||
# pip recognizes the index as a global option for the requirements.txt file | ||
# This index is needed for rmm-cu{11,12}. | ||
- --extra-index-url=https://pypi.nvidia.com | ||
- --extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple | ||
- git+https://github.com/python-streamz/streamz.git@master | ||
specific: |
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 we can refactor the py_rapids_build_cudf
list to include lists named depends_on_rmm
and depends_on_pylibcudf
rather than specifying those packages here. We already have a depends_on_pylibcudf
that we can use, but we should add a depends_on_rmm
list. It'd help deduplicate a lot.
That would make build_python_cudf
an empty dependency list, which is probably fine.
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 totally agree! I'm happy to just push that here as well and use this PR for more dependencies.yaml
refactoring. It'd help with #15483.
I'll do that and request another review.
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.
Ok @bdice I just pushed 8bfbb95, which does the following:
- adds a
depends_on_rmm
list - removes
build_python_pylibcudf
(which only held anrmm
requirement) - removes
build_python_cudf
(which only heldrmm
andpylibcudf
requirements) - corresponding changes in
files:
section (including some reordering so dependency lists are alphabetical)
Removes unnecessary installation of `cudf` wheels in wheel testing for `cudf_polars`. `cudf_polars` doesn't depend on `cudf`, and neither do its tests. However, right now it's downloading `cudf` during it's wheel tests. I mistakenly introduced that in #16575. This introduced a race condition that could lead to CI failures whenever the `cudf` wheels aren't published yet by the time the `cudf_polars` tests. Because the `cudf_polars` wheel tests (rightly) do not wait for `cudf` wheels to be available: https://github.com/rapidsai/cudf/blob/555734dee7a8fb10f50c8609a8e4fb2c025e6305/.github/workflows/pr.yaml#L154-L155 https://github.com/rapidsai/cudf/blob/555734dee7a8fb10f50c8609a8e4fb2c025e6305/.github/workflows/pr.yaml#L145-L146 Noticed this in #16611 ```text [rapids-download-from-s3] Downloading and decompressing s3://rapids-downloads/ci/cudf/pull-request/16611/a6b7eff/cudf_wheel_python_cudf_cu12_py310_x86_64.tar.gz into ./dist download failed: s3://rapids-downloads/ci/cudf/pull-request/16611/a6b7eff/cudf_wheel_python_cudf_cu12_py310_x86_64.tar.gz to - An error occurred (404) when calling the HeadObject operation: Not Found ``` ([build link](https://github.com/rapidsai/cudf/actions/runs/10472939821/job/29004728278?pr=16611)) Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: #16612
Thanks for the help @bdice ! I'm happy to see this get simplified 😊 |
/merge |
Description
Proposes some additional cleanup in
dependencies.yaml
, for things I noticed while working through #15483.files:
section for build dependenciespy_build_{project}
= dependencies for the[build-system]
tablepy_rapids_build_{project}
= dependencies for the[tool.rapids-build-backend]
tablecudf
was one of the first repos to addrapids-build-backend
streamz
from latest source on GitHubcustreamz
conda packages and wheels depend on packages for those, not this git dependencycudf/dependencies.yaml
Lines 752 to 754 in 2f7d354
cudf/conda/recipes/custreamz/meta.yaml
Lines 45 to 47 in 2f7d354
build_python_cudf
setstreamz
was 2 years ago (https://github.com/python-streamz/streamz), this doesn't seem like arapids-dask-dependency
, try-to-always-test-against-latest, situation to mestreamz
was regularly publishing wheels... it's been independencies.yaml
since that file was first introduced here in November 2022 (Bifurcate Dependency Lists [skip-gpuci] #11674)master
since then (history link) ... but ifcustreamz
really needed those, I'd expectcustreamz
to depend on the version built from GitHub sources. I strongly suspect that that isn't the case.build_python_cudf
andbuild_python_libcudf
lists independencies.yaml
, in favor of re-using thedepends_on_rmm
anddepends_on_pylibcudf
listsChecklist