[python-package] fix libpath.py

[shiyu1994]

This is to fix a mistake in libpath.py, which causes a wrong candidate library path, as shown by the first one below:

$ python python-package/lightgbm/libpath.py
/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lightgbm/libpath.py/lib_lightgbm.so
/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lib_lightgbm.so
/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lightgbm/bin/lib_lightgbm.so
/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lightgbm/lib/lib_lightgbm.so

With this fix, it becomes

/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lightgbm/lib_lightgbm.so
/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lib_lightgbm.so
/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lightgbm/bin/lib_lightgbm.so
/home/shiyu/Test-LightGBM/cuda-obj/LightGBM/python-package/lightgbm/lib/lib_lightgbm.so

[jameslamb]

Ahhhh sorry about that! This is my fault.

But given this bug has been there since v4.0.0 and we've received 0 reports of anyone encountering an error... could we just remove that item from the list of searched paths entirely?

I think that as of the move to scikit-build-core + the build-python.sh script, there is less opportunity for lib_lightgbm.so ending up in a non-standard path.

Removing that item would speed up library-loading a bit and reduce the risk of errors caused by non-standard approaches to building lightgbm.

[shiyu1994]

Actually I got a report from @chjinche this morning about the issue. And the first correct path is used in the LightGBM-Transform. So, can we just keep this since it seems that LightGBM-Transform heavily relies on the first path.

https://github.com/microsoft/LightGBM-Transform

[shiyu1994]

If we want to prioritize the safety for the package, @chjinche may also help to change the paths in LightGBM-Transform. Which approach do you think is preferred? @jameslamb @chjinche

[jameslamb]

Actually I got a report from @chjinche this morning about the issue

Thanks for that. In the future @chjinche , I'd appreciate if you please post here on the project's public issue tracker instead of sending bug reports in private messages.

can we just keep this since it seems that LightGBM-Transform heavily relies on the first path

I'm surprised to learn that project relies "heavily" on something that appears to have been broken for several months.

Seems that they're just now picking up development after more than a year of inactivity. https://github.com/microsoft/lightgbm-transform/commits/main

I think LightGBM-Transform should be changed to adapt to what LightGBM does, and not the other way around. And that we should take this opportunity to slightly simplify library-loading for lightgbm.

[chjinche]

hi @jameslamb to clarify, we are not asking for some private message, but are debugging community PR CI failure microsoft/lightgbm-transform#26 issue and found this.

Looks it is a fault as you mentioned, and not intentional, could you please allow this fix in this time, so unblock people, we may consider improving our releasing process in the future to be more tolerant for the candidate path changes.

[jameslamb]

are debugging community PR CI failure microsoft/lightgbm-transform#26 issue and found this

Thanks for that! Linking to that context in the PR description would have prevented me from drawing an incorrect conclusion.

could you please allow this fix in this time

I really don't think we should.

It seems that all you'd need to do is to fix these lines:

https://github.com/microsoft/lightgbm-transform/blob/1d7d34265954dc872c37512699e4c25aa27f7906/.ci/setup.sh#L11

https://github.com/microsoft/lightgbm-transform/blob/1d7d34265954dc872c37512699e4c25aa27f7906/scripts/publish_python_package.sh#L10

to copy lib_lightgbm.so to one of the other supported paths.

I don't support missing this opportunity to simplify lightgbm just for LightGBM-Transform to avoid having to make such a small change to a CI script. It is LightGBM-Transform that's using a custom build process (compiling lib_lightgbm.so and then manually moving it around with cp), and that should conform to how LightGBM works.

[chjinche]

Thanks @jameslamb, but looks more than the line work. This is because transform lib works as extensive so(lib_custom_parser.so, lib_transform.so) together with lib_transform.so, and also need some basic.py modification.

Not sure if just simple line modification works, but I'll try. Anyway, cur_path (/LightGBM/python-package/lightgbm/libpath.py/lib_lightgbm.so) is invalid, right? Why not restore to previous valid path?

[jameslamb]

Why not restore to previous valid path?

Because as far as I can tell, the only thing relying on lib_lightgbm.so existing at that path is LightGBM-Transform.

Please see all the discussion in #5061 and all the pull requests linked from it. In response to changes in Python packaging standards upstream (e.g. in pip, build, and wheel), we did significant work to remove non-standard features of lightgbm's packaging setup. One of those non-standard features was the fact that it permitted lib_lightgbm.so to be found in so many different places.

That previous path (lightgbm/lib_lightgbm.so) was left behind only in an effort to minimize as much as possible the number of changes I was proposing. I didn't carefully scrutinize each path (which is how the mistake this PR exposes was allowed to persist).

But now that I am looking very carefully at each path as a result of this PR, I'd prefer to remove a path that, as far as I can tell, only LightGBM-Transform relies on.

Having fewer possible places that lib_lightgbm.so can be found has the following benefits:

• speeds up library loading
• reduces the risk of accidentally loading the wrong library (e.g. one left over by some other build of an older LightGBM version)

Ultimately @shiyu1994 has the power to override me in this project, so if he says to drop my objection then I'll drop it.

But I feel strongly that making lightgbm safer is preferable to keeping around a rarely-used path only for the benefit of LightGBM-Transform.

[chjinche]

@jameslamb if I understand correctly, the first candidate path should be removed, right? Can you commit the other three will not change in the future? Please kindly share your future plan.

1. LightGBM/python-package/lightgbm/libpath.py/lib_lightgbm.so
2. LightGBM/python-package/lib_lightgbm.so
3. LightGBM/python-package/lightgbm/bin/lib_lightgbm.so
4. LightGBM/python-package/lightgbm/lib/lib_lightgbm.so

[jameslamb]

the first candidate path should be removed, right

correct.

can you commit the other three will not change in the future

I have no plan to remove those, correct.

[jameslamb]

Based on microsoft/lightgbm-transform#26 being merged and the changes that were passed there, I guess you were able to resolve this in lightgbm-transform @chjinche . Thank you!

@shiyu1994 could you please update this PR to just completely remove that first path? Let's take this opportunity to simplify lightgbm.

[shiyu1994]

Thanks. I've removed the first library path.

[jameslamb]

Thanks very much!