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

Execution device interoperability documentation #5130

Merged
merged 13 commits into from
Feb 5, 2023

Conversation

viclafargue
Copy link
Contributor

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@viclafargue viclafargue requested a review from dantegd January 17, 2023 09:40
@viclafargue viclafargue added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 18, 2023
@dantegd dantegd requested a review from wphicks January 19, 2023 16:49
@wphicks wphicks requested a review from beckernick January 19, 2023 20:36
@wphicks
Copy link
Contributor

wphicks commented Jan 19, 2023

Looks good! Nice and concise. Had a few suggestions for the text, but the code looks good. I'd also be interested in any feedback from @beckernick if you have a moment to glance at this?

@viclafargue viclafargue requested a review from a team as a code owner January 20, 2023 17:26
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Jan 20, 2023
@beckernick
Copy link
Member

This looks great overall. Left two small comments on the notebook.

I think the small README update is the right choice until we add support for a few more operators and graduate device selection out of experimental. At that point, we can do a broader update.

I think we should update the documentation with a device selection support matrix and update operator docstrings to indicate whether the operator supports device selection. I will separate issues for those and can take them on.

@beckernick
Copy link
Member

beckernick commented Jan 27, 2023

The implication of this notebook is that it's possible (for all models) to:

  1. Train an estimator on the GPU and save it
  2. Reconstruct and user the estimator on a CPU-only system

Does this work for all models or only GLMs at the moment? I had thought the latter, and if so we may want to tweak the framing here to focus on the non-deployment scenario.

@viclafargue
Copy link
Contributor Author

You are right. Some of the models implement pickling and CPU/GPU interoperability but lack the CPU-to-GPU feature at the moment (i.e.: UMAP and HDBSCAN). Most models with serialization to disk + CPU/GPU interop should allow the deployment scenario though. Sure, I will modify the notebook so that it focuses a bit more on the non-deployment scenario and will also specify in the markdown that some of the models do not implement the CPU-to-GPU feature.

@codecov-commenter
Copy link

Codecov Report

Base: 67.11% // Head: 67.20% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (f3d4900) compared to base (85b33df).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-23.02    #5130      +/-   ##
================================================
+ Coverage         67.11%   67.20%   +0.08%     
================================================
  Files               192      192              
  Lines             12396    12372      -24     
================================================
- Hits               8320     8315       -5     
+ Misses             4076     4057      -19     
Impacted Files Coverage Δ
python/cuml/common/device_selection.py 93.33% <50.00%> (-6.67%) ⬇️
python/cuml/dask/common/input_utils.py 34.37% <0.00%> (-1.51%) ⬇️
python/cuml/dask/ensemble/randomforestregressor.py 33.87% <0.00%> (-1.05%) ⬇️
...ython/cuml/dask/ensemble/randomforestclassifier.py 29.88% <0.00%> (-0.80%) ⬇️
python/cuml/preprocessing/TargetEncoder.py 85.22% <0.00%> (-0.08%) ⬇️
...on/cuml/benchmark/automated/bench_random_forest.py 0.00% <0.00%> (ø)
python/cuml/dask/preprocessing/label.py 40.00% <0.00%> (+0.71%) ⬆️
...ython/cuml/dask/neighbors/kneighbors_classifier.py 23.42% <0.00%> (+1.57%) ⬆️
python/cuml/dask/neighbors/nearest_neighbors.py 28.75% <0.00%> (+2.00%) ⬆️
python/cuml/dask/neighbors/kneighbors_regressor.py 32.75% <0.00%> (+2.11%) ⬆️

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.

Comment on lines 26 to 27
def get_current_device_type():
return GlobalSettings().device_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the reason for naming this function get_current_device_type() as opposed to get_global_device_type(), because the device type could be temporarily changed? If not, I would probably recommend to use a symmetric naming scheme here. Tagging @wphicks since I think you proposed the name in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing this. Just corrected it.

@dantegd
Copy link
Member

dantegd commented Feb 5, 2023

/merge

@rapids-bot rapids-bot bot merged commit a9de05d into rapidsai:branch-23.02 Feb 5, 2023
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants