Skip to content

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

Merged
rapids-bot[bot] merged 17 commits intorapidsai:branch-22.10from
galipremsagar:test_dask
Sep 27, 2022
Merged

[REVIEW] Reduce code duplication for dask & distributed nightly/stable installs#11565
rapids-bot[bot] merged 17 commits intorapidsai:branch-22.10from
galipremsagar:test_dask

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 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 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
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 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
@galipremsagar galipremsagar removed the DO NOT MERGE Hold off on merging; see PR for details label Aug 22, 2022
@github-actions github-actions bot added the Python Affects Python cuDF API. label Aug 22, 2022
@galipremsagar galipremsagar changed the title [REVIEW] Fix CI script to install dask & distributed nightly/stable based on INSTALL_DASK_MAIN [REVIEW] Fix CI script to install dask & distributed nightly/stable correctly Aug 22, 2022
@galipremsagar galipremsagar requested a review from a team as a code owner August 22, 2022 21:00
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 <20627856+charlesbluca@users.noreply.github.com>
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:

@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
galipremsagar and others added 2 commits September 27, 2022 08:30
Co-authored-by: Charles Blackmon-Luca <20627856+charlesbluca@users.noreply.github.com>
Co-authored-by: Charles Blackmon-Luca <20627856+charlesbluca@users.noreply.github.com>
@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