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

[REVIEW] Reduce code duplication for dask & distributed nightly/stable installs #11565

Merged
merged 17 commits into from
Sep 27, 2022

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Aug 18, 2022

Description

After dask/dask#9367 was fixed in dask upstream we had to bump the minimum version of dask to 2022.8.0 to correctly fetch nightly(if channel exists) or stable (if dask/dev label doesn't exist). Without this fix, conda builds were always picking up 2022.7.1 only and/or there would be a mix of nightly & stable packages in an env.

This PR also does some cleanup and makes the build.sh script easy to maintain.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@galipremsagar galipremsagar added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Aug 18, 2022
@github-actions github-actions bot added the gpuCI label Aug 18, 2022
@galipremsagar galipremsagar changed the title test ci [REVIEW] Fix CI script to install dask & distributed nightly/stable based on flag Aug 18, 2022
@galipremsagar galipremsagar changed the title [REVIEW] Fix CI script to install dask & distributed nightly/stable based on flag [REVIEW] Fix CI script to install dask & distributed nightly/stable based on INSTALL_DASK_MAIN Aug 18, 2022
@galipremsagar galipremsagar self-assigned this Aug 18, 2022
@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Aug 18, 2022
@galipremsagar galipremsagar marked this pull request as ready for review August 18, 2022 22:23
@galipremsagar galipremsagar requested a review from a team as a code owner August 18, 2022 22:23
ci/gpu/build.sh Outdated Show resolved Hide resolved
rapids-bot bot pushed a commit that referenced this pull request Aug 19, 2022
Dask-cudf groupby tests *should* be failing as a result of dask/dask#9302 (see [failures](https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cudf/job/prb/job/cudf-gpu-test/CUDA=11.5,GPU_LABEL=driver-495,LINUX_VER=ubuntu20.04,PYTHON=3.9/9946/) in #11565 is merged - where dask/main is being installed correctly).  This PR updates the dask_cudf groupby code to fix these failures.

Authors:
  - Richard (Rick) Zamora (https://github.com/rjzamora)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #11561
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@e64c2da). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head ff21bb4 differs from pull request most recent head 40ed5e5. Consider uploading reports for the commit 40ed5e5 to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10   #11565   +/-   ##
===============================================
  Coverage                ?   87.51%           
===============================================
  Files                   ?      133           
  Lines                   ?    21798           
  Branches                ?        0           
===============================================
  Hits                    ?    19077           
  Misses                  ?     2721           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rjzamora
Copy link
Member

Thanks for taking care of this @galipremsagar !

@galipremsagar galipremsagar added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Aug 19, 2022
@github-actions github-actions bot added the conda label Aug 19, 2022
ci/gpu/build.sh Outdated Show resolved Hide resolved
@galipremsagar galipremsagar removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Aug 22, 2022
@galipremsagar galipremsagar requested a review from a team as a code owner August 22, 2022 21:00
ci/benchmark/build.sh Show resolved Hide resolved
@@ -39,6 +39,9 @@ export LIBCUDF_KERNEL_CACHE_PATH="$HOME/.jitify-cache"
# Dask & Distributed option to install main(nightly) or `conda-forge` packages.
export INSTALL_DASK_MAIN=1

# Dask version to install when `INSTALL_DASK_MAIN=0`
export DASK_STABLE_VERSION="2022.8.0"
Copy link
Member

Choose a reason for hiding this comment

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

Want to note that the relevant fixes for the mixed stable/nightly issue here are in conda-forge/dask-feedstock#191 and conda-forge/distributed-feedstock#218, since our problem is that when installing conda-forge dask we ended up pulling in nightly dask-core/distributed.

With this context, conda-forge/conda-forge-repodata-patches-feedstock#312 applies this change across all stable dask/distributed packages that would've had this issue, so once that's in we shouldn't have to bump the stable version here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I'll hold off until conda-forge/conda-forge-repodata-patches-feedstock#312 is merged and will revert back to the prev stable version that we were pointing to as a minimum.

Co-authored-by: Charles Blackmon-Luca <[email protected]>
Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

A couple suggestions so we can verify that the new stable install command works:

ci/benchmark/build.sh Show resolved Hide resolved
ci/gpu/build.sh Show resolved Hide resolved
ci/gpu/build.sh Outdated Show resolved Hide resolved
ci/benchmark/build.sh Outdated Show resolved Hide resolved
ci/gpu/build.sh Outdated Show resolved Hide resolved
conda/environments/cudf_dev_cuda11.5.yml Outdated Show resolved Hide resolved
conda/recipes/custreamz/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/dask-cudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/dask-cudf/meta.yaml Outdated Show resolved Hide resolved
python/dask_cudf/setup.py Outdated Show resolved Hide resolved
@github-actions github-actions bot removed Python Affects Python cuDF API. conda labels Aug 24, 2022
@charlesbluca
Copy link
Member

After doing some more local testing of conda-forge/conda-forge-repodata-patches-feedstock#312 I realize there's still issues that need to be resolved 🤦🏽

mamba install -c dask/label/dev -c conda-forge dask==2022.7.1
...
  + dask                               2022.7.1  pyhd8ed1ab_0        conda-forge/noarch           5kB
  + dask-core                   2022.7.2a220805  py_g386b4753f_28    dask/label/dev/noarch      864kB
  + distributed                 2022.7.2a220805  py_ge1f3779a_27     dask/label/dev/noarch      758kB

@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.

@galipremsagar
Copy link
Contributor Author

@charlesbluca could you review this PR? I know this is not solving the original problem we were trying to address and it has to be done at dask side, but this does remove a lot of version number duplication and will be helpful while we pin & unpin for release.

@galipremsagar galipremsagar changed the title [REVIEW] Fix CI script to install dask & distributed nightly/stable correctly [REVIEW] Reduce code duplication for dask & distributed nightly/stable installs Sep 27, 2022
ci/benchmark/build.sh Outdated Show resolved Hide resolved
ci/gpu/build.sh Outdated Show resolved Hide resolved
galipremsagar and others added 2 commits September 27, 2022 08:30
Co-authored-by: Charles Blackmon-Luca <[email protected]>
Co-authored-by: Charles Blackmon-Luca <[email protected]>
@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Sep 27, 2022
@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d8feede into rapidsai:branch-22.10 Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants