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] FIL: Add optimization parameter blocks_per_sm for all but the tiniest models #3032

Merged
merged 18 commits into from
Nov 20, 2020
Merged

[REVIEW] FIL: Add optimization parameter blocks_per_sm for all but the tiniest models #3032

merged 18 commits into from
Nov 20, 2020

Conversation

levsnv
Copy link
Contributor

@levsnv levsnv commented Oct 21, 2020

No description provided.

@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@codecov-io
Copy link

codecov-io commented Oct 21, 2020 •
edited
Loading

Codecov Report

Merging #3032 (e4dc999) into branch-0.17 (b7bfb7e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #3032      +/-   ##
===============================================
+ Coverage        70.67%   70.68%   +0.01%     
===============================================
  Files              197      197              
  Lines            15573    15576       +3     
===============================================
+ Hits             11006    11010       +4     
+ Misses            4567     4566       -1
Impacted Files Coverage Δ
python/cuml/fil/fil.pyx 93.51% <100.00%> (+0.10%) ⬆️
...l/_thirdparty/sklearn/preprocessing/_imputation.py 62.50% <0.00%> (+0.40%) ⬆️

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 b7bfb7e...e4dc999. Read the comment docs.

levsnv
levsnv commented Oct 23, 2020
View reviewed changes
cpp/include/cuml/fil/fil.h Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
@levsnv levsnv changed the title [WIP] just control block count [REVIEW] just control block count Oct 23, 2020
@levsnv levsnv changed the title [REVIEW] just control block count [REVIEW] FIL: Add optimization parameter blocks_per_sm Oct 23, 2020
@levsnv levsnv changed the title [REVIEW] FIL: Add optimization parameter blocks_per_sm [REVIEW] FIL: Add optimization parameter blocks_per_sm for all but tinyest models Oct 23, 2020
@levsnv levsnv marked this pull request as ready for review October 23, 2020 05:47
@levsnv levsnv requested a review from a team as a code owner October 23, 2020 05:47
@levsnv levsnv changed the title [REVIEW] FIL: Add optimization parameter blocks_per_sm for all but tinyest models [REVIEW] FIL: Add optimization parameter blocks_per_sm for all but tiniest models Oct 23, 2020
@levsnv levsnv changed the title [REVIEW] FIL: Add optimization parameter blocks_per_sm for all but tiniest models [REVIEW] FIL: Add optimization parameter blocks_per_sm for all but the tiniest models Oct 23, 2020
levsnv
levsnv commented Oct 23, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
canonizer
canonizer suggested changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@canonizer canonizer left a comment

Choose a reason for hiding this comment

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

The code looks good, just minor technical comments.

However, some C++ tests for the new parameter are needed.

cpp/include/cuml/fil/fil.h Outdated Show resolved Hide resolved
cpp/include/cuml/fil/fil.h Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cpp/include/cuml/fil/fil.h Outdated Show resolved Hide resolved
cpp/src/fil/fil.cu Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
@levsnv levsnv requested a review from canonizer October 27, 2020 03:27
@levsnv levsnv requested a review from a team as a code owner October 27, 2020 03:28
canonizer
canonizer suggested changes Oct 27, 2020
View reviewed changes
cpp/include/cuml/fil/fil.h Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/test/sg/fil_test.cu Outdated Show resolved Hide resolved
cpp/test/sg/fil_test.cu Outdated Show resolved Hide resolved
python/cuml/fil/fil.pyx Show resolved Hide resolved
python/cuml/fil/fil.pyx Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
@levsnv
Copy link
Contributor Author

levsnv commented Nov 3, 2020

now depends on #3088

@levsnv levsnv requested a review from canonizer November 14, 2020 03:27
@levsnv levsnv added 3 - Ready for Review Ready for review by team CUDA / C++ CUDA issue labels Nov 14, 2020
@levsnv levsnv added the Cython / Python Cython or Python issue label Nov 14, 2020
canonizer
canonizer approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@canonizer canonizer left a comment

Choose a reason for hiding this comment

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

Approved, provided that the review comments are addressed.

cpp/src/fil/fil.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
cpp/test/sg/fil_test.cu Outdated Show resolved Hide resolved
Salonijain27
Salonijain27 suggested changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

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

It would be nice to have a python test in the PR, as we are updating the cython file here.

@canonizer
Copy link
Contributor

@Salonijain27 However, there's no new Python functionality in this PR (i.e. no external API changes), so I think existing tests should be sufficient.

Salonijain27
Salonijain27 approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@Salonijain27 Salonijain27 left a comment

Choose a reason for hiding this comment

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

Lgtm, just added a suggestion

python/cuml/fil/fil.pyx Show resolved Hide resolved
@levsnv
Copy link
Contributor Author

levsnv commented Nov 19, 2020

rerun tests: random noise in cuml/test/test_random_forest.py:264

@levsnv
Copy link
Contributor Author

levsnv commented Nov 19, 2020

rerun tests: ChecksumMismatchError: Conda detected a mismatch between the expected content and downloaded content

@JohnZed JohnZed merged commit 2a84c52 into rapidsai:branch-0.17 Nov 20, 2020
@levsnv levsnv deleted the just-control-block-count branch April 18, 2021 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Salonijain27 Salonijain27 Salonijain27 approved these changes

@canonizer canonizer canonizer approved these changes

@JohnZed JohnZed JohnZed approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team CUDA / C++ CUDA issue Cython / Python Cython or Python issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@levsnv @GPUtester @codecov-io @canonizer @JohnZed @Salonijain27