-
Notifications
You must be signed in to change notification settings - Fork 9
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 lgbm lib not found #125
Conversation
if libpath == "" | ||
throw(LibraryNotFoundError("$(library_name) not found. Please ensure this library is either in system dirs or the dedicated paths: $(custom_paths)")) | ||
if libpath != "" | ||
@info("$(library_names) found in $(custom_paths)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong if the lib was found in DL_LOAD_PATH
or ENV["PATH"]
. You'd either want to have this last if/else as part of the fallback block above, or have a flag indicating whether system dirs were used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its simpler than that, if the libpath
was not empty then it will either have a custom path prepended or will just be the libname if it was found in system dirs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its simpler than that, if the
libpath
was not empty then it will either have a custom path prepended or will just be the libname if it was found in system dirs.
I'm not sure what your suggestion is here. Are you saying this is a non-issue? That the log message should be something different? Or something else?
For clarity I am suggesting something like:
if libpath != ""
@info("$(library_names) found in `DL_LOAD_PATH`, or system library paths $(ENV["PATH"])!")
return libpath
end
# try specified paths
@info("$(library_names) not found in `DL_LOAD_PATH`, or system library paths, trying fallback")
libpath = Libdl.find_library(library_names, custom_paths)
if libpath != ""
@info("$(library_names) found in $(custom_paths)")
else
throw(LibraryNotFoundError("$(library_names) not found. Please check this library using " *
"Libdl.dlopen(l; throw_error=true) where l = joinpath(custom_paths, lib)"))
end
return libpath
@@ -63,11 +63,14 @@ end | |||
cp(settings["ref_lib_lightgbm_path"], settings["custom_fixture_path"]) # fake file copied from lightgbm | |||
|
|||
# Act | |||
output = LightGBM.find_library(settings["sample_lib"], [src_dir]) | |||
push!(Libdl.DL_LOAD_PATH, src_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to move these push!
and deleteat!
lines into the setup_env
and teardown
functions, as this means any additional tests won't need to remember to add these lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small changes needed, but LGTM otherwise
@info("$(library_name) not found in system dirs, trying fallback") | ||
libpath = Libdl.find_library(library_name, custom_paths) | ||
if libpath != "" | ||
@info("$(library_names) found in `DL_LOAD_PATH`, or system library paths $(ENV["PATH"])!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENV["PATH"]
is only the system path for link libraries on windows. This doesn't apply in linux or mac. See the README
@@ -16,21 +16,25 @@ struct LibraryNotFoundError <: Exception | |||
end | |||
|
|||
|
|||
function find_library(library_name::String, custom_paths::Vector{String}) | |||
function find_library(library_names::Vector{String}, custom_paths::Vector{String}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres no need to make this to a Vector{String}
(because we're not hunting for multiple libs).
As I understand, the "fix" here is to call Libdl.find_library
like this: Libdl.find_library([library_name])
instead of Libdl.find_library(library_name)
It would be good if you can explain how the fix works, or what was going wrong before that this changes. |
@FatemehTahavori this PR does not fix anything, I got a M1 mac to test with Let me first start by stating what the issue is.
Next, I note that brew installs packages in arm mode. So now I can run a hypothesis. If what I stated above is true, then when I have arm libomp and arm lightgbm, it will fail to load with intel julia, and succeed to load with arm julia.
The julia install at the 2 path (the first command) is the ARM one, and the julia install without the 2 in the path (the second command) is the Intel install. Now, to explain how I made this work
In light of this I have to recommend this PR be closed, since it doesn't actually do anything or fix the issue (this PR can't possibly fix cross-architecture linking nor is it the place of this specific project top fix that problem) |
@danielsoutar please see the above. I recommend the closure of this PR Also @FatemehTahavori in light of the above it seems the advice given in #122 is incorrect; the correct advise should be to install julia 1.8 with ARM build on mac M1 and to either compile own lightgbm binary or use the brew binary and set |
@yaxxie we could reproduce the issue in docker: Julia developmentdocker run -d -it --platform=linux/x86_64 --name lgbm -v Connect to Container using VSCodeInside Containercd ~ To add the package to Julia:import Pkg Add this branch to Julia:(@v1.6) pkg> add LightGBM#Fix_lgbm_libNotFound |
still fails that particular test when I check it. Furthermore, I suspect there is something not quite right about that docker image because:
and when I check the system linker paths (in that docker image) I find this:
You can see that for the
and then if you launch julia on this machine and try to load libcrypt:
You can see it works. As a bonus: inside the docker image, I draw your attention to this:
and remind ourselves what we found when looking for libcrypt:
notice this one: So once again, I put it to you that this PR fixes nothing. |
Since
I think that this PR needs to be closed. Please close it @FatemehTahavori |
@FatemehTahavori would you mind closing this PR? Unless I am mistaken, I don't believe we require it. |
No description provided.