[ci] [python-package] tell mypy 'auto' has a special meaning for feature_name and categorical_feature

[jameslamb]

Contributes to #3756.
Contributes to #3867.

Fixes the following error from mypy.

python-package/lightgbm/basic.py:691: error: Incompatible types in assignment (expression has type "List[object]", variable has type "Union[List[str], List[int], str, None]")  [assignment]

For arguments feature_name and categorical_feature, the Python package supports passing 3 possible non-None values:

• list of strings (column names)
• list of ints (0-based positional indexes of the columns)
• string "auto" (interpreted as "figure it out automatically" for pandas inputs, and as None otherwise)

Using the typing.Literal annotation, added in Python 3.8, mypy is able to understand that if these arguments are not None or a list, they must be literally "auto", not any arbitrary string (as the current type hint of str implies).

This helps it to understand that, for example...

if categorical_feature != "auto" and categorical_feature is not None:
    # categorical_feature MUST be a list at this point

Notes for Reviewers

For more details on the Literal type, see https://mypy.readthedocs.io/en/stable/literal_types.html#literal-types.

For an explanation of how string-literal type hints can be used to prevent runtime users, see https://mypy.readthedocs.io/en/stable/runtime_troubles.html.

[jmoralez]

Why is this removed? I think this is meant to create a copy and avoid errors derived from this value being changed outside of lightgbm.

[jameslamb]

I think this is meant to create a copy and avoid errors derived from this value being changed outside of lightgbm

Looking back through the blame, this line was introduced in #2121. I don't see any PR review discussion about it there or in the comment linked to from there... I assumed this list() was just thrown in to be extra-sure that from this point forward, the value was a list.

If we want protection against things outside of LightGBM having side effects on this value or vice versa (via the fact that a list is mutable and passed by reference), I think it'd be clearer to handle that with copy.deepcopy() explicitly, somewhere further upstream in the code.

Would you like me to try that?

[jmoralez]

I think using list(collection) is a common way to ensure that:

1. From this point forward the collection is a list.
2. The variable now holds a copy of the original collection.

So if the input was already a list calling list() on it only creates a copy and if was some other type of iterable it achieves both.

Since the input is expected to be a list of strings deepcopy might be overkill and won't achieve 2.

[jameslamb]

Ok I'll put this back for now. I really think it's both unclear that the point is to create a copy and unnecessary for hte purpose "ensure that it's a list", given that by this point in the code we've already checked that categorical_feature is,

not None:

LightGBM/python-package/lightgbm/basic.py

Line 685 in 3d63dda

if categorical_feature is not None:

and not the string "auto"

LightGBM/python-package/lightgbm/basic.py

Lines 688 to 689 in 3d63dda

	if categorical_feature == 'auto': # use cat cols from DataFrame
	categorical_feature = cat_cols_not_ordered

.. so the only other things that attribute permits are List[str] or List[int].

[jameslamb]

added this back in 51ab03a and ceef1f6

It's necessary to add a # type: ignore comment if we're going to keep that code branch, to avoid this from mypy.

python-package/lightgbm/basic.py:694: error: Incompatible types in assignment (expression has type "List[object]", variable has type "Union[List[str], List[int], Literal['auto'], None]")  [assignment]

[github-actions]

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.