-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[python] [setup] improving installation #880
Conversation
@StrikerRUS, |
For what this line is responsible? In my case it just creates
|
@StrikerRUS it points to a empty line |
python-package/setup.py
Outdated
logger.warning("Compilation with MSBuild from existing solution file failed.") | ||
else: | ||
os.chdir("..") | ||
return |
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 it is better to avoid return
this PR looks good to me. |
@guolinke Sorry about empty line - I forgot to change the line after pushing new code. I'm talking about this one
As I said before, while installation it copies dll files into strange folder and while creating wheel it copies dll into |
@guolinke Please answer
|
@StrikerRUS |
I suppose that this
since it'll anyway fail on this line with
|
python-package/setup.py
Outdated
shutil.rmtree(file_path) | ||
if os.path.isdir(path): | ||
contents = os.listdir(path) | ||
for file in contents: |
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.
look like file
is a built-in, can you help rename it?
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.
@wxchan Sure.
python-package/setup.py
Outdated
shutil.rmtree(file_path) | ||
|
||
|
||
def silent_call(cmd): |
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 am not so sure about what's this function for, why makes it silent
?
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.
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.
@StrikerRUS ok, got it.
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.
Can this function be designed like def silent_call(cmd, raise_error=True, error_msg='')
? In this case, if cmake_cmd fails, you don't need to call make_cmd then.
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.
@wxchan
It already has described behavior. In case of any error it returns 1
and you should just check returned value.
Do you mean that I should change this code
status = silent_call(cmake_cmd + ["-G", "MinGW Makefiles"])
status += silent_call(["mingw32-make.exe", "_lightgbm"])
if status != 0:
raise Exception('Please install CMake and MinGW first')
to
status = silent_call(cmake_cmd + ["-G", "MinGW Makefiles"])
if status != 0:
raise Exception('Please install CMake first')
status = silent_call(["mingw32-make.exe", "_lightgbm"])
if status != 0:
raise Exception('Please install MinGW first')
?
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.
yes, I think it's better, and you can move raise Exception into function to avoid duplicates, but it's trivial, up to you.
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.
@wxchan Moving raising Exception into the function will cause dozens of try/except
into the main code and will make code messy.
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.
doesn't sth like below work? why need try/except in main code? not sure, LGTM now, it's up to you.
def silent_call(cmd, raise_error=True, error_msg='')
try:
// do sth
return 0
except:
if raise_error:
raise Exception(error_msg)
return 1
silent_call(cmake_cmd, error_msg='Please install CMake first')
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.
@wxchan It seems you are right! raise_error
will help to deal with such situation
if status == 0 and os.path.exists(lib_path):
@wxchan Please comment on #880 (comment) and #880 (comment) |
python-package/setup.py
Outdated
os.system(cmake_cmd + " ../lightgbm/") | ||
build_cmd = "mingw32-make.exe _lightgbm" | ||
logger.info("Starting to compile with CMake and MinGW.") | ||
status = silent_call(cmake_cmd + ["-G", "MinGW Makefiles"]) |
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.
the name status
is a little confusing, can it be succeed
or fail
or sth like this? (I don't read through all codes because I am not sure about those VS install logic).
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.
@wxchan Will exitcode
be OK?
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.
ignore this.
To be honest, I am not sure what some of the functions in
@henry0312 can you help check this? |
@wxchan To be more precise, this command does following:
So, I suppose I can safely delete the line, because, as I understand, the library file should be located into the same directory as python files on user's machine, not into the separate one. |
…stalled on VS 2017 image
I'm not sure do we need to create the python wheel with .dll-file compiled by MinGW while Appveyor tests. At present, Appveyor MINGW job just checks ability to execute compiled by MinGW dll, but all artifacts are still generated by MSVC. |
I installed Python 2.7.13, 3.4.6 and 3.5.3 in my OSX, so I'll check the tests later if necessary (I'm busy today). |
@guolinke I wanted to say, that just adding I has only one though that it somehow affects random generator. |
@henry0312 Thanks for the tip! Will use it next time. |
@StrikerRUS maybe the bug is caused by python or sklearn ? |
@guolinke I'm trying to check it by comparing datasets passed to the test in different environments. |
@guolinke I've created two python environments on my machine: 3.5 and 3.6. After that I've copied the same dll in both of them (I took dll from successful build artifacts) and then launched the same task (from logs):
In both environments everything seems to be OK. Then I open |
@StrikerRUS You will mix CRLF and LF for line breaks when using Windows Notepad. |
@Laurae2 Is there any automatic method to fix it? |
@StrikerRUS |
@guolinke Sure, next time I wouldn't open with Notepad. |
@StrikerRUS I am not sure, you can run the test locally ? |
This may also explain that adding However, my |
@StrikerRUS |
@guolinke yeah. |
@StrikerRUS |
I reproduced the error with Python 3.5.3, and I solved it.
|
Patchdiff --git a/tests/python_package_test/test_sklearn.py b/tests/python_package_test/test_sklearn.py
index f942543..0ee655d 100644
--- a/tests/python_package_test/test_sklearn.py
+++ b/tests/python_package_test/test_sklearn.py
@@ -186,7 +186,7 @@ class TestSklearn(unittest.TestCase):
check_parameters_default_constructible(name, estimator)
check_no_fit_attributes_set_in_init(name, estimator)
# we cannot leave default params (see https://github.com/Microsoft/LightGBM/issues/833)
- estimator = estimator(min_data=1, min_data_in_bin=1)
+ estimator = estimator(min_child_samples=1, min_data_in_bin=1)
for check in _yield_all_checks(name, estimator):
if check.__name__ == 'check_estimators_nan_inf':
continue # skip test because LightGBM deals with nan Result with Python 2.7.13
Result with Python 3.5.3
Result with Python 3.6.2
|
The error occurs in X = [[ 0.5488135 0.71518937 0.60276338]
[ 0.54488318 0.4236548 0.64589411]
[ 0.43758721 0.891773 0.96366276]
[ 0.38344152 0.79172504 0.52889492]
[ 0.56804456 0.92559664 0.07103606]
[ 0.0871293 0.0202184 0.83261985]
[ 0.77815675 0.87001215 0.97861834]
[ 0.79915856 0.46147936 0.78052918]
[ 0.11827443 0.63992102 0.14335329]
[ 0.94466892 0.52184832 0.41466194]]
y = [0 1 2 0 1 2 0 1 2 0] I don't know why All tests logs:
|
@henry0312 Thank you very-very much! |
@StrikerRUS @wxchan should we solve the alias problem in python ? |
@guolinke Your idea seems to be the most realistic. @henry0312 @guolinke I've made another one experiment: I reset params back to In a result, MSVC + Python 3.5 task failed with |
Python dictionary doesn't have key orders, therefore when both |
@guolinke I am kinda lost by your discussions. Why order of params will change result? |
@wxchan |
* disabled logs from compilers; fixed #874 * fixed safe clear_fplder * added windows folder to manifest.in * added windows folder to build * added library path * added compilation with MSBuild from .sln-file * fixed unknown PlatformToolset returns exitcode 0 * hotfix * updated Readme * removed return * added installation with mingw test to appveyor * let's test appveyor with both VS 2015 and VS 2017; but MinGW isn't installed on VS 2017 image * fixed built-in name 'file' * simplified appveyor * removed excess data_files * fixed unreadable paths * separated exceptions for cmake and mingw * refactored silent_call * don't create artifacts with VS 2015 and mingw * be more precise with python versioning in Travis * removed unnecessary if statement * added classifiers for PyPI and python versions badge * changed python version in travis * added support of scikit-learn 0.18.x * added more python versions to Travis * added more python versions to Appveyor * reduced number of tests in Travis * Travis trick is not needed anymore * attempt to fix according to #880 (comment)
This PR makes installation of python package more clear.
Should fix #874 .
Adds possibility to install without CMake on Windows from existing .sln-file and Visual Studio.
Am I right that existing solution file hasn't configuration with GPU support?