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

[FEA] Support multiple classes in multi-node-multi-gpu logistic regression, from C++, Cython, to Dask Python class #5565

Merged
merged 13 commits into from
Sep 29, 2023
Merged

[FEA] Support multiple classes in multi-node-multi-gpu logistic regression, from C++, Cython, to Dask Python class #5565

merged 13 commits into from
Sep 29, 2023

Conversation

lijinf2
Copy link
Contributor

@lijinf2 lijinf2 commented Aug 29, 2023 •
edited
Loading

Github issue: #5501

This PR depends on and has included PR 5558.

@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 29, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added Cython / Python Cython or Python issue CUDA/C++ labels Aug 29, 2023
@lijinf2 lijinf2 added improvement Improvement / enhancement to an existing function bug Something isn't working breaking Breaking change and removed bug Something isn't working labels Aug 29, 2023
@lijinf2 lijinf2 force-pushed the lr_mg_multiclass branch 2 times, most recently from 76e4abf to ef3c55f Compare September 20, 2023 20:20
@lijinf2 lijinf2 marked this pull request as ready for review September 20, 2023 20:21
@lijinf2 lijinf2 requested review from a team as code owners September 20, 2023 20:21
@lijinf2 lijinf2 added breaking Breaking change and removed breaking Breaking change labels Sep 20, 2023
@lijinf2 lijinf2 mentioned this pull request Sep 20, 2023
Merged
@lijinf2 lijinf2 force-pushed the lr_mg_multiclass branch 4 times, most recently from c9af0a3 to e877100 Compare September 22, 2023 17:22
@dantegd dantegd added the 3 - Ready for Review Ready for review by team label Sep 25, 2023
@csadorf
Copy link
Contributor

csadorf commented Sep 25, 2023

/ok to test

@csadorf csadorf self-assigned this Sep 26, 2023
csadorf
csadorf requested changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just a few minor change requests and suggestions.

I'm not sure I agree with the assessment that this is a breaking change since all changes are additive. Why did you classify it as such?

python/cuml/tests/dask/test_dask_logistic_regression.py Show resolved Hide resolved
python/cuml/tests/dask/test_dask_logistic_regression.py Outdated Show resolved Hide resolved
cpp/include/cuml/linear_model/qn_mg.hpp Outdated Show resolved Hide resolved
cpp/src/glm/qn_mg.cu Outdated Show resolved Hide resolved
python/cuml/tests/dask/test_dask_logistic_regression.py Show resolved Hide resolved
@lijinf2 lijinf2 force-pushed the lr_mg_multiclass branch 2 times, most recently from 6e8d50f to b6fe8b2 Compare September 27, 2023 05:59
@lijinf2 lijinf2 mentioned this pull request Sep 27, 2023
Open
@lijinf2 lijinf2 removed the breaking Breaking change label Sep 27, 2023
@lijinf2
Copy link
Contributor Author

lijinf2 commented Sep 27, 2023

Looks good to me! Just a few minor change requests and suggestions.

I'm not sure I agree with the assessment that this is a breaking change since all changes are additive. Why did you classify it as such?

Thanks so much for helping review the PR! The revised PR should have included the requests and suggestions. If not, let me know!

The "breaking" label is required by the CI label checker. Not fully understand why. Maybe because the PR deleted some existing codes I think.

@lijinf2 lijinf2 added the breaking Breaking change label Sep 27, 2023
@lijinf2
Copy link
Contributor Author

lijinf2 commented Sep 27, 2023

@csadorf The failed CI check hits a TimeoutError. The same error happened before in merged PRs. Can you help me rerun the failed CI check to see if the error still exists? Thanks!

@csadorf
Copy link
Contributor

csadorf commented Sep 27, 2023

The "breaking" label is required by the CI label checker. Not fully understand why. Maybe because the PR deleted some existing codes I think.

The label checker asks to provide either the breaking or the non-breaking label to indicate whether the change set constitutes a breaking or non-breaking change to the user interface.

@csadorf csadorf added non-breaking Non-breaking change and removed breaking Breaking change labels Sep 27, 2023
@lijinf2
Copy link
Contributor Author

lijinf2 commented Sep 28, 2023

The "breaking" label is required by the CI label checker. Not fully understand why. Maybe because the PR deleted some existing codes I think.

The label checker asks to provide either the breaking or the non-breaking label to indicate whether the change set constitutes a breaking or non-breaking change to the user interface.

Thanks for the tip and replacement of the label! The PR has been revised according to the new comments.

lijinf2 and others added 13 commits September 27, 2023 21:19
csadorf
csadorf approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments!

@csadorf
Copy link
Contributor

csadorf commented Sep 29, 2023

/merge

@rapids-bot rapids-bot bot merged commit 3c4ceb9 into rapidsai:branch-23.10 Sep 29, 2023
50 checks passed
@lijinf2 lijinf2 mentioned this pull request Sep 29, 2023
Closed
rapids-bot bot pushed a commit that referenced this pull request Oct 4, 2023
@lijinf2
[FEA] Support L1 regularization and ElasticNet in MNMG Dask LogisticR…
39dfc7e 
…egression (#5587)

This PR depends on [PR5565](#5565).

Authors:
  - Jinfeng Li (https://github.com/lijinf2)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Simon Adorf (https://github.com/csadorf)

URL: #5587
@lijinf2 lijinf2 deleted the lr_mg_multiclass branch June 26, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csadorf csadorf csadorf approved these changes

@dantegd dantegd dantegd approved these changes

Assignees

@csadorf csadorf

Labels
3 - Ready for Review Ready for review by team CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lijinf2 @csadorf @dantegd