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.

Sign up for GitHub

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

Jump to bottom

Add type hints to core.py #7707

Merged

trivialfis merged 10 commits into dmlc:master from bridgream:mypy

Mar 23, 2022

Contributor

bridgream commented Feb 28, 2022

I attempted to add type hints to core.py. I wasn't able to eliminate all mypy errors, but I hoped to make incremental changes because adding type hints seemed very intrusive and could easily trigger conflicts.

Related to #6496

Contributor Author

bridgream commented Feb 28, 2022

CI failed because I used typing.ParamSpec, which was only introduced in Python 3.10. I'll seek to fix that.

trivialfis reviewed

View reviewed changes

python-package/xgboost/core.py Outdated Show resolved Hide resolved

trivialfis reviewed

View reviewed changes

python-package/xgboost/core.py Outdated

@@ @@ -27,12 +28,18 @@ @@
               # lesser tested.  For now we encourage users to pass a simple list of string.
               FeatNamesT = Optional[List[str]]
+              list_or_dict = TypeVar("list_or_dict", List, Dict)
+              ArrayLike = np.ndarray | Sequence

Member

trivialfis Feb 28, 2022 •

edited

Loading

I think for now we can just use Any. I have been working on defining the protocol of input data: #7242 We accept a lot more types than ndarray.

python-package/xgboost/core.py Outdated Show resolved Hide resolved

python-package/xgboost/core.py Outdated

                       raise RuntimeError(f"expected {ctype} pointer")
                   res = np.zeros(length, dtype=dtype)
-                  if not ctypes.memmove(res.ctypes.data, cptr, length * res.strides[0]):
+                  if not ctypes.memmove(res.ctypes.data, cptr, length * res.strides[0]):  # "memmove" does not return a value?

Member

trivialfis Feb 28, 2022

I think that's probably a bug in mypy.

Contributor Author

bridgream Mar 1, 2022

You were right. By the way, the maintainers of typeshed were able to fix this bug in a flash.

python-package/xgboost/core.py Outdated

    
            @@ -790,7 +807,7 @@ def set_uint_info(self, field: str, data) -> None:
          
                      from .data import dispatch_meta_backend

                      dispatch_meta_backend(self, data, field, 'uint32')

                  def save_binary(self, fname, silent=True) -> None:

                  def save_binary(self, fname: str | os.PathLike, silent: bool = True) -> None:

Member

trivialfis Feb 28, 2022

Consider using Union instead. This requires from future import __annotations__, which has different behavior.

Contributor Author

bridgream Mar 1, 2022

I'm not sure if that different behavior matters (except for the explicit import), but since typing.Union is available for Python <3.10, I agree it's better to stick with that.

python-package/xgboost/core.py Outdated

@@ @@ -806,7 +823,7 @@ def save_binary(self, fname, silent=True) -> None: @@
                                                            c_str(fname),
                                                            ctypes.c_int(silent)))
-                  def set_label(self, label) -> None:
+                  def set_label(self, label: Collection) -> None:

Member

trivialfis Feb 28, 2022

Not sure why is this a Collection? Maybe ArrayLike?

Contributor Author

bridgream Mar 1, 2022

My bad; I was confused by dispatch_meta_backend but apparently I missed the docstring...

Contributor Author

bridgream commented Mar 1, 2022

The CI build still failed due to TypeError: '_ctypes.PyCArrayType' object is not subscriptable. Could it be because I used a platform specific annotation Union[bytes, ctypes.Array[ctypes.c_char_p]]?

bridgream marked this pull request as draft

March 1, 2022 02:42

Contributor Author

bridgream commented Mar 1, 2022 •

edited

Loading

I was able to eliminate most mypy errors (from 60+ to 19; hopefully that was not because I introduced too many Any). I think most of the remaining errors would involve more intrusive changes or changes of other files.

Contributor Author

bridgream commented Mar 2, 2022

It turns out we cannot include ctypes.pointer[] in any part of the Python scripts. However, to properly annotate typed C pointers, it seems we have to use this grammar. In fact, typeshed has a class interface for ctypes.pointer but this is not recognized by Python.

Maybe we should separate the definitions of types from source code by putting them in a stub file?

Member

trivialfis commented Mar 13, 2022

Apologies for the slow response, will come back to this tomorrow.

Member

trivialfis commented Mar 14, 2022

Maybe we should separate the definitions of types from source code by putting them in a stub file?

That sounds like a good idea, we need to have some stubs later.

bridgream force-pushed the mypy branch from 926bf6c to e6098c1 Compare

March 15, 2022 00:43

Contributor Author

bridgream commented Mar 15, 2022

@trivialfis Thanks for following up! I've done a quick demo using stub (basically I just used stubgen and filled types that weren't automatically inferred, so that I was able to make mypy satisfied) and erased previous work (I made a backup of the previous commit, though).

Some issues:

With the existence of core.pyi, mypy simply ignores all other files including core.py itself (which is expected behavior). Is this still helpful? I guess we also wanted mypy to help us do static check for implementation?
Should we generate a stub file for each .py file, or just one __init__.pyi enumerating all APIs?
I've added a _typing.py to collect all typing-related declaration but maybe I should follow typing.py as in this draft PR. Could you please offer some advice?

Member

trivialfis commented Mar 16, 2022

Thank you for experimenting with this! Does it make sense to simply use ctypes.pointer for now? I think python typing is still evolving and we don't have to be too harsh on ourselves at the moment. It's unlikely that we have a perfect solution for everything in short term.

Contributor Author

bridgream commented Mar 16, 2022

@trivialfis Simply using ctypes.pointer should work. In principle, typeshed invents its own annotation for specific C pointers (e.g. ctypes.pointer[ctypes.c_char_p], but subscription is not natively supported by Python), so this is only usable in .pyi files.

Let me move on with inline annotation without specific types for C pointers for now.
Also no worries about the work; I'm having great fun playing with the typing system after all.

Member

trivialfis commented Mar 16, 2022

Let me move on with inline annotation without specific types for C pointers for now.

Excellent!


          use inline typing for core.py

a2afc75

bridgream force-pushed the mypy branch from 296fd01 to a2afc75 Compare

March 18, 2022 00:00

Chengyang Gu added 3 commits

March 17, 2022 20:09


          fix missing import

1384c10


          fix lint and incorrect annotation

a797bf6


          fix linting

48e816c

Member

trivialfis commented Mar 21, 2022

Hi, please let me know when it's ready for review. ;-)

Contributor Author

bridgream commented Mar 21, 2022

Hi @trivialfis, after some of our work, there are still 22 mypy errors in core.py but I find them quite tricky to tackle. The errors can be generated with

cd python-package
mypy xgboost/core.py --ignore-missing-imports

Besides the errors that relate to ctypes, some errors are due to the flexible user interface. For example, the setter feature_names starting from core.py:1004 takes either a str, a List[str] or None. Even if there's code to deal with each case, mypy cannot understand the implementation but simply throws errors.

While I don't think simply adding # type: ignore is a good solution, I'm not sure whether we still should solve all the errors now. Otherwise, given the improvement we've already done, I think it's a good time to review and possibly merge.

bridgream marked this pull request as ready for review

March 21, 2022 23:37

bridgream mentioned this pull request

Add Type Hints for Python Package #7742

Merged


          Some renames and formats.

3bf538a

trivialfis reviewed

View reviewed changes

Member

trivialfis left a comment

Thank you for the nice work. One comment inlined.

python-package/xgboost/_typing.py Outdated

+              ArrayLike = Any
+              PathLike = Union[str, os.PathLike]
+              CupyT = ArrayLike  # maybe need a stub for cupy arrays
+              NdarrayOrCupyT = Union[np.ndarray, CupyT]

Member

trivialfis Mar 22, 2022

I think we might be better off to return Any for prediction. The return type is actually a dependent type, which means given the input type, the return type is known so it's not a union. From a static type language's point of view (like c++, rust), the union can be changed at runtime, but the return type is determined at compile time.

Member

trivialfis Mar 22, 2022 •

edited

Loading

mypy doesn't support dependent type yet, you can find some related feature requests in their issue list. To provide a concrete example, if the users use a cupy array as input to inplace_predict, then they know the output is also a cupy array. However, with Union the users will be asked to handle both numpy array and cupy array.

Contributor Author

bridgream Mar 23, 2022

I agree. By the way, should we use string literal for cupy types, e.g. "cupy.ndarray" instead of CupyT here https://github.com/bridgream/xgboost/blob/mypy/python-package/xgboost/core.py#L276

Member

trivialfis Mar 23, 2022

@bridgream I think it will show an error as mypy couldn't find cupy on the test environment (cupy is an optional dependency)

Member

trivialfis Mar 23, 2022

On a second thought, overloading might help, but the change might be tedious as cupy is an optional dependency.

Contributor Author

bridgream Mar 23, 2022

Maybe we should follow your solution in this PR by creating an interface. Since xgboost only touches some basic interface, this shouldn't take too much work. We could then introduce @overload to achieve better hints.
I can try that next.

trivialfis and others added 5 commits

March 22, 2022 17:53


          Fix.


          Unify the array like.

be6b161


          use Any for NdarrayOrCupyT

eeb7430


          Naming.

c68c124


          Fix merge.

e0eb6e3

trivialfis approved these changes

View reviewed changes

Member

trivialfis left a comment

Thank you for the nice work! Will merge as part of 1.6.

Contributor Author

bridgream commented Mar 23, 2022

Great, thank you! I'll continue to improve type hints in other Python scripts.

trivialfis merged commit c92ab2c into dmlc:master

bridgream deleted the mypy branch

March 24, 2022 00:09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet