Skip to content

[REVIEW] Unpin dask and distributed for development#3036

Merged
rapids-bot[bot] merged 7 commits intorapidsai:branch-23.02from
galipremsagar:unpin_dask
Dec 14, 2022
Merged

[REVIEW] Unpin dask and distributed for development#3036
rapids-bot[bot] merged 7 commits intorapidsai:branch-23.02from
galipremsagar:unpin_dask

Conversation

@galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Dec 5, 2022

This PR unpins dask and distributed to 2022.12.0+ for 23.02 development.

xref: rapidsai/cudf#12302

@galipremsagar galipremsagar requested review from a team as code owners December 5, 2022 08:23
@galipremsagar galipremsagar added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change DO NOT MERGE Hold off on merging; see PR for details labels Dec 5, 2022
@BradReesWork BradReesWork added this to the 23.02 milestone Dec 7, 2022
@galipremsagar galipremsagar removed the DO NOT MERGE Hold off on merging; see PR for details label Dec 9, 2022
@jakirkham
Copy link
Member

jakirkham commented Dec 12, 2022

rerun tests

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Dec 13, 2022

Blocked by: rapidsai/raft#1068

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2022 •
edited
Loading

Codecov Report

Base: 58.86% // Head: 58.95% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (fa27aea) compared to base (2ae4b61).
Patch coverage: 88.88% of modified lines in pull request are covered.

❗ Current head fa27aea differs from pull request most recent head 84b20ad. Consider uploading reports for the commit 84b20ad to get more accurate results

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-23.02    #3036      +/-   ##
================================================
+ Coverage         58.86%   58.95%   +0.08%     
================================================
  Files               131      131              
  Lines              7848     7857       +9     
================================================
+ Hits               4620     4632      +12     
+ Misses             3228     3225       -3
Impacted Files Coverage Δ
...ugraph/cugraph/dask/structure/mg_property_graph.py 12.14% <33.33%> (+0.05%) ⬆️
python/cugraph/cugraph/structure/property_graph.py 94.95% <100.00%> (+0.02%) ⬆️
python/cugraph/cugraph/utilities/utils.py 71.71% <100.00%> (+1.34%) ⬆️

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.

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Dec 14, 2022

rerun tests

galipremsagar
galipremsagar commented Dec 14, 2022
View reviewed changes
python/cugraph/cugraph/tests/test_property_graph.py
df = pG.get_vertex_data(types=[key])
assert len(df) == stop - start + 1
assert (df["_VERTEX_"] == list(range(start, stop + 1))).all()
assert (
Copy link
Contributor Author

@galipremsagar galipremsagar Dec 14, 2022

Choose a reason for hiding this comment

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

These pytest fixes were needed because cudf removed the comparison of host & device objects in : rapidsai/cudf#12281, which will return False if performed.

bdice
bdice reviewed Dec 14, 2022
View reviewed changes
rlratzel
rlratzel approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

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

LGTM assuming the issue @bdice brought up is addressed.

@galipremsagar @bdice
Apply suggestions from code review
84b20ad 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@rlratzel
Copy link
Contributor

rlratzel commented Dec 14, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b5becc5 into rapidsai:branch-23.02 Dec 14, 2022
@rlratzel rlratzel mentioned this pull request Dec 14, 2022
Closed
@lowener lowener mentioned this pull request Dec 15, 2022
Closed
2 tasks
@rlratzel rlratzel mentioned this pull request Dec 15, 2022
Merged
@eriknw eriknw mentioned this pull request Dec 20, 2022
Merged
rapids-bot bot pushed a commit that referenced this pull request Jan 12, 2023
@eriknw
Fix tests for MG property graph (#3090)
5c240dd 
Small PR to fix MGPG tests.

Note that @galipremsagar fixed the equivalent tests for `test_property_graph.py` in #3036 by using `s._constructor`, which is pretty slick.

Also, `start_dask_client` returns a tuple of client and cluster now, so I updated the `dask_client` fixture so the cluster can be closed properly.

Authors:
  - Erik Welch (https://github.com/eriknw)
  - Alex Barghi (https://github.com/alexbarghi-nv)

Approvers:
  - Alex Barghi (https://github.com/alexbarghi-nv)
  - Rick Ratzel (https://github.com/rlratzel)

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

Reviewers

@bdice bdice bdice left review comments

@rlratzel rlratzel rlratzel approved these changes

+1 more reviewer

@raydouglass raydouglass raydouglass approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

improvement Improvement / enhancement to an existing function non-breaking Non-breaking change

Projects

None yet

Milestone

23.02

Development

Successfully merging this pull request may close these issues.

7 participants

@galipremsagar @jakirkham @codecov-commenter @rlratzel @raydouglass @bdice @BradReesWork