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

[JVM-Packages] Remove synchronized in java predict function. #7027

Closed
wants to merge 1 commit into from

Conversation

trivialfis
Copy link
Member

We have made the predict function thread safe in 1.4.

Follow up on #6021 . Related #5951 .

We have made the predict function thread safe in 1.4.
@trivialfis
Copy link
Member Author

Hmm, it's not the predict function not being thread-safe, but rather the jvm iterator.

@Craigacp
Copy link
Contributor

Craigacp commented Jun 9, 2021

I think removing the synchronized keyword should be sufficient (at least for our use case in Tribuo). What do you mean by the JVM iterator isn't thread safe?

@trivialfis
Copy link
Member Author

It looks like XGBoost is failing to construct DMatrix from java iterator for some reasons.

@Craigacp
Copy link
Contributor

Craigacp commented Jun 10, 2021

If you can point me at a test or some example code I can run that down.

Edit: Oh I see it now. I don't think I could see the other CI runs before, but the link seems to work now. So it's failing in the middle of Spark when trying to make a DMatrix?

I'm not sure how this change would affect that.

@trivialfis
Copy link
Member Author

I will take a deeper dive into jvm prediction after #6819 is done.

@gangpeng
Copy link

gangpeng commented Nov 6, 2021

@trivialfis
Hi,
We would like to remove the synchronized keyword from predict method from xgboost4j for only project. We only construct DMatrix from float[] in production, the code path looks thread-safe to me. Is the problem you mentioned only related to iterable?
Thanks!

@trivialfis trivialfis closed this May 23, 2024
@trivialfis trivialfis deleted the jvm-predict-remove-sync branch May 23, 2024 04:11
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