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.

Sign up for GitHub

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

Jump to bottom

Use new rapids-dask-dependency metapackage for managing dask versions #14364

Merged
merged 13 commits into from
Nov 13, 2023
Merged

Use new rapids-dask-dependency metapackage for managing dask versions #14364

merged 13 commits into from
Nov 13, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Nov 6, 2023

Description

Currently dask versions are pinned as part of every release cycle and then unpinned for the next development cycle across all of RAPIDS. This introduces a great deal of churn. To centralize the dependency, we have created a metapackage to manage the required dask version and this PR introduces that metapackage as a dependency of dask_cudf.

Checklist

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

@vyasr vyasr added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 6, 2023
@vyasr vyasr self-assigned this Nov 6, 2023
@vyasr
Copy link
Contributor Author

vyasr commented Nov 6, 2023

Since the conda tests will install dask-cuda as one of the testing dependencies and that will transitively pull in dask/distributed, I did a local CI repro where I removed dask-cuda from the list and ran the test script. The metapackage has the expected effect of installing causing the latest dask/distributed nightlies to be installed.

@vyasr vyasr marked this pull request as ready for review November 7, 2023 01:25
@vyasr vyasr requested review from a team as code owners November 7, 2023 01:25
@vyasr vyasr added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Nov 7, 2023
pentschev
pentschev reviewed Nov 7, 2023
View reviewed changes
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

Changes seem sensible to me and overall this looks good. I'll defer the ultimate approval to @galipremsagar who has been doing the pin/unpin work more often recently.

@pentschev
Copy link
Member

I was looking at the tests, for example this one, and I see:

dask                      2023.10.2a231106  py_g8c787a8c_8    dask/label/dev
dask-core                 2023.10.2a231101 py_g91098a634_4    dask/label/dev
distributed               2023.10.2a231106  py_g8c787a8c_8    dask/label/dev

Those are being installed as expected but I don't see rapids-dask-dependency being installed there. Could we be missing that dependency somewhere?

In contrast, the wheels test is installing it as expected:

Collecting rapids-dask-dependency==23.12.*,>=0.0.0a0

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2023

@pentschev that is expected, and is why I left the previous comment #14364 (comment). The lines that you're referring to come from the "Generate Python testing dependencies" step, which installs dask-cuda because it's a testing dependency for dask_cudf. As a result, when dask_cudf is installed later you see the new metapackage getting installed but not dask itself because dask is already installed into the environment.

@pentschev
Copy link
Member

Thanks @vyasr , now I see why I didn't find it before. The package name is rapids_dask_dependency and I was searching for rapids-dask-dependency, to me the latter seems more common/natural. Is there a reason we're using underscores instead of dashes?

@pentschev
Copy link
Member

Also note that the wheels package uses dashes instead of underscores.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2023

Oh this is fun. Yeah wheels have different normalization rules for names. Note that it's using hyphens even though the pyproject.toml file uses underscores. We can make it consistently hyphens, it'll just require a bit of cleanup.

@pentschev
Copy link
Member

Oh this is fun. Yeah wheels have different normalization rules for names. Note that it's using hyphens even though the pyproject.toml file uses underscores. We can make it consistently hyphens, it'll just require a bit of cleanup.

I don't think this is urgent, but probably nice-to-have later on. I'm sure I'll miss the package again in the future when grepping things.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2023

Better to change it now than to encode the package name everywhere now and have to change it later: rapidsai/rapids-dask-dependency#4. Once that's merged I'll update this PR accordingly.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2023 •
edited
Loading

Updated the package name.

@galipremsagar galipremsagar mentioned this pull request Nov 7, 2023
Merged
galipremsagar
galipremsagar reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

I guess we have to add rapids-dask-dependency to version upgrade scripts

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2023

I guess we have to add rapids-dask-dependency to version upgrade scripts

Good point.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2023

Done

@vyasr vyasr requested a review from galipremsagar November 8, 2023 16:25
@galipremsagar galipremsagar added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for Review Ready for review by team labels Nov 8, 2023
galipremsagar
galipremsagar approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

Changes LGTM, Thanks @vyasr ! I'm working on PRs for other repos, hence tagging this DONOT MERGE for now. Will undo this tagging in few hours.

This was referenced Nov 8, 2023
Merged
Merged
rapids-bot bot pushed a commit to rapidsai/dask-cuda that referenced this pull request Nov 13, 2023
@galipremsagar
Use new rapids-dask-dependency metapackage for managing dask versio…
9a5d06d 
…ns (#1270)

Currently dask versions are pinned as part of every release cycle and then unpinned for the next development cycle across all of RAPIDS. This introduces a great deal of churn. To centralize the dependency, we have created a metapackage to manage the required dask version and this PR introduces that metapackage as a dependency of dask-cuda.

xref: rapidsai/cudf#14364

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

Approvers:
  - Peter Andreas Entschev (https://github.com/pentschev)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #1270
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Nov 13, 2023
@galipremsagar
Use new rapids-dask-dependency metapackage for managing dask vers…
325a7e6 
…ions (#5649)

Currently dask versions are pinned as part of every release cycle and then unpinned for the next development cycle across all of RAPIDS. This introduces a great deal of churn. To centralize the dependency, we have created a metapackage to manage the required dask version and this PR introduces that metapackage as a dependency of cuml.



xref: rapidsai/cudf#14364

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Simon Adorf (https://github.com/csadorf)

Approvers:
  - Simon Adorf (https://github.com/csadorf)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #5649
rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Nov 13, 2023
@galipremsagar
Use new rapids-dask-dependency metapackage for managing dask versio…
e5bea87 
…ns (#1968)

Currently dask versions are pinned as part of every release cycle and then unpinned for the next development cycle across all of RAPIDS. This introduces a great deal of churn. To centralize the dependency, we have created a metapackage to manage the required dask version and this PR introduces that metapackage as a dependency of raft_dask.

xref: rapidsai/cudf#14364

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

Approvers:
  - Divye Gala (https://github.com/divyegala)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #1968
@galipremsagar galipremsagar removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Nov 13, 2023
@galipremsagar
Copy link
Contributor

/merge

@AyodeAwe AyodeAwe merged commit 4313cfa into rapidsai:branch-23.12 Nov 13, 2023
58 of 62 checks passed
@vyasr vyasr deleted the feat/rapids_dask_dependency branch November 13, 2023 19:52
raydouglass pushed a commit to rapidsai/cugraph that referenced this pull request Nov 14, 2023
@galipremsagar
Use new rapids-dask-dependency metapackage for managing dask vers…
b90e1a5 
…ions (#3991)

Currently dask versions are pinned as part of every release cycle and then unpinned for the next development cycle across all of RAPIDS. This introduces a great deal of churn. To centralize the dependency, we have created a metapackage to manage the required dask version and this PR introduces that metapackage as a dependency of cugraph.

xref: rapidsai/cudf#14364

Authors:
   - Chuck Hastings (https://github.com/ChuckHastings)
   - Rick Ratzel (https://github.com/rlratzel)
   - Vyas Ramasubramani (https://github.com/vyasr)
   - GALI PREM SAGAR (https://github.com/galipremsagar)
   - Naim ([email protected])

Approvers:
   - Jake Awe (https://github.com/AyodeAwe)
younseojava pushed a commit to ROCm/dask-cuda-rocm that referenced this pull request Apr 16, 2024
@galipremsagar @younseojava
Use new rapids-dask-dependency metapackage for managing dask versio…
9d6e5a4 
…ns (rapidsai#1270)

Currently dask versions are pinned as part of every release cycle and then unpinned for the next development cycle across all of RAPIDS. This introduces a great deal of churn. To centralize the dependency, we have created a metapackage to manage the required dask version and this PR introduces that metapackage as a dependency of dask-cuda.

xref: rapidsai/cudf#14364

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

Approvers:
  - Peter Andreas Entschev (https://github.com/pentschev)
  - Jake Awe (https://github.com/AyodeAwe)

URL: rapidsai#1270
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@galipremsagar galipremsagar galipremsagar approved these changes

@AyodeAwe AyodeAwe AyodeAwe approved these changes

@pentschev pentschev pentschev approved these changes

Assignees

@vyasr vyasr

Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vyasr @pentschev @galipremsagar @AyodeAwe