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.

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

Pin libcudf runtime dependency for cudf / libcudf-kafka nightlies #9847

Merged

Conversation

charlesbluca
Copy link
Member

@charlesbluca charlesbluca commented Dec 6, 2021

Looking at the cudf nightlies on Anaconda, I noticed we aren't pinning the libcudf dependency, only constraining it to the current minor version (i.e. libcudf >=22.2.0a.211206,<22.3.0a0); this makes it possible to install mismatched versions of cudf and libcudf nightlies, which can result in a broken cudf installation.

This PR pins the libcudf runtime dependency for our cudf nightlies, as well as for libcudf-kafka, which also has a libcudf runtime dependency.

@charlesbluca charlesbluca requested a review from a team as a code owner December 6, 2021 20:43
@charlesbluca charlesbluca added 3 - Ready for Review Ready for review by team conda non-breaking Non-breaking change bug Something isn't working labels Dec 6, 2021
@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #9847 (84582ea) into branch-22.04 (a7d88cd) will increase coverage by 75.71%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##           branch-22.04    #9847       +/-   ##
=================================================
+ Coverage         10.42%   86.13%   +75.71%     
=================================================
  Files               119      139       +20     
  Lines             20603    22433     +1830     
=================================================
+ Hits               2148    19323    +17175     
+ Misses            18455     3110    -15345     
Impacted Files Coverage Δ
...ython/custreamz/custreamz/tests/test_dataframes.py 99.39% <0.00%> (-0.01%) ⬇️
python/custreamz/custreamz/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/udf/pipeline.py
...thon/dask_cudf/dask_cudf/tests/test_distributed.py 86.79% <0.00%> (ø)
python/cudf/cudf/core/mixins/reductions.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/mixins/__init__.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/tests/test_dispatch.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/mixins/binops.py 100.00% <0.00%> (ø)
... and 105 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64ee514...84582ea. Read the comment docs.

Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Hmm. It looks like it used to be in the recipe many years ago, but was removed here by @kkraus14. Not sure if he has any input here. Looks fine to me though, so I'm inclined to approve.

@charlesbluca
Copy link
Member Author

Wondering if we should also do this for libcudf_kafka, which also has libcudf as an unpinned dependency for its nightlies?

@charlesbluca charlesbluca changed the title Pin libcudf dependency for cudf nightlies Pin libcudf dependency for cudf / libcudf-kafka nightlies Dec 15, 2021
@charlesbluca charlesbluca changed the title Pin libcudf dependency for cudf / libcudf-kafka nightlies Pin libcudf runtime dependency for cudf / libcudf-kafka nightlies Dec 15, 2021
@kkraus14
Copy link
Collaborator

Hmm. It looks like it used to be in the recipe many years ago, but was removed here by @kkraus14. Not sure if he has any input here. Looks fine to me though, so I'm inclined to approve.

IIRC this was very early on in cudf and the desire at the time was to allow changing the nightly libcudf version underneath cudf to pull in nightly bug fixes in libcudf without necessarily pulling in changes in cudf. It likely isn't relevant anymore now that cudf is a lot more mature.

@charlesbluca
Copy link
Member Author

Thanks for the clarification @kkraus14 🙂 in that case, think we should be safe to merge this

@charlesbluca charlesbluca marked this pull request as draft December 15, 2021 18:51
@charlesbluca
Copy link
Member Author

Hmm actually I didn't consider the case where the versions match up but the build strings don't; for example, it would still be possible to do something like

mamba install cudf=*=cuda_11_py38_g38631a635f_189 libcudf=*=cuda11_gdb9aef8181_190

And end up with a broken cuDF installation (which is exactly what happened in this erroneous image build for dask-sql's gpuCI).

Would it be too restrictive to pin nightlies by their git describe tag and number?

    - libcudf {{ version }} *{{ GIT_DESCRIBE_HASH }}_{{ GIT_DESCRIBE_NUMBER }}

@ajschmidt8
Copy link
Member

Hmm actually I didn't consider the case where the versions match up but the build strings don't; for example, it would still be possible to do something like

mamba install cudf=*=cuda_11_py38_g38631a635f_189 libcudf=*=cuda11_gdb9aef8181_190

And end up with a broken cuDF installation (which is exactly what happened in this erroneous image build for dask-sql's gpuCI).

Would it be too restrictive to pin nightlies by their git describe tag and number?

    - libcudf {{ version }} *{{ GIT_DESCRIBE_HASH }}_{{ GIT_DESCRIBE_NUMBER }}

Hmmm. I think that was attempted before and caused some issues: #7793

@charlesbluca
Copy link
Member Author

In that case I'm okay with merging this for now, as it should at least reduce some conda solving issues. I'm interested in what the failures were cc @raydouglass in case you recall - it looks like the pinning was originally meant to solve https://github.com/rapidsai/ops/issues/1490 but it didn't?

@charlesbluca charlesbluca marked this pull request as ready for review January 13, 2022 14:53
@charlesbluca charlesbluca changed the base branch from branch-22.02 to branch-22.04 January 19, 2022 22:30
@ajschmidt8
Copy link
Member

@charlesbluca, for reference, we're currently exploring a lot of improvements to our current build/CI system.

One of the things we're looking into is consolidating our recipes, similarly to what's described here (particularly the screenshot below). This would let us build the CPP and Python packages from a single recipe and pin the Python packages to the exact CPP package from the same build. No word on any ETAs yet, but just wanted to let you know that this is something we're looking into.

image

@github-actions
Copy link

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

LGTM. We can discuss this more with @jakirkham in the upcoming meeting that I mentioned to you. For now, this is in line with how cuml does things, so it should be fine.

@charlesbluca
Copy link
Member Author

@gpucibot merge

@ajschmidt8
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e9876cf into rapidsai:branch-22.04 Mar 8, 2022
@ajschmidt8
Copy link
Member

(I deployed some updates that broke the merge bot which is probably why the initial @gpucibot merge command didn't work. They've since been reverted)

@charlesbluca charlesbluca deleted the pin-cudf-nightly-deps branch July 19, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants