Skip to content

Conversation

@jameslamb
Copy link
Collaborator

Contributes to #3756

Enforces keyword-only arguments in more internal functions, to make the Python codebase a bit stricter and easier to follow.

@jameslamb jameslamb changed the title WIP: [python-package] enforce keyword-only args in more internal functions [python-package] enforce keyword-only args in more internal functions Jan 5, 2026
@jameslamb jameslamb marked this pull request as ready for review January 5, 2026 17:58
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just one minor question below

self.first_metric = ""

def _gt_delta(self, curr_score: float, best_score: float, delta: float) -> bool:
def _gt_delta(self, curr_score: float, best_score: float, *, delta: float) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why not after self?

Copy link
Collaborator Author

@jameslamb jameslamb Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that a binary "op" type of function should always begin with 2 positional arguments, just something I'm used to from other languages.

But I see this is only called by lightgbm's own code (not passed through anything from functools or something) and is called like any other normal function:

if first_time_updating_best_score_list or self.cmp_op[i](metric_value, self.best_score[i]):

Seeing that, I think keyword arguments there would improve clarity. I'll make that change (and for _lt_delta).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 2e48184

I had to add a # type: ignore[call-arg] comment. I think mypy is confused by the use of partial() here:

self.cmp_op.append(partial(self._gt_delta, delta=delta))

python-package/lightgbm/callback.py:416: error: Unexpected keyword argument "curr_score"  [call-arg]
python-package/lightgbm/callback.py:416: error: Unexpected keyword argument "best_score"  [call-arg]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making calls of these functions even more clear.

Copy link
Collaborator

@borchero borchero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants