Skip to content
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-package] add more type hints in basic.py #5729

Merged
merged 4 commits into from
Feb 27, 2023
Merged

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Feb 20, 2023

Contributes to #3756.

Adds some type hints basic.py.

  • proposes using "ctypes._Pointer' as a type hint for ctypes pointers
  • adds type hints on _InnerPredictor.__create_sparse_native(), _InnerPredictor.__get_num_preds(), _InnerPredictor.__pred_for_csr(), _InnerPredictor.__pred_for_csc()
  • switches to using keyword arguments (instead of positional ones) in calls to those functions, to avoid "accidentally passed data in the wrong order" types of bugs and make it easier for people reading the code to understand what's goin on
    • (extra important for methods like these where most of the arguments all can be small integers... it'd be easy to make a mistake and not catch it)

This improves mypy's ability to catch bugs, and makes it a bit easier to understand what the code is doing when reading it.

Notes for Reviewers

Back in #5446, we talked about how to type-annotate ctypes pointers. At the time @StrikerRUS recommended using ctypes._Pointer #5446 (comment) but I proposed just using Any, because I was afraid that the name _Pointer beginning with _ meant that it was consider an internal implementation detail that might change in future versions of ctypes.

This PR proposes adopting that suggestion to use ctypes._Pointer. Since then, I've learned more about how mypy evaluates type annotations.

Specifically, per the mypy docs (link)

Type comments can’t cause runtime errors because comments are not evaluated by Python.

In a similar way, using string literal types sidesteps the problem of annotations that would cause runtime errors.

I think that using a string-literal type hint means that if a future ctypes version removes ctypes._Pointer, it'll break type hinting but not actually cause runtime issues for users. Given that, I now support using the more specific type annotation to improve mypy's ability to catch bugs.

Consider the following example:

cat > ./test.py <<EOL
import ctypes

def do_stuff(x: "ctypes._does_not_exist") -> None:
    print(x)

do_stuff("--- hello ---")
EOL

mypy fails with an informative error.

mypy ./test.py

test.py:3: error: Name "ctypes._does_not_exist" is not defined [name-defined]
Found 1 error in 1 file (checked 1 source file)

But the code runs without issue.

python ./test.py

--- hello ---

@jameslamb jameslamb requested a review from guolinke February 26, 2023 19:29
@jameslamb jameslamb merged commit e8cdc2c into master Feb 27, 2023
@jameslamb jameslamb deleted the python/basic-hints branch February 27, 2023 04:23
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed.
To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues
including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants