Add arguments of versions to `convert_positional_args` by fusawa-yugo · Pull Request #6009 · optuna/optuna

fusawa-yugo · 2025-03-13T07:05:35Z

Motivation

This PR is from #5485

Description of the changes

I added arguments and warning for deprecated_version and removed_version, referring to deprecated_func
The format of warning is:

"Positional arguments in {name} have been deprecated since v{d_ver}. "
"They will be removed in v{r_ver}. "
"Please use keyword arguments instead. "
"See https://github.com/optuna/optuna/releases/tag/v{d_ver} for details."

c-bata · 2025-03-17T01:56:30Z

@not522 @porink0424 Could you review this PR?

not522

Thank you for your PR! Could you check my comments?

not522 · 2025-03-17T06:16:03Z

optuna/_convert_positional_args.py

+    deprecated_version: str | None = None,
+    removed_version: str | None = None,


Let me confirm whether we will make the arguments optional. I recommend it is temporarily optional, and we will make it required on follow-up. How about that?

I agree with you.

~~Although I have no preference, I think it would be better to make the input typings stricter based on LSP.~~

~~The reason behind this comes from the fact that looser input typings implicitly assume that the function have a routine where we handle the possible input typings.~~
~~Namely, we have the processing for removed_version=None in this case and everywhere this function is called may use removed_version=None, which is a bit annoying to update later.~~
~~As we have the guidelines for deprecation, I think it would be better to not have optional for now.~~

Btw, is removed_version actually replaced_version?

Sorry, now I noticed that most of them have not specified version, so we need to make it optional for now, probably.

optuna/_convert_positional_args.py

porink0424

Thanks for your PR, I've left some comments.

optuna/_convert_positional_args.py

fusawa-yugo · 2025-03-27T04:51:20Z

Thank you for reviews!!
I revised the warnings.
Warning is displayed like below.

Test function

@convert_positional_args(
    previous_positional_arg_names=["a", "b"],
    deprecated_version="3.0.0",
    removed_version="4.0.0",
)
def example_function(a: int, b: int, c: int = 3, d: int = 4) -> None:
    print(f"a: {a}, b: {b}, c: {c}, d: {d}")

example_function(1, 2)

Warning

FutureWarning: Positional arguments ['a', 'b'] in example_function() have been deprecated since v3.0.0. They will be removed in v4.0.0. Please use keyword arguments instead. See https://github.com/optuna/optuna/releases/tag/v3.0.0 for details.
  example_function(1, 2)
a: 1, b: 2, c: 3, d: 4

not522 · 2025-03-28T02:38:43Z

optuna/_convert_positional_args.py

            positional_arg_names = _get_positional_arg_names(func)
            inferred_kwargs = _infer_kwargs(previous_positional_arg_names, *args)
+
+            if inferred_kwargs and (deprecated_version or removed_version):


inferred_kwargs include ordinal positional arguments. So we should check the lengths of them.

Suggested change

if inferred_kwargs and (deprecated_version or removed_version):

if len(inferred_kwargs) > len(positional_arg_names) and (deprecated_version or removed_version):

not522 · 2025-03-28T02:46:58Z

Thank you for your update! It's almost OK.

How about swap the order of warning messages? For example, the following outputs the versions first, but I think It's better to first output what users should do (i.e., example_function() got {'b', 'a'} as ...).

from optuna._convert_positional_args import convert_positional_args

@convert_positional_args(
    previous_positional_arg_names=["a", "b", "c", "d"],
    deprecated_version="3.0.0",
    removed_version="4.0.0",
)
def example_function(*, a: int, b: int, c: int = 3, d: int = 4) -> None:
    print(f"a: {a}, b: {b}, c: {c}, d: {d}")

example_function(1, 2)

/Users/naotomizuno/optuna/tmp_convert.py:11: FutureWarning: Positional arguments ['a', 'b', 'c', 'd'] in example_function() have been deprecated since v3.0.0. They will be removed in v4.0.0. Please use keyword arguments instead. See https://github.com/optuna/optuna/releases/tag/v3.0.0 for details.
example_function() got {'b', 'a'} as positional arguments but they were expected to be given as keyword arguments.

fusawa-yugo · 2025-03-28T04:55:29Z

Thank you for your comments again!!
I updated the code.
Please review it again!

not522

LGTM!

c-bata · 2025-03-28T05:23:02Z

@HideakiImamura I unassigned @porink0424 since he is busy for a while. Could you assign an additional reviewer?

optuna/_convert_positional_args.py

nabenabe0928

Thank you for the PR, i left some comments:)

nabenabe0928 · 2025-04-01T05:41:21Z

optuna/_convert_positional_args.py

+    deprecated_version: str | None = None,
+    removed_version: str | None = None,


Sorry, now I noticed that most of them have not specified version, so we need to make it optional for now, probably.

nabenabe0928 · 2025-04-01T05:44:37Z

optuna/_convert_positional_args.py

+    if deprecated_version is None and removed_version is None:
+        pass
+    else:


Suggested change

if deprecated_version is None and removed_version is None:

pass

else:

if deprecated_version is not None or removed_version is not None:

fusawa-yugo · 2025-04-03T05:55:54Z

Thank you for review!!
Could you check it again?

nabenabe0928 · 2025-04-07T04:04:38Z

optuna/_convert_positional_args.py

+    "Positional arguments {deprecated_positional_arg_names} in {func_name}() "
+    "have been deprecated since v{d_ver}. "
+    "They will be replaced with the corresponding keyword arguments in v{r_ver}, "
+    "so please use keyword specification instead. "


Suggested change

"so please use keyword specification instead. "

"so please use the keyword specification instead. "

optuna/_convert_positional_args.py

nabenabe0928

Thank you for the changes, almost LGTM!

nabenabe0928

Thank you for the update, LGTM!

fusawa-yugo added 2 commits March 13, 2025 15:54

Add arguments of versions to convert_positional_args

82d8add

Fix to pass checker

f8a01b8

nabenabe0928 changed the title ~~Add arguments of versions to convert_positional_args~~ Add arguments of versions to convert_positional_args Mar 14, 2025

c-bata assigned not522 and porink0424 Mar 17, 2025

c-bata added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Mar 17, 2025

not522 reviewed Mar 17, 2025

View reviewed changes

porink0424 reviewed Mar 19, 2025

View reviewed changes

fusawa-yugo added 2 commits March 27, 2025 13:43

Updated format of warning

dd05c95

Updated format of warning

8c6b701

not522 reviewed Mar 28, 2025

View reviewed changes

Minor Fix

76e61c5

not522 approved these changes Mar 28, 2025

View reviewed changes

not522 removed their assignment Mar 28, 2025

c-bata unassigned porink0424 Mar 28, 2025

nabenabe0928 self-assigned this Mar 31, 2025

nabenabe0928 reviewed Apr 1, 2025

View reviewed changes

optuna/_convert_positional_args.py Outdated Show resolved Hide resolved

nabenabe0928 reviewed Apr 1, 2025

View reviewed changes

optuna/_convert_positional_args.py Outdated Show resolved Hide resolved

nabenabe0928 reviewed Apr 1, 2025

View reviewed changes

lint

db32402

nabenabe0928 reviewed Apr 7, 2025

View reviewed changes

optuna/_convert_positional_args.py Outdated Show resolved Hide resolved

nabenabe0928 reviewed Apr 7, 2025

View reviewed changes

revert

6c85b2a

Fix

ebd8c12

nabenabe0928 approved these changes Apr 10, 2025

View reviewed changes

nabenabe0928 merged commit 11ff024 into optuna:master Apr 10, 2025
14 checks passed

nabenabe0928 removed their assignment Apr 10, 2025

nabenabe0928 added this to the v4.3.0 milestone Apr 10, 2025

		deprecated_version: str \| None = None,
		removed_version: str \| None = None,

	if inferred_kwargs and (deprecated_version or removed_version):
	if len(inferred_kwargs) > len(positional_arg_names) and (deprecated_version or removed_version):

	"so please use keyword specification instead. "
	"so please use the keyword specification instead. "

Uh oh!

Conversation

fusawa-yugo commented Mar 13, 2025

Motivation

Description of the changes

Uh oh!

c-bata commented Mar 17, 2025

Uh oh!

not522 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabenabe0928 Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

porink0424 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fusawa-yugo commented Mar 27, 2025

Test function

Warning

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not522 commented Mar 28, 2025

Uh oh!

fusawa-yugo commented Mar 28, 2025

Uh oh!

not522 left a comment

Choose a reason for hiding this comment

Uh oh!

c-bata commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

nabenabe0928 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fusawa-yugo commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nabenabe0928 left a comment

Choose a reason for hiding this comment

Uh oh!

nabenabe0928 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nabenabe0928 Apr 1, 2025 •

edited

Loading