[python] add more type hints in basic.py

[jameslamb]

Contributes to #3756.

Proposes adding a few more hints in basic.py. Just quickly scanned through this file tonight and chose a few hints that looked easy to determine just from looking at the code and docstrings, trying to slowly make progress on #3756 as I have time.

[StrikerRUS]

Suggested change

	def attr(self, key: str) -> Any:
	def attr(self, key: str) -> Optional[str]:

LightGBM/python-package/lightgbm/basic.py

Lines 3987 to 3989 in 65b3db1

	if value is not None:
	if not isinstance(value, str):
	raise ValueError("Only string values are accepted")

[jameslamb]

Oh interesting, I didn't realize this could only be set by Booster.set_attr().

Can you help me understand the purpose of these methods? It seems they were added back in 2016 (eba6d20#diff-5e0fb2a7b1ec4988ccde5e74a6c4b609841c1143f281da2afc85e8c464d37b3b) and today are only used in one place, a test.

git grep -E '\.attr\('

LightGBM/tests/python_package_test/test_sklearn.py

Line 483 in 1cc9f9d

assert gbm.booster_.attr('attr_set_inside_callback') == '40'

git grep -E '\.set_attr\('

LightGBM/tests/python_package_test/test_sklearn.py

Line 32 in 1cc9f9d

env.model.set_attr(attr_set_inside_callback=str(env.iteration * 10))

[jameslamb]

I agree either way about the type hint so I don't think this conversation needs to block this PR. But I'm wondering if there's an opportunity here to remove these methods and the internal object Booster.__attr.

[StrikerRUS]

Can you help me understand the purpose of these methods?

I'm sorry, I can't.

I guess it's a legacy code that was initially borrowed from XGBoost. In XGBoost those methods are used for distributed training with Rabit: dmlc/xgboost@ecb3a27.

[StrikerRUS]

I have no objection to remove set_attr() and attr() methods in a separate PR to make LightGBM codebase clearer.

[jameslamb]

Ok thanks! I'll make a separate PR that proposes removing them.

[jameslamb]

created #5272

[StrikerRUS]

LGTM!

[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.