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

Update API reference and examples in docs #561

Merged
merged 23 commits into from
Apr 12, 2021

Conversation

charlesbluca
Copy link
Member

Addresses #554; I tried to tidy up the installation/specializations/quickstart pages so that some of the more technical stuff could go into some published examples, and probably a new page discussing configuration of LocalCUDACluster / dask-cuda-worker.

Also added sphinx-click to the dependencies so we can more easily document any changes to dask-cuda-worker (#560) - I would also like to get the docstrings for the CLI, LocalCUDACluster, and initiailize() up to date and more concise if possible.

@charlesbluca charlesbluca requested a review from a team as a code owner April 6, 2021 00:42
@charlesbluca charlesbluca marked this pull request as draft April 6, 2021 00:43
@github-actions github-actions bot added doc Documentation python python code needed labels Apr 6, 2021
@charlesbluca charlesbluca added 2 - In Progress Currently a work in progress non-breaking Non-breaking change labels Apr 6, 2021
@codecov-io
Copy link

codecov-io commented Apr 6, 2021

Codecov Report

Merging #561 (ba92e58) into branch-0.20 (cb9bf35) will increase coverage by 1.00%.
The diff coverage is 52.50%.

❗ Current head ba92e58 differs from pull request most recent head 65e47f7. Consider uploading reports for the commit 65e47f7 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.20     #561      +/-   ##
===============================================
+ Coverage        61.06%   62.07%   +1.00%     
===============================================
  Files               22       22              
  Lines             2571     2634      +63     
===============================================
+ Hits              1570     1635      +65     
+ Misses            1001      999       -2     
Impacted Files Coverage Δ
dask_cuda/cuda_worker.py 78.75% <ø> (ø)
dask_cuda/initialize.py 92.59% <ø> (+3.70%) ⬆️
dask_cuda/device_host_file.py 69.11% <30.61%> (-19.52%) ⬇️
dask_cuda/local_cuda_cluster.py 79.16% <66.66%> (-2.23%) ⬇️
dask_cuda/cli/dask_cuda_worker.py 96.87% <100.00%> (+1.42%) ⬆️
dask_cuda/explicit_comms/comms.py 98.87% <100.00%> (+0.09%) ⬆️
dask_cuda/explicit_comms/dataframe/shuffle.py 97.94% <0.00%> (+0.68%) ⬆️
dask_cuda/_version.py 44.80% <0.00%> (+0.71%) ⬆️
dask_cuda/utils.py 90.90% <0.00%> (+1.06%) ⬆️
dask_cuda/proxy_object.py 91.36% <0.00%> (+1.11%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1526017...65e47f7. Read the comment docs.

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.

Thanks @charlesbluca for working on this, I added some comments.

docs/source/quickstart.rst Outdated Show resolved Hide resolved
docs/source/quickstart.rst Outdated Show resolved Hide resolved
dask_cuda/cli/dask_cuda_worker.py Outdated Show resolved Hide resolved
dask_cuda/cli/dask_cuda_worker.py Show resolved Hide resolved
docs/source/install.rst Outdated Show resolved Hide resolved
docs/source/install.rst Outdated Show resolved Hide resolved
dask_cuda/cli/dask_cuda_worker.py Outdated Show resolved Hide resolved
Copy link
Member Author

@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 few more questions:

dask_cuda/local_cuda_cluster.py Outdated Show resolved Hide resolved
dask_cuda/local_cuda_cluster.py Outdated Show resolved Hide resolved
@jakirkham jakirkham changed the base branch from branch-0.19 to branch-0.20 April 8, 2021 21:55
rapids-bot bot pushed a commit that referenced this pull request Apr 9, 2021
Based on some discussions in #561, this PR removes some kwargs from `LocalCUDACluster` that may not be needed:

- `processes` doesn't have any impact on its construction (other than throwing a `ValueError` if it's set false); this should be safe to remove altogether
- `data` is pretty complex and it isn't commonly specified, so it can be removed from the named arguments and accessed through the `kwargs` object if it is specified

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

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

URL: #562
docs/source/install.rst Outdated Show resolved Hide resolved
@charlesbluca
Copy link
Member Author

I think this PR is at a good closing point - the API reference is updated, the installation + quickstart is fleshed out a bit more, and there are clear minimal examples of using LocalCUDACluster and dask-cuda-worker. Some follow up stuff that would be nice:

@charlesbluca charlesbluca marked this pull request as ready for review April 9, 2021 14:28
@charlesbluca charlesbluca added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Apr 12, 2021
Copy link
Member Author

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

One last question I missed:

"--dashboard-prefix",
type=str,
default=None,
help="""Prefix for the dashboard. Can be a string (like ...) or ``None`` for no
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we know what dashboard prefix input should look like?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, maybe @jacobtomlinson can help with this one.

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.

LGTM, thanks @charlesbluca !

@pentschev
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 3c6e0a9 into rapidsai:branch-0.20 Apr 12, 2021
rapids-bot bot pushed a commit that referenced this pull request Apr 19, 2021
Adds sphinx-click to the Conda environment used by RTD, which is necessary now because of #561. Fixes doc builds, which are currently failing.

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - https://github.com/jakirkham
  - Jordan Jacobelli (https://github.com/Ethyling)

URL: #578
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team doc Documentation non-breaking Non-breaking change python python code needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants