[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.
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.
look like file is a built-in, can you help rename it?
python-package/setup.py
Outdated
| shutil.rmtree(file_path) | ||
|
|
||
|
|
||
| def silent_call(cmd): |
There was a problem hiding this comment.
I am not so sure about what's this function for, why makes it silent?
There was a problem hiding this comment.
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.
@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.
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.
@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.
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.
@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.
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.
@wxchan Will exitcode be OK?
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 After 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 nanResult with Python 2.7.13Result with Python 3.5.3Result 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?