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

[c++] Fixed Predictor lifecycle and trees initialization in Contrib mode #6778

Merged
merged 5 commits into from
Jan 20, 2025

Conversation

AndreyOrb
Copy link
Contributor

@AndreyOrb AndreyOrb commented Jan 8, 2025

  1. Fixed Predictor lifecycle
  2. Fixed Boosting trees initialization

Fixed #5482

2) Fixed Boosting trees initialization

microsoft#5482
Copy link
Collaborator

@jameslamb jameslamb 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 this! Will defer to other reviewers with more C++ experience but one question... is it possible to test that correctness of this by adding test cases in https://github.com/microsoft/LightGBM/tree/master/tests/cpp_tests?

There are tests on prediction there, but I specifically mean testing that this previously-not-thread-safe behavior remains thread-safe.

That'd reduce the risk of issues be re-introduced in future refactorings.

@AndreyOrb
Copy link
Contributor Author

@jameslamb, I've tried to. Unfortunately, the tests fail (not my tests, I reverted my changes ensure that it's not my code).
We can try again after the baseline tests are fixed.
image

Where should I add the test? In test_single_row.cpp ?

@jameslamb
Copy link
Collaborator

Ah interesting. Looking at that error, I suspect the issue is the working directory you're running from. That test has hard-coded relative paths from the root of the repo, so the testlightgbm binary only works if invoked from the root of the repo.

Here's how we run the tests in continuous integration:

LightGBM/.ci/test.sh

Lines 61 to 63 in e0c34e7

cmake -B build -S . "${cmake_args[@]}"
cmake --build build --target testlightgbm -j4 || exit 1
./testlightgbm || exit 1

Where should I add the test? In test_single_row.cpp ?

Yep that seems like a good place, thanks!

@AndreyOrb
Copy link
Contributor Author

You are right. Changing the working directory has helped.
image

Baseline tests have passed:
image

I have updated the tests to check Contrib prediction type.
Here I can see that several threads enter the "danger" code area:
image

The new test before the fix manages to crash the test:
image
image

I used 100 iterations to ensure I hit the raise condition:
image

But then, the test is becoming slow:
image

For production, I reduced the number to 5:
image

@AndreyOrb AndreyOrb requested a review from jameslamb January 8, 2025 21:11
@jameslamb
Copy link
Collaborator

jameslamb commented Jan 10, 2025

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/12703948521

Status: success ✔️.

@AndreyOrb
Copy link
Contributor Author

Is there anything else I should do to push the PR?

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

I have no other comments. Hoping @shiyu1994 or @guolinke can review this when they have time.

@AndreyOrb
Copy link
Contributor Author

@shiyu1994, @guolinke Could you review the PR?
I'm afraid I will not be able to fix the PR later, if anything pops up.

Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

Thank you!

@StrikerRUS StrikerRUS changed the title Fixed Predictor lifecycle and trees initialization in Contrib mode [c++] Fixed Predictor lifecycle and trees initialization in Contrib mode Jan 19, 2025
Copy link
Collaborator

@StrikerRUS StrikerRUS 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 a lot for your PR!

@StrikerRUS StrikerRUS merged commit 3654eca into microsoft:master Jan 20, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is predict(..., pred_contrib=True) thread safe?
4 participants