[python-package] prefix all internal object names with "_"

[jameslamb]

Short Description

I'm opening this RFC to propose the following:

all objects in the Python package which are intended to be internal should have names beginning with _

The convention "anything starting with _ is internal" is common in Python programming. If LightGBM's Python package followed that convention consistently, it would simplify LightGBM's position on which parts of the Python package are considered "internal" and subject to breaking changes in any release.

Detailed Description

In pursuit of the v4.0.0 release (#5153), we have been accepting breaking changes which simplify the project and reduce its maintenance burden, so that limited maintainer time can be focused on providing a good experience with LightGBM's main functionality.

I think "all internal objects should begin with _" is one such type of change. Making it clearer which parts of the Python package are considered internal reduces the surface area of the public API which users need to consider, and increases contributors' flexibility to change implementations without breaking user code.

R packages support a "namespace" concept (see "Writing R extensions"), which clearly separates exported and internal objects, and I think that mechanism has been really helpful. The closest thing that Python has, to my knowledge, is the __all__ member for modules (https://docs.python.org/3/tutorial/modules.html#importing-from-a-package). __all__ controls the behavior of "star imports".

For example, if you run the following:

from lightgbm import *
dir()

the only LightGBM objects you'll see are those in the package's __all__.

This mechanism is a good start, but prefixing all internal objects with _ would be an even stronger signal about which things are internal. It would also improve the consistency of the Python package's code.

Specific Proposal

expand to see original proposal (click me)

I am proposing that the following be prefixed with a _ and treated as internal:

• LightGBM/python-package/lightgbm/basic.py

  Line 128 in ff947bf

  NUMERIC_TYPES = (int, float, bool)

• LightGBM/python-package/lightgbm/basic.py

  Line 144 in ff947bf

  def is_numeric(obj):

• LightGBM/python-package/lightgbm/basic.py

  Line 155 in ff947bf

  def is_numpy_1d_array(data):

• LightGBM/python-package/lightgbm/basic.py

  Line 160 in ff947bf

  def is_numpy_column_array(data):

• LightGBM/python-package/lightgbm/basic.py

  Line 168 in ff947bf

  def cast_numpy_array_to_dtype(array, dtype):

• LightGBM/python-package/lightgbm/basic.py

  Line 175 in ff947bf

  def is_1d_list(data):

• LightGBM/python-package/lightgbm/basic.py

  Line 190 in ff947bf

  def list_to_1d_numpy(data, dtype=np.float32, name='list'):

• LightGBM/python-package/lightgbm/basic.py

  Line 240 in ff947bf

  def cfloat32_array_to_numpy(cptr, length):

• LightGBM/python-package/lightgbm/basic.py

  Line 248 in ff947bf

  def cfloat64_array_to_numpy(cptr, length):

• LightGBM/python-package/lightgbm/basic.py

  Line 256 in ff947bf

  def cint32_array_to_numpy(cptr, length):

• LightGBM/python-package/lightgbm/basic.py

  Line 264 in ff947bf

  def cint64_array_to_numpy(cptr, length):

• LightGBM/python-package/lightgbm/basic.py

  Line 272 in ff947bf

  def c_str(string):

• LightGBM/python-package/lightgbm/basic.py

  Line 277 in ff947bf

  def c_array(ctype, values):

• LightGBM/python-package/lightgbm/basic.py

  Line 282 in ff947bf

  def json_default_with_numpy(obj):

• LightGBM/python-package/lightgbm/basic.py

  Line 292 in ff947bf

  def param_dict_to_str(data):

• LightGBM/python-package/lightgbm/basic.py

  Lines 436 to 470 in ff947bf

  	MAX_INT32 = (1 << 31) - 1
  	"""Macro definition of data type in C API of LightGBM"""
  	C_API_DTYPE_FLOAT32 = 0
  	C_API_DTYPE_FLOAT64 = 1
  	C_API_DTYPE_INT32 = 2
  	C_API_DTYPE_INT64 = 3
  	"""Matrix is row major in Python"""
  	C_API_IS_ROW_MAJOR = 1
  	"""Macro definition of prediction type in C API of LightGBM"""
  	C_API_PREDICT_NORMAL = 0
  	C_API_PREDICT_RAW_SCORE = 1
  	C_API_PREDICT_LEAF_INDEX = 2
  	C_API_PREDICT_CONTRIB = 3
  	"""Macro definition of sparse matrix type"""
  	C_API_MATRIX_TYPE_CSR = 0
  	C_API_MATRIX_TYPE_CSC = 1
  	"""Macro definition of feature importance type"""
  	C_API_FEATURE_IMPORTANCE_SPLIT = 0
  	C_API_FEATURE_IMPORTANCE_GAIN = 1
  	"""Data type of data field"""
  	FIELD_TYPE_MAPPER = {"label": C_API_DTYPE_FLOAT32,
  	"weight": C_API_DTYPE_FLOAT32,
  	"init_score": C_API_DTYPE_FLOAT64,
  	"group": C_API_DTYPE_INT32}
  	"""String name to int feature importance type mapper"""
  	FEATURE_IMPORTANCE_TYPE_MAPPER = {"split": C_API_FEATURE_IMPORTANCE_SPLIT,
  	"gain": C_API_FEATURE_IMPORTANCE_GAIN}

• LightGBM/python-package/lightgbm/basic.py

  Line 472 in ff947bf

  def convert_from_sliced_object(data):

• LightGBM/python-package/lightgbm/basic.py

  Line 482 in ff947bf

  def c_float_array(data):

• LightGBM/python-package/lightgbm/basic.py

  Line 502 in ff947bf

  def c_int_array(data):

• LightGBM/python-package/lightgbm/libpath.py

  Line 9 in ff947bf

  def find_lib_path() -> List[str]:

Considerations for Implementation

Check that any name being modified is only used at runtime and not part of the state for objects like Booster or LGBMClassifier which might be pickled (since pickle only stores references to imports and expects to be able to reference them when running load()).

Status: Accepted

@StrikerRUS @jmoralez @shiyu1994 @guolinke Whenever you have time, will you please comment here on whether or not you support this proposal? Thank you for your time and consideration.

This was originally opened as a request for comment, but it's been accepted and we're now accepting contributions to make more of the Python package's namespace private?

How to Contribute

Anyone is welcome to submit a pull request to help contribute to this effort.

Please limit pull requests to 1 function/method/module per pull request, to make them easier to review.

When you open pull requests, please put Contributes to #5313 in the description.

Add __all__ entries to modules.

• python-package/basic.py ([python-package] make public API members explicit with module-level __all__ variables #5655)
• python-package/callback.py ([python-package] make public API members explicit with module-level __all__ variables #5655)
• python-package/dask.py ([python-package] make public API members explicit with module-level __all__ variables #5655)
• python-package/engine.py ([python-package] make public API members explicit with module-level __all__ variables #5655)
• python-package/libpath.py ([python-package] make public API members explicit with module-level __all__ variables #5655)
• python-package/plotting.py ([python-package] make public API members explicit with module-level __all__ variables #5655)
• python-package/sklearn.py ([python-package] make public API members explicit with module-level __all__ variables #5655)

Prefix objects, functions, class attributes with _

• basic.ZERO_THRESHOLD ([python-package] prefix internal objects with '_' #5654)
• basic.NUMERIC_TYPES ([python-package] prefix NUMERIC_TYPES with _ to make it a internal object #5409)
• basic.is_numeric() ([python] Prefix basic.is_numeric() with _ #5421)
• basic.is_numpy_1d_array() ([python-package] prefix is_numpy_1d_array with _ #5520)
• basic.is_numpy_column_array() ([python-package] prefix is_numpy_column_array with _ #5531)
• basic.cast_numpy_array_to_dtype() ([python-package] prefix cast_numpy_array_to_dtype with _ #5532)
• basic.is_1d_list() ([python-package] prefix several internal functions with _ #5545)
• basic.list_to_1d_numpy() ([python-package] prefix several internal functions with _ #5545)
• basic.cfloat32_array_to_numpy() ([python-package] prefix several internal functions with _ #5545)
• basic.cfloat64_array_to_numpy() ([python-package] prefix several internal functions with _ #5545)
• basic.cint32_array_to_numpy() ([python-package] prefix several internal functions with _ #5545)
• basic.cint64_array_to_numpy() ([python-package] prefix several internal functions with _ #5545)
• basic.c_str() ([python-package] prefix several internal functions with _ #5545)
• basic.c_array() ([python-package] prefix several internal functions with _ #5545)
• basic.json_default_with_numpy() ([python-package] prefix internal objects with '_' #5654)
• basic.param_dict_to_str() ([python-package] prefix internal objects with '_' #5654)
• basic.MAX_INT32 ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_DTYPE_FLOAT32 ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_DTYPE_FLOAT64 ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_DTYPE_INT32 ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_DTYPE_INT64 ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_IS_ROW_MAJOR ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_PREDICT_NORMAL ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_PREDICT_RAW_SCORE ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_PREDICT_LEAF_INDEX ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_PREDICT_CONTRIB ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_MATRIX_TYPE_CSR ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_MATRIX_TYPE_CSC ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_FEATURE_IMPORTANCE_SPLIT ([python-package] prefix internal objects with '_' #5654)
• basic.C_API_FEATURE_IMPORTANCE_GAIN ([python-package] prefix internal objects with '_' #5654)
• basic.FIELD_TYPE_MAPPER ([python-package] prefix internal objects with '_' #5654)
• basic.FEATURE_IMPORTANCE_TYPE_MAPPER ([python-package] prefix internal objects with '_' #5654)
• basic.convert_from_sliced_object() ([python-package] prefix basic.convert_from_sliced_object with _ #5613)
• basic.c_float_array() ([python-package] prefix c_int_array and c_float_array with _ #5614)
• basic.c_int_array() ([python-package] prefix c_int_array and c_float_array with _ #5614)

[jmoralez]

Thanks for writing this up, I support it. Are we also adding the __all__? I think it's good having both.

[jameslamb]

Are we also adding the __all__? I think it's good having both.

There is already a __all__ controlling what happens when you from lightgbm import *.

LightGBM/python-package/lightgbm/__init__.py

Lines 30 to 36 in 1b43214

	__all__ = ['Dataset', 'Booster', 'CVBooster', 'Sequence',
	'register_logger',
	'train', 'cv',
	'LGBMModel', 'LGBMRegressor', 'LGBMClassifier', 'LGBMRanker',
	'DaskLGBMRegressor', 'DaskLGBMClassifier', 'DaskLGBMRanker',
	'log_evaluation', 'record_evaluation', 'reset_parameter', 'early_stopping',
	'plot_importance', 'plot_split_value_histogram', 'plot_metric', 'plot_tree', 'create_tree_digraph']

Are you talking about also adding a __all__ to each module, for example one in python-package/lightgbm/sklearn.py that would limit what gets imported by from lightgbm.sklearn import *?

I don't know enough about this to know whether or not that would work. Like I'm not sure if __all__ as a concept is specific to __init__.py files. But I'd be interested in trying it!

[jmoralez]

Yeah I meant at the module level, I believe it would be very useful for callback for example.

Edit: Adding reference

The public names defined by a module are determined by checking the module’s namespace for a variable named _all_

https://docs.python.org/3/reference/simple_stmts.html#import

Edit2:
On that same paragraph it says:

If _all_ is not defined, the set of public names includes all names found in the module’s namespace which do not begin with an underscore character ('_')

So we can set __all__ on the modules as a first step and that would avoid importing the current internal functions that aren't prefixed by _.

[jameslamb]

Ok nice! I hadn't thought of that. Totally support that idea. I think that new public classes and functions are rare enough in this project that the communication benefit of doing that is worth the slight additional maintenance burden.

[StrikerRUS]

I'm +1 for

all objects in the Python package which are intended to be internal should have names beginning with _

Also I'm proposing rethinking which properties of Dataset, Booster and other public classes should be public.
For example, I don't think that pandas_categorical or handle properties should be public.

LightGBM/python-package/lightgbm/basic.py

Line 1200 in 1b43214

self.pandas_categorical = None

LightGBM/python-package/lightgbm/basic.py

Line 2581 in 1b43214

self.handle = None

[guolinke]

I'm +1

[shiyu1994]

I'm +1 for this proposal! Thanks.

[jameslamb]

Ok thanks all! I'll update the description and convert this from an RFC to a regular maintenance issue, similar to #3756 .

[jameslamb]

I've updated the description and title to describe how to contribute on this initiative. @StrikerRUS @jmoralez feel free to edit it with any changes you'd like!

For example, I didn't list out any class attributes yet (but agree with the idea in #5313 (comment)).

[jameslamb]

I'm glad we originally opened this as a good first issue and that some contributors made their first PRs into the project working on it!

But I'm going to take this over and finish it. It's been open for 6 months, and I'd like to get these changes in for v4.0.0 (#5153).

[jameslamb]

The last piece of this is @StrikerRUS 's proposal to also make some more of the class attributes for public classes private.

Here's my proposal for which class attributes I think we should make private in the v4.0.0 release:

• Booster.handle
  • ctypes pointer to the Booster object on the C++ side. Keeping this private would allow changing the implementation for how that reference is managed
• Booster.network
  • used to keep track of whether or not this Booster is actively participating in distributed training, users should only be modifying this via public method Booster.free_network()
  • [python-package] make some Booster and Dataset attributes private #5723
• Dataset.handle
  • ctypes pointer to the Dataset object on the C++ side. Keeping this private would allow changing the implementation for how that reference is managed
• Dataset.params_back_up
  • implementation detail for parameter-updating, users shouldn't ever think about this
  • [python-package] make some Booster and Dataset attributes private #5723
• Dataset.need_slice
  • used to defer when slicing actually happens when used_indices is modified (including by Dataset.subset()). Users should never be modifying this directly
  • [python-package] make some Booster and Dataset attributes private #5723
• no changes to lightgbm.dask classes
• no changes to lightgbm.sklearn classes
• no changes to lightgbm.basic.CVBooster

@jmoralez @guolinke @StrikerRUS @shiyu1994 @btrotta what do you think?

[github-actions]

This issue 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.