Skip to content

Conversation

@charlesbluca
Copy link
Collaborator

@charlesbluca charlesbluca commented Feb 8, 2022

This PR bumps the dask-sphinx-theme to be more in line with Dask / Distributed's docs, and adds the sphinx-tabs extension so that code-blocks can be tabbed to show their GPU equivalent (when possible)

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2022

Codecov Report

Merging #394 (9d804fb) into main (c3ad6a9) will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
+ Coverage   89.02%   89.17%   +0.15%     
==========================================
  Files          69       69              
  Lines        3326     3326              
  Branches      653      653              
==========================================
+ Hits         2961     2966       +5     
+ Misses        296      287       -9     
- Partials       69       73       +4     
Impacted Files Coverage Δ
dask_sql/_version.py 34.00% <0.00%> (+1.44%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@charlesbluca
Copy link
Collaborator Author

@VibhuJawa if you get a chance could you check out machine_learning.rst and let me know if there's any other blocks in there that can be run on GPU? Based my examples off the GPU model tests but not sure if I missed anything 🙂

@VibhuJawa
Copy link
Collaborator

@VibhuJawa if you get a chance could you check out machine_learning.rst and let me know if there's any other blocks in there that can be run on GPU? Based my examples off the GPU model tests but not sure if I missed anything 🙂

Looking at it right now. We need to add to do two things:

  1. Add explanation around wrap_fit and wrap_predict .
  2. Use the same cuML and sklearn model to avoid confusion

Will make a PR with the suggested changes to this PR in a bit.

@VibhuJawa
Copy link
Collaborator

VibhuJawa commented Feb 8, 2022

@charlesbluca , Raised a PR herre charlesbluca#2 .

Also, looking at the other sections (see below) of the machine-learning docs, they should probably work with GPU's . We don't have have tests for those now though. I think it might be worth-while adding tests for them and adding the GPU sections for those too. (Obviously in a follow up PR )

  1. section 5
  2. section-6
  3. example

What do you think ?

@VibhuJawa
Copy link
Collaborator

Raised #397 to track the update to other ML sections.

@charlesbluca
Copy link
Collaborator Author

That sounds good to me - I can make TODOs for those examples so that we can remember to add GPU sections there in a follow up.

It's not immediately obvious what code changes would be made to example 6 to account for GPUs - or is the idea that a model created with a cuML class should have no issues being exported?

@VibhuJawa
Copy link
Collaborator

VibhuJawa commented Feb 9, 2022

It's not immediately obvious what code changes would be made to example 6 to account for GPUs - or is the idea that a model created with a cuML class should have no issues being exported?

Depends on the model, i think most models work but some models like FIL have issues . We would have to test it to know. Anyways, having this under CI will give us confidence after putting stuff under CI.

That sounds good to me - I can make TODOs for those examples so that we can remember to add GPU sections there in a follow up.

Yup. Raised issue to track this, #397 . Lets add stuff there. :-)

@charlesbluca charlesbluca marked this pull request as ready for review March 15, 2022 19:41
@charlesbluca
Copy link
Collaborator Author

Going to go ahead and merge this in, as the gpuCI failures are unrelated

@charlesbluca charlesbluca merged commit 157b3f4 into dask-contrib:main Mar 22, 2022
@charlesbluca charlesbluca deleted the update-docs branch July 19, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants