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

Fix Booster read/write locale dependency #2891

Merged
merged 3 commits into from
Mar 17, 2020

Conversation

AlbertoEAF
Copy link
Contributor

@AlbertoEAF AlbertoEAF commented Mar 8, 2020

This request fixes the bug mentioned on the issue #2890.

In C++, the process is by default is initialized to use the minimal "C" locale, however, in Java, when using the SWIG wrappers, this default is broken and the current machine locale is used instead.

Since during model read/write we rely on many C/C++ functions, some of which are 100% dependant on the locale, such as std::stod(), overriding the locale to a sane default is required.
Instead of setting the locale globally across the full process, as we might want to still read/write datasets according to the locale, a class to manage the locale context LocaleContext was created.
We only activate/deactivate that context (to force the "C" locale) during model reads/writes, to ensure our model format is independent of any locale changes in the machine or calling processes.

Motivation for this:

To avoid such issues, the fix should be done on the core library, so no bindings can be affected by locale issues, whatever happens, and this way the model format is always consistent.

@AlbertoEAF
Copy link
Contributor Author

Hello @guolinke and @chivee,

can you tell me if this looks good to you and if so, to approve/revise?

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.

LGTM

@guolinke
Copy link
Collaborator

@StrikerRUS any style issues?

@StrikerRUS StrikerRUS self-requested a review March 16, 2020 12:05
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.

@AlbertoEAF Please fix the following cpplint errors:

include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
include/LightGBM/utils/LocaleContext.h Outdated Show resolved Hide resolved
@AlbertoEAF
Copy link
Contributor Author

Changed everything you requested @StrikerRUS :)

Anything else?

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.

@AlbertoEAF Thank you very much for fixing all linting issues!

@StrikerRUS StrikerRUS merged commit 6b68967 into microsoft:master Mar 17, 2020
@AlbertoEAF
Copy link
Contributor Author

Thanks for merging @StrikerRUS :)

@AlbertoEAF AlbertoEAF deleted the fix/locale-independent-model branch March 18, 2020 10:48
henry0312 added a commit to henry0312/LightGBM that referenced this pull request Apr 7, 2020
guolinke pushed a commit that referenced this pull request Apr 12, 2020
This reverts commit 6b68967 (#2891) and fixes #2979.
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants