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

[REVIEW] Unpin dask & distributed for development #11058

Merged
merged 22 commits into from
Jun 15, 2022
Merged

[REVIEW] Unpin dask & distributed for development #11058

merged 22 commits into from
Jun 15, 2022

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Jun 6, 2022 •
edited
Loading

This PR relaxes dask & distributed pinnings for 22.08 development.

Requires: dask/dask#9169

This PR also includes pyarrow_schema_dispatch implementation for dask-cudf

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 6, 2022
@galipremsagar galipremsagar self-assigned this Jun 6, 2022
@galipremsagar galipremsagar requested a review from a team as a code owner June 6, 2022 19:13
@galipremsagar galipremsagar requested a review from a team as a code owner June 6, 2022 19:13
@github-actions github-actions bot added conda Python Affects Python cuDF API. labels Jun 6, 2022
This was referenced Jun 6, 2022
Merged
Merged
Merged
Merged
Merged
@galipremsagar galipremsagar added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jun 7, 2022
@galipremsagar
Copy link
Contributor Author

This PR now requires an upstream dask change to be merged: dask/dask#9169

charlesbluca
charlesbluca approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@charlesbluca charlesbluca left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could we add something to the PR title / body referencing the addition of the arrow schema dispatch? Just to make it easier to track this change in the future

@galipremsagar
Copy link
Contributor Author

Could we add something to the PR title / body referencing the addition of the arrow schema dispatch? Just to make it easier to track this change in the future

Done 👍 Updated the body.

@galipremsagar galipremsagar removed the 3 - Ready for Review Ready for review by team label Jun 7, 2022
@galipremsagar galipremsagar mentioned this pull request Jun 7, 2022
Merged
3 tasks
@galipremsagar
Copy link
Contributor Author

rerun tests

@galipremsagar galipremsagar removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jun 7, 2022
@jakirkham
Add C/C++ compilers to tests
9c778d0 
Should ensure `CONDA_BUILD_SYSROOT` is set.
@galipremsagar galipremsagar mentioned this pull request Jun 13, 2022
Merged
galipremsagar
galipremsagar commented Jun 13, 2022
View reviewed changes
ci/cpu/build.sh Outdated Show resolved Hide resolved
@galipremsagar
Copy link
Contributor Author

rerun tests

1 similar comment
@galipremsagar
Copy link
Contributor Author

rerun tests

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Jun 15, 2022
jakirkham
jakirkham reviewed Jun 15, 2022
View reviewed changes
python/dask_cudf/dask_cudf/backends.py
Comment on lines -300 to +306
from dask.dataframe.dispatch import grouper_dispatch
from dask.dataframe.dispatch import pyarrow_schema_dispatch
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to bump the minimum Dask version because of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a pressing issue to bump min requirement, since it is in try/except block the current code should just work for old and new version of dask.

jakirkham
jakirkham reviewed Jun 15, 2022
View reviewed changes
python/dask_cudf/dask_cudf/tests/test_distributed.py
import numba.cuda
import pytest

import dask
from dask import dataframe as dd
from dask.distributed import Client
from distributed.utils_test import loop # noqa: F401
from distributed.utils_test import cleanup, loop # noqa: F401
Copy link
Member

Choose a reason for hiding this comment

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

Is cleanup used below somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had to import due to a CI failure, I think the reason is similar to: https://github.com/rapidsai/dask-cuda/pull/924/files

Copy link
Contributor Author

@galipremsagar galipremsagar Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just tested removing cleanup locally and got errors around Cluster failing to start, so will probably need it to be imported explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking 🙏 Strange. Maybe we should raise a Distributed issue about this?

@rapids-bot rapids-bot bot merged commit 885fd08 into rapidsai:branch-22.08 Jun 15, 2022
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Jun 15, 2022
@galipremsagar
Unpin dask & distributed for development (#2342)
92f6ba4 
Changes to be in line with: rapidsai/cudf#11058

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

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #2342
@jakirkham
Copy link
Member

Thanks Prem! 😄

rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Jun 15, 2022
@galipremsagar
Unpin dask & distributed for development (#704)
ec54774 
Changes to be in line with: rapidsai/cudf#11058

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

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #704
rapids-bot bot pushed a commit to rapidsai/dask-cuda that referenced this pull request Jun 16, 2022
@galipremsagar
Unpin dask & distributed for development (#927)
c89f0db 
Changes to be in line with: rapidsai/cudf#11058

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

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Peter Andreas Entschev (https://github.com/pentschev)

URL: #927
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Jun 23, 2022
@galipremsagar
Unpin dask & distributed for development (#4771)
0810260 
Changes to be in line with: rapidsai/cudf#11058

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

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #4771
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
@galipremsagar
Unpin dask & distributed for development (rapidsai#4771)
6630909 
Changes to be in line with: rapidsai/cudf#11058

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

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Dante Gama Dessavre (https://github.com/dantegd)

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

@jakirkham jakirkham jakirkham approved these changes

@charlesbluca charlesbluca charlesbluca approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

Assignees

@galipremsagar galipremsagar

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge 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
@galipremsagar @jakirkham @ajschmidt8 @charlesbluca