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

Fixed Predictor lifecycle and trees initialization in Contrib mode #6778

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AndreyOrb
Copy link

  1. Fixed Predictor lifecycle
  2. Fixed Boosting trees initialization

#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
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
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
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
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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants