-
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
[ci] warnings from mypy #3867
Comments
Up to 30 errors as of d6ebd06. This will happen...adding more type hints gives Also, some of the issues below will be fixed when #3916 is merged. 30 errors in 3 files (check 10 source files)
|
I was looking for some more information on how to test my fixes. Is there something more extensive to ensure I'm not breaking anything, other than running mypy? I'm quite new to python, and I just wanted to ensure I'm making correct changes. Any advice on this one? |
Hi @AXRegister , thanks very much for your interest in contributing! To test locally, you can try installing the package locally and running the unit tests. Like this: # install the Python package
git submodule init
git submodule update --recursive
cd python-package
python setup.py install
# test the Python package
cd ..
pytest tests/python_package_test If those work ok, you can have some confidence that your changes won't break existing functionality. When you open a pull request, a much more extensive set of checks (checking different combinations of Python version, compiler, and operating system) will run automatically and might suggest additional changes. If you run into any problems, please either ask a question here or open a pull request and ask questions there. |
Up to 64 errors as of b5502d1. Found 64 errors in 3 files (checked 10 source files)
|
71 errors as of a2b60e8 Found 71 errors in 5 files (checked 10 source files)
|
This issue has been open with the label I opened some PRs for this tonight, and plan to finish it over the next few days. I'd like to enable Line 79 in 67b4205
|
59 errors as of f57ef6f Found 59 errors in 3 files (checked 10 source files)
|
53 errors as of e048a6b Found 53 errors in 3 files (checked 10 source files)
|
35 errors as of 946817a Found 35 errors in 2 files (checked 10 source files)
|
8 errors as of 8f577de Found 8 errors in 2 files (checked 9 source files)
That's especially good given that the type hints are so much more complete in the project than they were the last time I posted an update here back in February: #3867 (comment) Getting close! |
I keep looking this up every time I run into it, so wanted to put it in writing here. For almost all cases, the There's one case where that isn't true. For the following specific combination,
That's been true since support for SHAP contributions on sparse matrices was added in #3000. Here's the code path where that happens: LightGBM/python-package/lightgbm/basic.py Line 1328 in 6f19edd
LightGBM/python-package/lightgbm/basic.py Lines 1361 to 1375 in 6f19edd
LightGBM/python-package/lightgbm/basic.py Line 1472 in 6f19edd
That one possibility means that everything handling the result of a For example, this added some complexity to the Dask interface: LightGBM/python-package/lightgbm/dask.py Lines 957 to 1027 in 6f19edd
For the purpose of this issue and #3756, this can be handled with some mix of the approaches listed at https://mypy.readthedocs.io/en/stable/type_narrowing.html. Longer-term, I think it's worth investigating whether it's possible to return an array type for that combination. Although that'd have to be done carefully, since (for example), I couldn't quite figure out from the description on #3000 why the decision was made to return a list. @guolinke or @imatiach-msft do you remember? |
Summary
mypy
shows some issues in LightGBM's Python package.mypy \ --exclude='python-package/compile/|python-package/build' \ --ignore-missing-imports \ python-package/
18 errors in 4 files (click me)
Motivation
mypy
is a static analysis tool for type checking Python code. It can catch issues not caught by other linters and unit tests, like:None
but other code acts on it as if it is not Nonenum.lower()
whennum
is anint
)fun(other_fun())
being problematic iffun()
expects a data frame andother_fun()
returns a listLightGBM's Python package recently started on the path to adding type hints (#3756), so this project can start to get more value out of
mypy
.Description
If you're interested in contributing, you do not need to fix all of the
mypy
issues at once. Pull requests that address any of the issues found bymypy
are welcome!To test your fixes, install the latest version of
mypy
, then run it over the code.pip install --upgrade mypy mypy \ --exclude='python-package/compile/|python-package/build' \ --ignore-missing-imports \ python-package/
The text was updated successfully, but these errors were encountered: