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

[py] Use generic in xgboost model doc decorator #11071

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

MrEarle
Copy link
Contributor

@MrEarle MrEarle commented Dec 7, 2024

Context

XGBoost classes are wrapped with xboost_model_doc decorator.

Problem

xgboost_model_doc is typed as returning Callable[[Type], [Type], which ends up obfuscating the real type of the class. For example, if you try to define a union type with other classes, it ends up as Unknown:

from typing import Union
from xgboost import XGBRegressor

class MyClass1: ...
class MyClass2: ...

ModelType = XGBRegressor | MyClass1 | MyClass2
ModelType2 = Union[XGBRegressor, MyClass1, MyClass2]

Both ModelType and ModelType2 are treated as variables instead of types by mypy.

image
image

Solution

Use a generic type so that the type checker can infer that the returned class is the same as the decorated one.

With this fix, the above issue is solved.
image

Caveat

This is my first time contributing to this repo. I'm pretty sure this won't break anything (it's just a typing change), but please let me know if I missed anything,

@MrEarle MrEarle marked this pull request as ready for review December 7, 2024 13:39
@MrEarle MrEarle changed the title Use generic in xgboost model doc decorator [py] Use generic in xgboost model doc decorator Dec 7, 2024
@trivialfis
Copy link
Member

trivialfis commented Dec 8, 2024

Hi, thank you for the PR. I don't have any objection to the change but would like to learn more about the issue. Here's the code snippet copied from your example:

from typing import Union
from xgboost import XGBRegressor

class MyClass1: ...
class MyClass2: ...

ModelType = XGBRegressor | MyClass1 | MyClass2
ModelType2 = Union[XGBRegressor, MyClass1, MyClass2]
reveal_type(ModelType)
reveal_type(ModelType2)


def my_fn(m: ModelType) -> None:
    pass

Running mypy 1.13.0 with python 3.11.10:

$ mypy ./model_type.py 

I got the following output:

model_type.py:11: note: Revealed type is "types.UnionType[xgboost.sklearn.XGBRegressor, model_type.MyClass1, model_type.MyClass2]"
model_type.py:12: note: Revealed type is "typing._SpecialForm"
Success: no issues found in 1 source file

It doesn't contain any errors. I'm using the master branch of XGBoost. Also, have you tried the typing.TypeAlias?

@MrEarle
Copy link
Contributor Author

MrEarle commented Dec 8, 2024

Huh now that you mention it, I only get a mypy error when combining it with a sklearn model:

from xgboost import XGBRegressor
from sklearn.ensemble import RandomForestRegressor # type: ignore

class MyClass:
    def fit(self, X, y):
        return self

_ModelType = MyClass | XGBRegressor | RandomForestRegressor

reveal_type(_ModelType)

def train_model(model: _ModelType) -> _ModelType:
    return model.fit([[1]], [1])

And mypy's output:
image

But if I remove sklearn's model, mypy doesn't complain, so part of the issue is probably with sklearn as well (my bad).

But even without the union type, vscode has a hard time infering the type:

Without the fix:
image

With the fix:
image

So I guess the issue is really with VSCode.

@trivialfis
Copy link
Member

I don't use vscode, if not mistaken, it might be using https://github.com/microsoft/pyright instead of mypy? I have tried pyright before, but the result was not consistent with mypy, not sure about the current status.

Anyway, we can merge the PR if it helps with vscode users and the tests can pass.

@MrEarle
Copy link
Contributor Author

MrEarle commented Dec 11, 2024

Yeah, sorry. Forgot to run black. Should be fine now.

@trivialfis trivialfis merged commit a584757 into dmlc:master Dec 11, 2024
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants